[PATCH v2 0/1] python/machine: Fix AF_UNIX path too long on macOS

[PATCH v2 0/1] python/machine: Fix AF_UNIX path too long on macOS

[Peter Delevoryas]

v2:
- Reduced QEMUMachine socket directory prefix length.
- Replaced QEMUMachine default name with "qmp".
- Rewrote commit description.

Thanks,
Peter Delevoryas

Peter Delevoryas (1):
  python/machine: Fix AF_UNIX path too long on macOS

 python/qemu/machine/machine.py         | 2 +-
 tests/avocado/avocado_qemu/__init__.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.37.0

[PATCH v2 1/1] python/machine: Fix AF_UNIX path too long on macOS

[Peter Delevoryas]

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 reduces the size of both paths, and removes the unique
identification information from the socket name, since it seems to be
unnecessary.

This commit produces paths like the following:

    pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T
    $ tree qemu*
    qemu_oc7h7f3u
    ├── qmp-console.sock
    └── qmp-monitor.sock

[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         | 2 +-
 tests/avocado/avocado_qemu/__init__.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 37191f433b..b1823966b3 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 "qmp"
         self._temp_dir: Optional[str] = None
         self._base_temp_dir = base_temp_dir
         self._sock_dir = sock_dir
diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
index ed4853c805..43b8c8848c 100644
--- a/tests/avocado/avocado_qemu/__init__.py
+++ b/tests/avocado/avocado_qemu/__init__.py
@@ -296,7 +296,7 @@ def require_accelerator(self, accelerator):
                         "available" % accelerator)
 
     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.37.0

Re: [PATCH v2 1/1] python/machine: Fix AF_UNIX path too long on macOS

[Daniel P. Berrangé]

On Sat, Jul 16, 2022 at 10:34:34AM -0700, 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


Looking at this again, I realize my suggestion for dealing with the
second part of the path was mistaken.

The "qemu-37331-10bacf110-monitor.sock" part is combining two
pieces.

First the result of

  f"qemu-{os.getpid()}-{id(self):02x}"

is

  qemu-37331-10bacf110

and the code later than appends '-monitor.sock'

So...

> 
> 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 reduces the size of both paths, and removes the unique
> identification information from the socket name, since it seems to be
> unnecessary.
> 
> This commit produces paths like the following:
> 
>     pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T
>     $ tree qemu*
>     qemu_oc7h7f3u
>     ├── qmp-console.sock
>     └── qmp-monitor.sock
> 
> [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         | 2 +-
>  tests/avocado/avocado_qemu/__init__.py | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> index 37191f433b..b1823966b3 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 "qmp"

...my suggestion here was wrong.

We don't need the os.getpid() unoiqueness because the tmpdir already
ensures that is safe, but keeping 'id(self)' is a good idea, if the
test case creates multiple machines concurrently. Bearing in mind we
later append '-monitor.sock' we don't need 'qmp' in the self._name.

So on reflection I think I should have suggested using:

    self._name = name or f"{id(self):02x}"

And *in addition*, a few lines later change:

            self._monitor_address = os.path.join(
                self.sock_dir, f"{self._name}-monitor.sock"
            )

To

            self._monitor_address = os.path.join(
                self.sock_dir, f"{self._name}.qmp"
            )


> diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
> index ed4853c805..43b8c8848c 100644
> --- a/tests/avocado/avocado_qemu/__init__.py
> +++ b/tests/avocado/avocado_qemu/__init__.py
> @@ -296,7 +296,7 @@ def require_accelerator(self, accelerator):
>                          "available" % accelerator)
>  
>      def _new_vm(self, name, *args):
> -        self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
> +        self._sd = tempfile.TemporaryDirectory(prefix="qemu_")

This bit is fine.

>          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)

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 1/1] python/machine: Fix AF_UNIX path too long on macOS

[Peter Delevoryas]

