| Message ID | 20230110080756.38271-2-peter@pjd.dev |
|---|---|
| State | New |
| Headers | show |
| Series | python/machine: Fix AF_UNIX path too long | expand |
On 10/1/23 09:07, Peter Delevoryas wrote: > On macOS, private $TMPDIR's are the default. These $TMPDIR's are > generated from a user's unix UID and UUID [1], which can create a > relatively long path: > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > QEMU's avocado tests create a temporary directory prefixed by > "avo_qemu_sock_", and create QMP sockets within _that_ as well. > The QMP socket is unnecessarily long, because a temporary directory > is created for every QEMUMachine object. > > /avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock > > The path limit for unix sockets on macOS is 104: [2] > > /* > * [XSI] Definitions for UNIX IPC domain. > */ > struct sockaddr_un { > unsigned char sun_len; /* sockaddr len including null */ > sa_family_t sun_family; /* [XSI] AF_UNIX */ > char sun_path[104]; /* [XSI] path name (gag) */ > }; > > This results in avocado tests failing on macOS because the QMP unix > socket can't be created, because the path is too long: > > ERROR| Failed to establish connection: OSError: AF_UNIX path too long Ah, I'm using this klugde: $ TMPDIR=/tmp avocado run ... > This change resolves by reducing the size of the socket directory prefix > and the suffix on the QMP and console socket names. > > The result is paths like this: > > pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T > $ tree qemu* > qemu_df4evjeq > qemu_jbxel3gy > qemu_ml9s_gg7 > qemu_oc7h7f3u > qemu_oqb1yf97 > ├── 10a004050.con > └── 10a004050.qmp > > [1] https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path > [2] /Library/Developer/CommandLineTools/SDKs/MacOSX12.3.sdk/usr/include/sys/un.h > > Signed-off-by: Peter Delevoryas <peter@pjd.dev> > --- > python/qemu/machine/machine.py | 6 +++--- > tests/avocado/avocado_qemu/__init__.py | 2 +- > 2 files changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Tue, Jan 10, 2023 at 12:07:56AM -0800, Peter Delevoryas wrote: > On macOS, private $TMPDIR's are the default. These $TMPDIR's are > generated from a user's unix UID and UUID [1], which can create a > relatively long path: > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > QEMU's avocado tests create a temporary directory prefixed by > "avo_qemu_sock_", and create QMP sockets within _that_ as well. > The QMP socket is unnecessarily long, because a temporary directory > is created for every QEMUMachine object. > > /avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock > > The path limit for unix sockets on macOS is 104: [2] > > /* > * [XSI] Definitions for UNIX IPC domain. > */ > struct sockaddr_un { > unsigned char sun_len; /* sockaddr len including null */ > sa_family_t sun_family; /* [XSI] AF_UNIX */ > char sun_path[104]; /* [XSI] path name (gag) */ > }; > > This results in avocado tests failing on macOS because the QMP unix > socket can't be created, because the path is too long: > > ERROR| Failed to establish connection: OSError: AF_UNIX path too long > > This change resolves by reducing the size of the socket directory prefix > and the suffix on the QMP and console socket names. > > The result is paths like this: > > pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T > $ tree qemu* > qemu_df4evjeq > qemu_jbxel3gy > qemu_ml9s_gg7 > qemu_oc7h7f3u > qemu_oqb1yf97 > ├── 10a004050.con > └── 10a004050.qmp > > [1] https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path > [2] /Library/Developer/CommandLineTools/SDKs/MacOSX12.3.sdk/usr/include/sys/un.h > > Signed-off-by: Peter Delevoryas <peter@pjd.dev> > --- > python/qemu/machine/machine.py | 6 +++--- > tests/avocado/avocado_qemu/__init__.py | 2 +- > 2 files changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
On Tue, Jan 10, 2023 at 3:34 AM Peter Delevoryas <peter@pjd.dev> wrote: > > On macOS, private $TMPDIR's are the default. These $TMPDIR's are > generated from a user's unix UID and UUID [1], which can create a > relatively long path: > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > QEMU's avocado tests create a temporary directory prefixed by > "avo_qemu_sock_", and create QMP sockets within _that_ as well. > The QMP socket is unnecessarily long, because a temporary directory > is created for every QEMUMachine object. > > /avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock > > The path limit for unix sockets on macOS is 104: [2] > > /* > * [XSI] Definitions for UNIX IPC domain. > */ > struct sockaddr_un { > unsigned char sun_len; /* sockaddr len including null */ > sa_family_t sun_family; /* [XSI] AF_UNIX */ > char sun_path[104]; /* [XSI] path name (gag) */ > }; > > This results in avocado tests failing on macOS because the QMP unix > socket can't be created, because the path is too long: > > ERROR| Failed to establish connection: OSError: AF_UNIX path too long > > This change resolves by reducing the size of the socket directory prefix > and the suffix on the QMP and console socket names. > > The result is paths like this: > > pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T > $ tree qemu* > qemu_df4evjeq > qemu_jbxel3gy > qemu_ml9s_gg7 > qemu_oc7h7f3u > qemu_oqb1yf97 > ├── 10a004050.con > └── 10a004050.qmp > > [1] https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path > [2] /Library/Developer/CommandLineTools/SDKs/MacOSX12.3.sdk/usr/include/sys/un.h > > Signed-off-by: Peter Delevoryas <peter@pjd.dev> I'm tentatively staging this with a benefit-of-the-doubt [1] -- my tests are still running -- but I do have a question: > --- > python/qemu/machine/machine.py | 6 +++--- > tests/avocado/avocado_qemu/__init__.py | 2 +- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py > index 748a0d807c9d..d70977378305 100644 > --- a/python/qemu/machine/machine.py > +++ b/python/qemu/machine/machine.py > @@ -157,7 +157,7 @@ def __init__(self, > self._wrapper = wrapper > self._qmp_timer = qmp_timer > > - self._name = name or f"qemu-{os.getpid()}-{id(self):02x}" > + self._name = name or f"{id(self):x}" Why is it safe to not differentiate based on the process ID? ... I suppose the thinking is: by default, in machine.py, this is a temp dir created by tempfile.mkdtemp which will be unique per-process. I suppose there's no protection against a caller supplying the same tempdir (or sockdir) to multiple instances, but I suppose in those cases we get to argue that "Well, don't do that, then." Does that sound about right? --js [1] staged @ https://gitlab.com/jsnow/qemu/-/commits/python > self._temp_dir: Optional[str] = None > self._base_temp_dir = base_temp_dir > self._sock_dir = sock_dir > @@ -167,7 +167,7 @@ def __init__(self, > self._monitor_address = monitor_address > else: > self._monitor_address = os.path.join( > - self.sock_dir, f"{self._name}-monitor.sock" > + self.sock_dir, f"{self._name}.qmp" > ) > > self._console_log_path = console_log > @@ -192,7 +192,7 @@ def __init__(self, > self._console_set = False > self._console_device_type: Optional[str] = None > self._console_address = os.path.join( > - self.sock_dir, f"{self._name}-console.sock" > + self.sock_dir, f"{self._name}.con" > ) > self._console_socket: Optional[socket.socket] = None > self._remove_files: List[str] = [] > diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py > index 910f3ba1eab8..25a546842fab 100644 > --- a/tests/avocado/avocado_qemu/__init__.py > +++ b/tests/avocado/avocado_qemu/__init__.py > @@ -306,7 +306,7 @@ def require_netdev(self, netdevname): > self.cancel('no support for user networking') > > def _new_vm(self, name, *args): > - self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_") > + self._sd = tempfile.TemporaryDirectory(prefix="qemu_") > vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir, > sock_dir=self._sd.name, log_dir=self.logdir) > self.log.debug('QEMUMachine "%s" created', name) > -- > 2.39.0 > >
On Tue, Jan 10, 2023 at 06:18:29PM -0500, John Snow wrote: > On Tue, Jan 10, 2023 at 3:34 AM Peter Delevoryas <peter@pjd.dev> wrote: > > > > On macOS, private $TMPDIR's are the default. These $TMPDIR's are > > generated from a user's unix UID and UUID [1], which can create a > > relatively long path: > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > QEMU's avocado tests create a temporary directory prefixed by > > "avo_qemu_sock_", and create QMP sockets within _that_ as well. > > The QMP socket is unnecessarily long, because a temporary directory > > is created for every QEMUMachine object. > > > > /avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock > > > > The path limit for unix sockets on macOS is 104: [2] > > > > /* > > * [XSI] Definitions for UNIX IPC domain. > > */ > > struct sockaddr_un { > > unsigned char sun_len; /* sockaddr len including null */ > > sa_family_t sun_family; /* [XSI] AF_UNIX */ > > char sun_path[104]; /* [XSI] path name (gag) */ > > }; > > > > This results in avocado tests failing on macOS because the QMP unix > > socket can't be created, because the path is too long: > > > > ERROR| Failed to establish connection: OSError: AF_UNIX path too long > > > > This change resolves by reducing the size of the socket directory prefix > > and the suffix on the QMP and console socket names. > > > > The result is paths like this: > > > > pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T > > $ tree qemu* > > qemu_df4evjeq > > qemu_jbxel3gy > > qemu_ml9s_gg7 > > qemu_oc7h7f3u > > qemu_oqb1yf97 > > ├── 10a004050.con > > └── 10a004050.qmp > > > > [1] https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path > > [2] /Library/Developer/CommandLineTools/SDKs/MacOSX12.3.sdk/usr/include/sys/un.h > > > > Signed-off-by: Peter Delevoryas <peter@pjd.dev> > > I'm tentatively staging this with a benefit-of-the-doubt [1] -- my > tests are still running -- but I do have a question: > > > --- > > python/qemu/machine/machine.py | 6 +++--- > > tests/avocado/avocado_qemu/__init__.py | 2 +- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py > > index 748a0d807c9d..d70977378305 100644 > > --- a/python/qemu/machine/machine.py > > +++ b/python/qemu/machine/machine.py > > @@ -157,7 +157,7 @@ def __init__(self, > > self._wrapper = wrapper > > self._qmp_timer = qmp_timer > > > > - self._name = name or f"qemu-{os.getpid()}-{id(self):02x}" > > + self._name = name or f"{id(self):x}" > > Why is it safe to not differentiate based on the process ID? > > ... I suppose the thinking is: by default, in machine.py, this is a > temp dir created by tempfile.mkdtemp which will be unique per-process. > I suppose there's no protection against a caller supplying the same > tempdir (or sockdir) to multiple instances, but I suppose in those > cases we get to argue that "Well, don't do that, then." > > Does that sound about right? Yeah, I think that's it > > --js > > [1] staged @ https://gitlab.com/jsnow/qemu/-/commits/python > > > > self._temp_dir: Optional[str] = None > > self._base_temp_dir = base_temp_dir > > self._sock_dir = sock_dir > > @@ -167,7 +167,7 @@ def __init__(self, > > self._monitor_address = monitor_address > > else: > > self._monitor_address = os.path.join( > > - self.sock_dir, f"{self._name}-monitor.sock" > > + self.sock_dir, f"{self._name}.qmp" > > ) > > > > self._console_log_path = console_log > > @@ -192,7 +192,7 @@ def __init__(self, > > self._console_set = False > > self._console_device_type: Optional[str] = None > > self._console_address = os.path.join( > > - self.sock_dir, f"{self._name}-console.sock" > > + self.sock_dir, f"{self._name}.con" > > ) > > self._console_socket: Optional[socket.socket] = None > > self._remove_files: List[str] = [] > > diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py > > index 910f3ba1eab8..25a546842fab 100644 > > --- a/tests/avocado/avocado_qemu/__init__.py > > +++ b/tests/avocado/avocado_qemu/__init__.py > > @@ -306,7 +306,7 @@ def require_netdev(self, netdevname): > > self.cancel('no support for user networking') > > > > def _new_vm(self, name, *args): > > - self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_") > > + self._sd = tempfile.TemporaryDirectory(prefix="qemu_") > > vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir, > > sock_dir=self._sd.name, log_dir=self.logdir) > > self.log.debug('QEMUMachine "%s" created', name) > > -- > > 2.39.0 > > > > >
On Tue, Jan 10, 2023 at 06:18:29PM -0500, John Snow wrote: > On Tue, Jan 10, 2023 at 3:34 AM Peter Delevoryas <peter@pjd.dev> wrote: > > > > On macOS, private $TMPDIR's are the default. These $TMPDIR's are > > generated from a user's unix UID and UUID [1], which can create a > > relatively long path: > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > QEMU's avocado tests create a temporary directory prefixed by > > "avo_qemu_sock_", and create QMP sockets within _that_ as well. > > The QMP socket is unnecessarily long, because a temporary directory > > is created for every QEMUMachine object. > > > > /avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock > > > > The path limit for unix sockets on macOS is 104: [2] > > > > /* > > * [XSI] Definitions for UNIX IPC domain. > > */ > > struct sockaddr_un { > > unsigned char sun_len; /* sockaddr len including null */ > > sa_family_t sun_family; /* [XSI] AF_UNIX */ > > char sun_path[104]; /* [XSI] path name (gag) */ > > }; > > > > This results in avocado tests failing on macOS because the QMP unix > > socket can't be created, because the path is too long: > > > > ERROR| Failed to establish connection: OSError: AF_UNIX path too long > > > > This change resolves by reducing the size of the socket directory prefix > > and the suffix on the QMP and console socket names. > > > > The result is paths like this: > > > > pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T > > $ tree qemu* > > qemu_df4evjeq > > qemu_jbxel3gy > > qemu_ml9s_gg7 > > qemu_oc7h7f3u > > qemu_oqb1yf97 > > ├── 10a004050.con > > └── 10a004050.qmp > > > > [1] https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path > > [2] /Library/Developer/CommandLineTools/SDKs/MacOSX12.3.sdk/usr/include/sys/un.h > > > > Signed-off-by: Peter Delevoryas <peter@pjd.dev> > > I'm tentatively staging this with a benefit-of-the-doubt [1] -- my > tests are still running -- but I do have a question: > > > --- > > python/qemu/machine/machine.py | 6 +++--- > > tests/avocado/avocado_qemu/__init__.py | 2 +- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py > > index 748a0d807c9d..d70977378305 100644 > > --- a/python/qemu/machine/machine.py > > +++ b/python/qemu/machine/machine.py > > @@ -157,7 +157,7 @@ def __init__(self, > > self._wrapper = wrapper > > self._qmp_timer = qmp_timer > > > > - self._name = name or f"qemu-{os.getpid()}-{id(self):02x}" > > + self._name = name or f"{id(self):x}" > > Why is it safe to not differentiate based on the process ID? > > ... I suppose the thinking is: by default, in machine.py, this is a > temp dir created by tempfile.mkdtemp which will be unique per-process. > I suppose there's no protection against a caller supplying the same > tempdir (or sockdir) to multiple instances, but I suppose in those > cases we get to argue that "Well, don't do that, then." Every process will have a separate tempdir, and if there are multiple instances of this class, 'id(self)' will provide uniqueness within the process. With regards, Daniel
On Wed, Jan 11, 2023 at 4:07 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Tue, Jan 10, 2023 at 06:18:29PM -0500, John Snow wrote: > > On Tue, Jan 10, 2023 at 3:34 AM Peter Delevoryas <peter@pjd.dev> wrote: > > > > > > On macOS, private $TMPDIR's are the default. These $TMPDIR's are > > > generated from a user's unix UID and UUID [1], which can create a > > > relatively long path: > > > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > > > QEMU's avocado tests create a temporary directory prefixed by > > > "avo_qemu_sock_", and create QMP sockets within _that_ as well. > > > The QMP socket is unnecessarily long, because a temporary directory > > > is created for every QEMUMachine object. > > > > > > /avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock > > > > > > The path limit for unix sockets on macOS is 104: [2] > > > > > > /* > > > * [XSI] Definitions for UNIX IPC domain. > > > */ > > > struct sockaddr_un { > > > unsigned char sun_len; /* sockaddr len including null */ > > > sa_family_t sun_family; /* [XSI] AF_UNIX */ > > > char sun_path[104]; /* [XSI] path name (gag) */ > > > }; > > > > > > This results in avocado tests failing on macOS because the QMP unix > > > socket can't be created, because the path is too long: > > > > > > ERROR| Failed to establish connection: OSError: AF_UNIX path too long > > > > > > This change resolves by reducing the size of the socket directory prefix > > > and the suffix on the QMP and console socket names. > > > > > > The result is paths like this: > > > > > > pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T > > > $ tree qemu* > > > qemu_df4evjeq > > > qemu_jbxel3gy > > > qemu_ml9s_gg7 > > > qemu_oc7h7f3u > > > qemu_oqb1yf97 > > > ├── 10a004050.con > > > └── 10a004050.qmp > > > > > > [1] https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path > > > [2] /Library/Developer/CommandLineTools/SDKs/MacOSX12.3.sdk/usr/include/sys/un.h > > > > > > Signed-off-by: Peter Delevoryas <peter@pjd.dev> > > > > I'm tentatively staging this with a benefit-of-the-doubt [1] -- my > > tests are still running -- but I do have a question: > > > > > --- > > > python/qemu/machine/machine.py | 6 +++--- > > > tests/avocado/avocado_qemu/__init__.py | 2 +- > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py > > > index 748a0d807c9d..d70977378305 100644 > > > --- a/python/qemu/machine/machine.py > > > +++ b/python/qemu/machine/machine.py > > > @@ -157,7 +157,7 @@ def __init__(self, > > > self._wrapper = wrapper > > > self._qmp_timer = qmp_timer > > > > > > - self._name = name or f"qemu-{os.getpid()}-{id(self):02x}" > > > + self._name = name or f"{id(self):x}" > > > > Why is it safe to not differentiate based on the process ID? > > > > ... I suppose the thinking is: by default, in machine.py, this is a > > temp dir created by tempfile.mkdtemp which will be unique per-process. > > I suppose there's no protection against a caller supplying the same > > tempdir (or sockdir) to multiple instances, but I suppose in those > > cases we get to argue that "Well, don't do that, then." > > Every process will have a separate tempdir, and if there are > multiple instances of this class, 'id(self)' will provide > uniqueness within the process. Right. The only small thing is if a caller passes the same directory to multiple instances across multiple processes, you could *theoretically* get a collision, and we don't guard against it. It's not a super likely occurrence so I'm fine with ignoring it, but I think it's technically possible.
On Wed, Jan 11, 2023 at 10:06:49AM -0500, John Snow wrote: > On Wed, Jan 11, 2023 at 4:07 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > On Tue, Jan 10, 2023 at 06:18:29PM -0500, John Snow wrote: > > > On Tue, Jan 10, 2023 at 3:34 AM Peter Delevoryas <peter@pjd.dev> wrote: > > > > > > > > On macOS, private $TMPDIR's are the default. These $TMPDIR's are > > > > generated from a user's unix UID and UUID [1], which can create a > > > > relatively long path: > > > > > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > > > > > QEMU's avocado tests create a temporary directory prefixed by > > > > "avo_qemu_sock_", and create QMP sockets within _that_ as well. > > > > The QMP socket is unnecessarily long, because a temporary directory > > > > is created for every QEMUMachine object. > > > > > > > > /avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock > > > > > > > > The path limit for unix sockets on macOS is 104: [2] > > > > > > > > /* > > > > * [XSI] Definitions for UNIX IPC domain. > > > > */ > > > > struct sockaddr_un { > > > > unsigned char sun_len; /* sockaddr len including null */ > > > > sa_family_t sun_family; /* [XSI] AF_UNIX */ > > > > char sun_path[104]; /* [XSI] path name (gag) */ > > > > }; > > > > > > > > This results in avocado tests failing on macOS because the QMP unix > > > > socket can't be created, because the path is too long: > > > > > > > > ERROR| Failed to establish connection: OSError: AF_UNIX path too long > > > > > > > > This change resolves by reducing the size of the socket directory prefix > > > > and the suffix on the QMP and console socket names. > > > > > > > > The result is paths like this: > > > > > > > > pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T > > > > $ tree qemu* > > > > qemu_df4evjeq > > > > qemu_jbxel3gy > > > > qemu_ml9s_gg7 > > > > qemu_oc7h7f3u > > > > qemu_oqb1yf97 > > > > ├── 10a004050.con > > > > └── 10a004050.qmp > > > > > > > > [1] https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path > > > > [2] /Library/Developer/CommandLineTools/SDKs/MacOSX12.3.sdk/usr/include/sys/un.h > > > > > > > > Signed-off-by: Peter Delevoryas <peter@pjd.dev> > > > > > > I'm tentatively staging this with a benefit-of-the-doubt [1] -- my > > > tests are still running -- but I do have a question: > > > > > > > --- > > > > python/qemu/machine/machine.py | 6 +++--- > > > > tests/avocado/avocado_qemu/__init__.py | 2 +- > > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py > > > > index 748a0d807c9d..d70977378305 100644 > > > > --- a/python/qemu/machine/machine.py > > > > +++ b/python/qemu/machine/machine.py > > > > @@ -157,7 +157,7 @@ def __init__(self, > > > > self._wrapper = wrapper > > > > self._qmp_timer = qmp_timer > > > > > > > > - self._name = name or f"qemu-{os.getpid()}-{id(self):02x}" > > > > + self._name = name or f"{id(self):x}" > > > > > > Why is it safe to not differentiate based on the process ID? > > > > > > ... I suppose the thinking is: by default, in machine.py, this is a > > > temp dir created by tempfile.mkdtemp which will be unique per-process. > > > I suppose there's no protection against a caller supplying the same > > > tempdir (or sockdir) to multiple instances, but I suppose in those > > > cases we get to argue that "Well, don't do that, then." > > > > Every process will have a separate tempdir, and if there are > > multiple instances of this class, 'id(self)' will provide > > uniqueness within the process. > > Right. The only small thing is if a caller passes the same directory > to multiple instances across multiple processes, you could > *theoretically* get a collision, and we don't guard against it. It's > not a super likely occurrence so I'm fine with ignoring it, but I > think it's technically possible. If they want to be insane, then they can also pass a 'name' explicitly to override the default socket path choice. With regards, Daniel
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index 748a0d807c9d..d70977378305 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -157,7 +157,7 @@ def __init__(self, self._wrapper = wrapper self._qmp_timer = qmp_timer - self._name = name or f"qemu-{os.getpid()}-{id(self):02x}" + self._name = name or f"{id(self):x}" self._temp_dir: Optional[str] = None self._base_temp_dir = base_temp_dir self._sock_dir = sock_dir @@ -167,7 +167,7 @@ def __init__(self, self._monitor_address = monitor_address else: self._monitor_address = os.path.join( - self.sock_dir, f"{self._name}-monitor.sock" + self.sock_dir, f"{self._name}.qmp" ) self._console_log_path = console_log @@ -192,7 +192,7 @@ def __init__(self, self._console_set = False self._console_device_type: Optional[str] = None self._console_address = os.path.join( - self.sock_dir, f"{self._name}-console.sock" + self.sock_dir, f"{self._name}.con" ) self._console_socket: Optional[socket.socket] = None self._remove_files: List[str] = [] diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py index 910f3ba1eab8..25a546842fab 100644 --- a/tests/avocado/avocado_qemu/__init__.py +++ b/tests/avocado/avocado_qemu/__init__.py @@ -306,7 +306,7 @@ def require_netdev(self, netdevname): self.cancel('no support for user networking') def _new_vm(self, name, *args): - self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_") + self._sd = tempfile.TemporaryDirectory(prefix="qemu_") vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir, sock_dir=self._sd.name, log_dir=self.logdir) self.log.debug('QEMUMachine "%s" created', name)
On macOS, private $TMPDIR's are the default. These $TMPDIR's are generated from a user's unix UID and UUID [1], which can create a relatively long path: /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ QEMU's avocado tests create a temporary directory prefixed by "avo_qemu_sock_", and create QMP sockets within _that_ as well. The QMP socket is unnecessarily long, because a temporary directory is created for every QEMUMachine object. /avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock The path limit for unix sockets on macOS is 104: [2] /* * [XSI] Definitions for UNIX IPC domain. */ struct sockaddr_un { unsigned char sun_len; /* sockaddr len including null */ sa_family_t sun_family; /* [XSI] AF_UNIX */ char sun_path[104]; /* [XSI] path name (gag) */ }; This results in avocado tests failing on macOS because the QMP unix socket can't be created, because the path is too long: ERROR| Failed to establish connection: OSError: AF_UNIX path too long This change resolves by reducing the size of the socket directory prefix and the suffix on the QMP and console socket names. The result is paths like this: pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T $ tree qemu* qemu_df4evjeq qemu_jbxel3gy qemu_ml9s_gg7 qemu_oc7h7f3u qemu_oqb1yf97 ├── 10a004050.con └── 10a004050.qmp [1] https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path [2] /Library/Developer/CommandLineTools/SDKs/MacOSX12.3.sdk/usr/include/sys/un.h Signed-off-by: Peter Delevoryas <peter@pjd.dev> --- python/qemu/machine/machine.py | 6 +++--- tests/avocado/avocado_qemu/__init__.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)