[PATCH v4 0/1] python/machine: Fix AF_UNIX path too long

[PATCH v4 0/1] python/machine: Fix AF_UNIX path too long

[Peter Delevoryas]

v1: https://lore.kernel.org/qemu-devel/20220705214659.73369-1-peter@pjd.dev/
v2: https://lore.kernel.org/qemu-devel/20220716173434.17183-1-peter@pjd.dev/
v3:
    - Changed QEMUMachine._name to f"{id(self):x}". Suggestion was to do
      f"{id(self):02x}", but the id's look like they are probably just the
      object address (8-byte pointer), so the "02" had no effect.
    - Changed QMP socket name suffix from "-monitor.sock" to ".qmp".
    - Changed console socket name suffix from "-console.sock" to ".con".
v4:
	- Just resending v3 after rebasing from a long time ago

I tried to run `make check-avocado` before sending again, but it looks like
there is some other issue. Probably related to the python version I have (I
have like 5 different Python versions installed on my work laptop).

Thanks,
Peter

$ make check-avocado
changing dir to build for /Library/Developer/CommandLineTools/usr/bin/make "check-avocado"...
  GIT     ui/keycodemapdb tests/fp/berkeley-testfloat-3 tests/fp/berkeley-softfloat-3 dtc
  VENV    /Users/pdel/qemu/build/tests/venv
  VENVPIP install -e /Users/pdel/qemu/python/
  VENVPIP install -r /Users/pdel/qemu/tests/requirements.txt
  MKDIR   /Users/pdel/qemu/build/tests/results
  AVOCADO Downloading avocado tests VM image for aarch64
The image was downloaded:
Provider Version Architecture File
fedora   31      aarch64      /Users/pdel/avocado/data/cache/by_location/4f156e531446a679cbfe13caef8b7c9f9f79aafa/Fedora-C
loud-Base-31-1.9.aarch64.qcow2
  AVOCADO tests/avocado
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_aarch64_raspi3_atf
Fetching asset from tests/avocado/boot_xen.py:BootXen.test_arm64_xen_411_and_dom0
Fetching asset from tests/avocado/boot_xen.py:BootXen.test_arm64_xen_414_and_dom0
Fetching asset from tests/avocado/boot_xen.py:BootXen.test_arm64_xen_415_and_dom0
Fetching asset from tests/avocado/machine_aarch64_virt.py:Aarch64VirtMachine.test_alpine_virt_tcg_gic_max
Fetching asset from tests/avocado/machine_aarch64_virt.py:Aarch64VirtMachine.test_aarch64_virt
Fetching asset from tests/avocado/replay_kernel.py:ReplayKernelNormal.test_aarch64_virt
Fetching asset from tests/avocado/reverse_debugging.py:ReverseDebugging_AArch64.test_aarch64_virt
JOB ID     : 18a949ed9150e22d6ecea69b99ede1ded17233f4
JOB LOG    : /Users/pdel/qemu/build/tests/results/job-2023-01-10T00.03-18a949e/job.log

Avocado crashed: TypeError: cannot pickle '_thread.RLock' object

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

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

-- 
2.39.0

[PATCH v4 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 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(-)

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

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

[Philippe Mathieu-Daudé]

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>

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

[Daniel P. Berrangé]

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
-- 
|: 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 v4 1/1] python/machine: Fix AF_UNIX path too long on macOS

[John Snow]

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

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

[Peter Delevoryas]

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

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

[Daniel P. Berrangé]

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
-- 
|: 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 v4 1/1] python/machine: Fix AF_UNIX path too long on macOS

[John Snow]

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.

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

[Daniel P. Berrangé]

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