On Mon, Jul 18, 2022 at 09:56:17AM +0100, Daniel P. Berrangé wrote:
> On Sat, Jul 16, 2022 at 10:34:34AM -0700, 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
> 
> 
> Looking at this again, I realize my suggestion for dealing with the
> second part of the path was mistaken.
> 
> The "qemu-37331-10bacf110-monitor.sock" part is combining two
> pieces.
> 
> First the result of
> 
>   f"qemu-{os.getpid()}-{id(self):02x}"
> 
> is
> 
>   qemu-37331-10bacf110
> 
> and the code later than appends '-monitor.sock'
> 
> So...
> 
> > 
> > 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 reduces the size of both paths, and removes the unique
> > identification information from the socket name, since it seems to be
> > unnecessary.
> > 
> > This commit produces paths like the following:
> > 
> >     pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T
> >     $ tree qemu*
> >     qemu_oc7h7f3u
> >     ├── qmp-console.sock
> >     └── qmp-monitor.sock
> > 
> > [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         | 2 +-
> >  tests/avocado/avocado_qemu/__init__.py | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > index 37191f433b..b1823966b3 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 "qmp"
> 
> ...my suggestion here was wrong.
> 
> We don't need the os.getpid() unoiqueness because the tmpdir already
> ensures that is safe, but keeping 'id(self)' is a good idea, if the
> test case creates multiple machines concurrently. Bearing in mind we
> later append '-monitor.sock' we don't need 'qmp' in the self._name.

Yeah you're right, it's a good idea to make the QEMUMachine name unique.

When I made v2, I looked around to see if we were creating 2
QEMUMachine's concurrently. I didn't see anything, but looking again, I
see this in tests/migration/guestperf/engine.py:

    if self._dst_host != "localhost":
        dstmonaddr = ("localhost", 9001)
    else:
        dstmonaddr = "/var/tmp/qemu-dst-%d-monitor.sock" % os.getpid()
    srcmonaddr = "/var/tmp/qemu-src-%d-monitor.sock" % os.getpid()

    src = QEMUMachine(self._binary,
                      args=self._get_src_args(hardware),
                      wrapper=self._get_src_wrapper(hardware),
                      name="qemu-src-%d" % os.getpid(),
                      monitor_address=srcmonaddr)

    dst = QEMUMachine(self._binary,
                      args=self._get_dst_args(hardware, uri),
                      wrapper=self._get_dst_wrapper(hardware),
                      name="qemu-dst-%d" % os.getpid(),
                      monitor_address=dstmonaddr)

This test case would probably still pass with this change, because the
monitor_address is specified, but I think you're right, it's a good idea
to make the default name different for each QEMUMachine instance to
avoid breaking anything.

> 
> So on reflection I think I should have suggested using:
> 
>     self._name = name or f"{id(self):02x}"

+1, yeah I'll change it to that

> 
> And *in addition*, a few lines later change:
> 
>             self._monitor_address = os.path.join(
>                 self.sock_dir, f"{self._name}-monitor.sock"
>             )
> 
> To
> 
>             self._monitor_address = os.path.join(
>                 self.sock_dir, f"{self._name}.qmp"
>             )

Oh ok, yeah that looks good too. Thanks for the detailed review and
suggestions!

Peter

> 
> 
> > diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
> > index ed4853c805..43b8c8848c 100644
> > --- a/tests/avocado/avocado_qemu/__init__.py
> > +++ b/tests/avocado/avocado_qemu/__init__.py
> > @@ -296,7 +296,7 @@ def require_accelerator(self, accelerator):
> >                          "available" % accelerator)
> >  
> >      def _new_vm(self, name, *args):
> > -        self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
> > +        self._sd = tempfile.TemporaryDirectory(prefix="qemu_")
> 
> This bit is fine.

+1

> 
> >          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)
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

Re: [PATCH v2 1/1] python/machine: Fix AF_UNIX path too long on macOS

[Peter Delevoryas]

