From d6d480d5611d5a412555d6887dbac983880123c3 Mon Sep 17 00:00:00 2001 From: Kaliko Jack Date: Tue, 6 Feb 2024 16:58:10 +0100 Subject: [PATCH] Gather more OSError exception in musicpd.ConnectionError --- CHANGES.txt | 2 ++ musicpd.py | 27 ++++++++++++------- test.py | 75 +++++++++++++++++++++++++++++++++++++++++++++++------ 3 files changed, 87 insertions(+), 17 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index d32426a..52e46d1 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -10,6 +10,8 @@ Changes in 0.9.0 * Improved Range object to deal with window parameter * Add logging * Switch to pyproject.toml (setuptools build system) + * The connect sequence raises ConnectionError only on error, + previously getaddrinfo or unix socket connection error raised an OSError Changes in 0.8.0 ---------------- diff --git a/musicpd.py b/musicpd.py index 8b5d789..d8dc01b 100644 --- a/musicpd.py +++ b/musicpd.py @@ -19,7 +19,7 @@ ERROR_PREFIX = "ACK " SUCCESS = "OK" NEXT = "list_OK" #: Module version -VERSION = '0.9.0b1' +VERSION = '0.9.0b2' #: Seconds before a connection attempt times out #: (overriden by :envvar:`MPD_TIMEOUT` env. var.) CONNECTION_TIMEOUT = 30 @@ -642,10 +642,13 @@ class MPDClient: # abstract socket if path.startswith('@'): path = '\0'+path[1:] - sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) - sock.settimeout(self.mpd_timeout) - sock.connect(path) - sock.settimeout(self.socket_timeout) + try: + sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + sock.settimeout(self.mpd_timeout) + sock.connect(path) + sock.settimeout(self.socket_timeout) + except socket.error as socket_err: + raise ConnectionError(socket_err.strerror) from socket_err return sock def _connect_tcp(self, host, port): @@ -654,23 +657,29 @@ class MPDClient: except AttributeError: flags = 0 err = None - for res in socket.getaddrinfo(host, port, socket.AF_UNSPEC, - socket.SOCK_STREAM, socket.IPPROTO_TCP, - flags): + try: + gai = socket.getaddrinfo(host, port, socket.AF_UNSPEC, + socket.SOCK_STREAM, socket.IPPROTO_TCP, + flags) + except socket.error as gaierr: + raise ConnectionError(gaierr.strerror) from gaierr + for res in gai: af, socktype, proto, _, sa = res sock = None try: + log.debug('opening socket %s', sa) sock = socket.socket(af, socktype, proto) sock.settimeout(self.mpd_timeout) sock.connect(sa) sock.settimeout(self.socket_timeout) return sock except socket.error as socket_err: + log.debug('opening socket %s failed: %s}', sa, socket_err) err = socket_err if sock is not None: sock.close() if err is not None: - raise ConnectionError(str(err)) + raise ConnectionError(err.strerror) raise ConnectionError("getaddrinfo returns an empty list") def noidle(self): diff --git a/test.py b/test.py index 033fe06..a123b30 100755 --- a/test.py +++ b/test.py @@ -1,6 +1,6 @@ #!/usr/bin/env python3 # coding: utf-8 -# SPDX-FileCopyrightText: 2012-2023 kaliko +# SPDX-FileCopyrightText: 2012-2024 kaliko # SPDX-License-Identifier: LGPL-3.0-or-later # pylint: disable=missing-docstring """ @@ -14,11 +14,12 @@ import itertools import os import types import unittest -import unittest.mock as mock +import unittest.mock import warnings import musicpd +mock = unittest.mock # show deprecation warnings warnings.simplefilter('default') @@ -27,7 +28,7 @@ warnings.simplefilter('default') TEST_MPD_HOST, TEST_MPD_PORT = ('example.com', 10000) -class testEnvVar(unittest.TestCase): +class TestEnvVar(unittest.TestCase): def test_envvar(self): # mock "os.path.exists" here to ensure there are no socket in @@ -559,7 +560,7 @@ class TestMPDClient(unittest.TestCase): with self.assertRaises(AttributeError): self.client.foo_bar() -class testConnection(unittest.TestCase): +class TestConnection(unittest.TestCase): def test_exposing_fileno(self): with mock.patch('musicpd.socket') as socket_mock: @@ -613,14 +614,72 @@ class testConnection(unittest.TestCase): # Set socket timeout Raises Exception with self.assertRaises(ValueError): cli.socket_timeout = 'foo' - with self.assertRaises(ValueError, msg='socket_timeout expects a non zero positive integer'): + with self.assertRaises(ValueError, + msg='socket_timeout expects a non zero positive integer'): cli.socket_timeout = '0' - with self.assertRaises(ValueError, msg='socket_timeout expects a non zero positive integer'): + with self.assertRaises(ValueError, + msg='socket_timeout expects a non zero positive integer'): cli.socket_timeout = '-1' -class testException(unittest.TestCase): - def test_CommandError_on_newline(self): +class TestConnectionError(unittest.TestCase): + + @mock.patch('socket.socket') + def test_connect_unix(self, socket_mock): + """Unix socket socket.error should raise a musicpd.ConnectionError""" + mocked_socket = socket_mock.return_value + mocked_socket.connect.side_effect = musicpd.socket.error(42, 'err 42') + os.environ['MPD_HOST'] = '/run/mpd/socket' + cli = musicpd.MPDClient() + with self.assertRaises(musicpd.ConnectionError) as cme: + cli.connect() + self.assertEqual('err 42', str(cme.exception)) + + def test_non_available_unix_socket(self): + delattr(musicpd.socket, 'AF_UNIX') + os.environ['MPD_HOST'] = '/run/mpd/socket' + cli = musicpd.MPDClient() + with self.assertRaises(musicpd.ConnectionError) as cme: + cli.connect() + self.assertEqual('Unix domain sockets not supported on this platform', + str(cme.exception)) + + @mock.patch('socket.getaddrinfo') + def test_connect_tcp_getaddrinfo(self, gai_mock): + """TCP socket.gaierror should raise a musicpd.ConnectionError""" + gai_mock.side_effect = musicpd.socket.error(42, 'gaierr 42') + cli = musicpd.MPDClient() + with self.assertRaises(musicpd.ConnectionError) as cme: + cli.connect(host=TEST_MPD_HOST) + self.assertEqual('gaierr 42', str(cme.exception)) + + @mock.patch('socket.getaddrinfo') + @mock.patch('socket.socket') + def test_connect_tcp_connect(self, socket_mock, gai_mock): + """A socket.error should raise a musicpd.ConnectionError + Mocking getaddrinfo to prevent network access (DNS) + """ + gai_mock.return_value = [range(5)] + mocked_socket = socket_mock.return_value + mocked_socket.connect.side_effect = musicpd.socket.error(42, 'tcp conn err 42') + cli = musicpd.MPDClient() + with self.assertRaises(musicpd.ConnectionError) as cme: + cli.connect(host=TEST_MPD_HOST) + self.assertEqual('tcp conn err 42', str(cme.exception)) + + @mock.patch('socket.getaddrinfo') + def test_connect_tcp_connect_empty_gai(self, gai_mock): + """An empty getaddrinfo should raise a musicpd.ConnectionError""" + gai_mock.return_value = [] + cli = musicpd.MPDClient() + with self.assertRaises(musicpd.ConnectionError) as cme: + cli.connect(host=TEST_MPD_HOST) + self.assertEqual('getaddrinfo returns an empty list', str(cme.exception)) + + +class TestCommandErrorException(unittest.TestCase): + + def test_error_on_newline(self): os.environ['MPD_HOST'] = '/run/mpd/socket' with mock.patch('musicpd.socket') as socket_mock: sock = mock.MagicMock(name='socket') -- 2.39.5