On Mon, Jul 18, 2022 at 09:56:17AM +0100, Daniel P. Berrangé wrote:
> On Sat, Jul 16, 2022 at 10:34:34AM -0700, 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
> 
> 
> Looking at this again, I realize my suggestion for dealing with the
> second part of the path was mistaken.
> 
> The "qemu-37331-10bacf110-monitor.sock" part is combining two
> pieces.
> 
> First the result of
> 
>   f"qemu-{os.getpid()}-{id(self):02x}"
> 
> is
> 
>   qemu-37331-10bacf110
> 
> and the code later than appends '-monitor.sock'
> 
> So...
> 
> > 
> > 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 reduces the size of both paths, and removes the unique
> > identification information from the socket name, since it seems to be
> > unnecessary.
> > 
> > This commit produces paths like the following:
> > 
> >     pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T
> >     $ tree qemu*
> >     qemu_oc7h7f3u
> >     ├── qmp-console.sock
> >     └── qmp-monitor.sock
> > 
> > [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         | 2 +-
> >  tests/avocado/avocado_qemu/__init__.py | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > index 37191f433b..b1823966b3 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 "qmp"
> 
> ...my suggestion here was wrong.
> 
> We don't need the os.getpid() unoiqueness because the tmpdir already
> ensures that is safe, but keeping 'id(self)' is a good idea, if the
> test case creates multiple machines concurrently. Bearing in mind we
> later append '-monitor.sock' we don't need 'qmp' in the self._name.
> 
> So on reflection I think I should have suggested using:
> 
>     self._name = name or f"{id(self):02x}"
> 
> And *in addition*, a few lines later change:
> 
>             self._monitor_address = os.path.join(
>                 self.sock_dir, f"{self._name}-monitor.sock"
>             )
> 
> To
> 
>             self._monitor_address = os.path.join(
>                 self.sock_dir, f"{self._name}.qmp"
>             )
>

Finally getting back to this (sorry, been working on other stuff), and I noticed
the console socket is just below this:

    self._console_address = os.path.join(
        self.sock_dir, f"{self._name}-console.sock"
    )

So I probably shouldn't do the "-monitor.sock" change right?

That's ok though, the other changes are sufficient to fix the problem. I'll give
you a chance to reply to this email, or else I'll just send a v3 that doesn't
include the -monitor.sock change.

Thanks,
Peter

> 
> 
> > diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
> > index ed4853c805..43b8c8848c 100644
> > --- a/tests/avocado/avocado_qemu/__init__.py
> > +++ b/tests/avocado/avocado_qemu/__init__.py
> > @@ -296,7 +296,7 @@ def require_accelerator(self, accelerator):
> >                          "available" % accelerator)
> >  
> >      def _new_vm(self, name, *args):
> > -        self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
> > +        self._sd = tempfile.TemporaryDirectory(prefix="qemu_")
> 
> This bit is fine.
> 
> >          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)
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

Re: [PATCH v2 1/1] python/machine: Fix AF_UNIX path too long on macOS

[Daniel P. Berrangé]

On Thu, Jul 21, 2022 at 07:44:21PM -0700, Peter Delevoryas wrote:
> On Mon, Jul 18, 2022 at 09:56:17AM +0100, Daniel P. Berrangé wrote:
> > On Sat, Jul 16, 2022 at 10:34:34AM -0700, 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
> > 
> > 
> > Looking at this again, I realize my suggestion for dealing with the
> > second part of the path was mistaken.
> > 
> > The "qemu-37331-10bacf110-monitor.sock" part is combining two
> > pieces.
> > 
> > First the result of
> > 
> >   f"qemu-{os.getpid()}-{id(self):02x}"
> > 
> > is
> > 
> >   qemu-37331-10bacf110
> > 
> > and the code later than appends '-monitor.sock'
> > 
> > So...
> > 
> > > 
> > > 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 reduces the size of both paths, and removes the unique
> > > identification information from the socket name, since it seems to be
> > > unnecessary.
> > > 
> > > This commit produces paths like the following:
> > > 
> > >     pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T
> > >     $ tree qemu*
> > >     qemu_oc7h7f3u
> > >     ├── qmp-console.sock
> > >     └── qmp-monitor.sock
> > > 
> > > [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         | 2 +-
> > >  tests/avocado/avocado_qemu/__init__.py | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > > index 37191f433b..b1823966b3 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 "qmp"
> > 
> > ...my suggestion here was wrong.
> > 
> > We don't need the os.getpid() unoiqueness because the tmpdir already
> > ensures that is safe, but keeping 'id(self)' is a good idea, if the
> > test case creates multiple machines concurrently. Bearing in mind we
> > later append '-monitor.sock' we don't need 'qmp' in the self._name.
> > 
> > So on reflection I think I should have suggested using:
> > 
> >     self._name = name or f"{id(self):02x}"
> > 
> > And *in addition*, a few lines later change:
> > 
> >             self._monitor_address = os.path.join(
> >                 self.sock_dir, f"{self._name}-monitor.sock"
> >             )
> > 
> > To
> > 
> >             self._monitor_address = os.path.join(
> >                 self.sock_dir, f"{self._name}.qmp"
> >             )
> >
> 
> Finally getting back to this (sorry, been working on other stuff), and I noticed
> the console socket is just below this:
> 
>     self._console_address = os.path.join(
>         self.sock_dir, f"{self._name}-console.sock"
>     )
> 
> So I probably shouldn't do the "-monitor.sock" change right?

I'd suggest changing this one to   f"{self._name}.con" at the
same time.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 1/1] python/machine: Fix AF_UNIX path too long on macOS

[Peter Delevoryas]

On Fri, Jul 22, 2022 at 08:20:11AM +0100, Daniel P. Berrangé wrote:
> On Thu, Jul 21, 2022 at 07:44:21PM -0700, Peter Delevoryas wrote:
> > On Mon, Jul 18, 2022 at 09:56:17AM +0100, Daniel P. Berrangé wrote:
> > > On Sat, Jul 16, 2022 at 10:34:34AM -0700, 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
> > > 
> > > 
> > > Looking at this again, I realize my suggestion for dealing with the
> > > second part of the path was mistaken.
> > > 
> > > The "qemu-37331-10bacf110-monitor.sock" part is combining two
> > > pieces.
> > > 
> > > First the result of
> > > 
> > >   f"qemu-{os.getpid()}-{id(self):02x}"
> > > 
> > > is
> > > 
> > >   qemu-37331-10bacf110
> > > 
> > > and the code later than appends '-monitor.sock'
> > > 
> > > So...
> > > 
> > > > 
> > > > 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 reduces the size of both paths, and removes the unique
> > > > identification information from the socket name, since it seems to be
> > > > unnecessary.
> > > > 
> > > > This commit produces paths like the following:
> > > > 
> > > >     pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T
> > > >     $ tree qemu*
> > > >     qemu_oc7h7f3u
> > > >     ├── qmp-console.sock
> > > >     └── qmp-monitor.sock
> > > > 
> > > > [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         | 2 +-
> > > >  tests/avocado/avocado_qemu/__init__.py | 2 +-
> > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > > > index 37191f433b..b1823966b3 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 "qmp"
> > > 
> > > ...my suggestion here was wrong.
> > > 
> > > We don't need the os.getpid() unoiqueness because the tmpdir already
> > > ensures that is safe, but keeping 'id(self)' is a good idea, if the
> > > test case creates multiple machines concurrently. Bearing in mind we
> > > later append '-monitor.sock' we don't need 'qmp' in the self._name.
> > > 
> > > So on reflection I think I should have suggested using:
> > > 
> > >     self._name = name or f"{id(self):02x}"
> > > 
> > > And *in addition*, a few lines later change:
> > > 
> > >             self._monitor_address = os.path.join(
> > >                 self.sock_dir, f"{self._name}-monitor.sock"
> > >             )
> > > 
> > > To
> > > 
> > >             self._monitor_address = os.path.join(
> > >                 self.sock_dir, f"{self._name}.qmp"
> > >             )
> > >
> > 
> > Finally getting back to this (sorry, been working on other stuff), and I noticed
> > the console socket is just below this:
> > 
> >     self._console_address = os.path.join(
> >         self.sock_dir, f"{self._name}-console.sock"
> >     )
> > 
> > So I probably shouldn't do the "-monitor.sock" change right?
> 
> I'd suggest changing this one to   f"{self._name}.con" at the
> same time.

Ohh ok, yeah that's nice and short. Ok I'll include the socket name suffix
changes then.

> 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>