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

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

[Peter Delevoryas]

I noticed that I can't run any avocado tests on macOS because the QMP
unix socket path is too long:

$ ./configure --target-list=arm-softmmu
$ make
$ make check-avocado AVOCADO_TESTS=tests/avocado/boot_linux_console.py
changing dir to build for /Library/Developer/CommandLineTools/usr/bin/make "check-avocado"...
  GIT     ui/keycodemapdb meson tests/fp/berkeley-testfloat-3 tests/fp/berkeley-softfloat-3 dtc slirp
  AVOCADO tests/avocado
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_virt
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_emcraft_sf2
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_emcraft_sf2
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_raspi2_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_raspi2_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_exynos4210_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_exynos4210_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_cubieboard_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_cubieboard_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_cubieboard_sata
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_cubieboard_sata
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_quanta_gsj
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_quanta_gsj_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_quanta_gsj_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_quanta_gsj_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_sd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_sd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic_20_08
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_ast2600_debian
JOB ID     : 84bd26125345c6f12c92d118e98acde6aacfea57
JOB LOG    : /Users/pdel/qemu/build/tests/results/job-2022-07-02T11.24-84bd261/job.log
 (01/16) tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_virt: ERROR: ConnectError: Failed to establish connection: AF_UNIX path too lo
ng\n    Command: \n     Output: None\n (0.11 s)
Interrupting job (failfast).
RESULTS    : PASS 0 | ERROR 1 | FAIL 0 | SKIP 15 | WARN 0 | INTERRUPT 0 | CANCEL 0

The job log shows a backtrace that points to the QMP monitor's unix socket path:

2022-07-02 11:24:05,906 protocol         L0554 DEBUG| Awaiting connection on /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock ...
2022-07-02 11:24:05,907 protocol         L0426 ERROR| Failed to establish connection: OSError: AF_UNIX path too long

I think the path limit for unix sockets on macOS might be 104 [1]

/*
 * [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) */
};

The path we're using is exactly 105 characters:

$ python
Python 2.7.10 (default, Jan 19 2016, 22:24:01)
[GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock')
105

In this change I removed some of the unnecessary elements of the path, to keep
us under the limit. This seemed like the simplest thing to do, and I imagine
the extra human-readable elements were just added to make it easier to identify
left-over sockets or something else related to debugging.

[1]: /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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 37191f433b..93451774e3 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"{os.getpid()}{id(self):02x}"
         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}.sock"
             )
 
         self._console_log_path = console_log
-- 
2.37.0

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

[Daniel P. Berrangé]

On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote:
> I noticed that I can't run any avocado tests on macOS because the QMP
> unix socket path is too long:


> I think the path limit for unix sockets on macOS might be 104 [1]

All platforms have a very limited path limit, so it isn't really
a macOS specific problem, rather....

> 
> /*
>  * [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) */
> };
> 
> The path we're using is exactly 105 characters:
> 
> $ python
> Python 2.7.10 (default, Jan 19 2016, 22:24:01)
> [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin
> Type "help", "copyright", "credits" or "license" for more information.
> >>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock')

It is a problem related to where the test suite is creating the
paths.

/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/

is way too deep a directory location.

It seems we just create this location using 'tempfile.TemporyDirectory'
to get a standard tmp dir.

Do you know why python is choosing

  /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/

as the temp dir ? Is that a standard location on macOS or is it
from some env variable you have set ?

> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> index 37191f433b..93451774e3 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"{os.getpid()}{id(self):02x}"

I don't think this is the right fix really, because IMHO the problem
is the hugely long path, rather than the final socket name.

That said, there is redundancy in the path - avocado is passing in
a dierctory created using 'tempfile.TemporyDirectory' so there is no
reason why we need to add more entropy via the POD and the 'id(self)'
hex string.

IMHO avocado should pass in the 'name' parameter explicitly, using a
plain name and thus get a shorter string.

>          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}.sock"
>              )
>  
>          self._console_log_path = console_log
> -- 
> 2.37.0
> 
> 

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

[Peter Delevoryas]

On Wed, Jul 06, 2022 at 09:02:14AM +0100, Daniel P. Berrangé wrote:
> On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote:
> > I noticed that I can't run any avocado tests on macOS because the QMP
> > unix socket path is too long:
> 
> 
> > I think the path limit for unix sockets on macOS might be 104 [1]
> 
> All platforms have a very limited path limit, so it isn't really
> a macOS specific problem, rather....
> 
> > 
> > /*
> >  * [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) */
> > };
> > 
> > The path we're using is exactly 105 characters:
> > 
> > $ python
> > Python 2.7.10 (default, Jan 19 2016, 22:24:01)
> > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin
> > Type "help", "copyright", "credits" or "license" for more information.
> > >>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock')
> 
> It is a problem related to where the test suite is creating the
> paths.
> 
> /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/
> 
> is way too deep a directory location.

That's a good point.

> 
> It seems we just create this location using 'tempfile.TemporyDirectory'
> to get a standard tmp dir.
> 
> Do you know why python is choosing
> 
>   /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/
> 
> as the temp dir ? Is that a standard location on macOS or is it
> from some env variable you have set ?

Hmmm yeah it is odd, I'm not really sure why it's created there or if
standard glibc tmpfile creation goes there too, I'll go experiment and
report back. And yeah, maybe I'll double check any environment variables or
other things.

The macOS system I use happens to be a Facebook work laptop, which could
also be related now that I think about it.

> 
> > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > index 37191f433b..93451774e3 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"{os.getpid()}{id(self):02x}"
> 
> I don't think this is the right fix really, because IMHO the problem
> is the hugely long path, rather than the final socket name.

True, yeah let me try to investigate the directory placement.

> 
> That said, there is redundancy in the path - avocado is passing in
> a dierctory created using 'tempfile.TemporyDirectory' so there is no
> reason why we need to add more entropy via the POD and the 'id(self)'
> hex string.

Oh good point, I hadn't thought about that.

> 
> IMHO avocado should pass in the 'name' parameter explicitly, using a
> plain name and thus get a shorter string.

I see, yeah that makes sense. Maybe I can include a couple patches for this,
one fixing the directory location, and one refactoring the temporary file
name template (If I'm understanding your suggestion correctly).

Thanks for the review! I really appreciate it.
Peter

> 
> >          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}.sock"
> >              )
> >  
> >          self._console_log_path = console_log
> > -- 
> > 2.37.0
> > 
> > 
> 
> 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 RESEND] python/machine: Fix AF_UNIX path too long on macOS

[Peter Delevoryas]

On Wed, Jul 06, 2022 at 09:46:48AM -0700, Peter Delevoryas wrote:
> On Wed, Jul 06, 2022 at 09:02:14AM +0100, Daniel P. Berrangé wrote:
> > On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote:
> > > I noticed that I can't run any avocado tests on macOS because the QMP
> > > unix socket path is too long:
> > 
> > 
> > > I think the path limit for unix sockets on macOS might be 104 [1]
> > 
> > All platforms have a very limited path limit, so it isn't really
> > a macOS specific problem, rather....
> > 
> > > 
> > > /*
> > >  * [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) */
> > > };
> > > 
> > > The path we're using is exactly 105 characters:
> > > 
> > > $ python
> > > Python 2.7.10 (default, Jan 19 2016, 22:24:01)
> > > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin
> > > Type "help", "copyright", "credits" or "license" for more information.
> > > >>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock')
> > 
> > It is a problem related to where the test suite is creating the
> > paths.
> > 
> > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/
> > 
> > is way too deep a directory location.
> 
> That's a good point.
> 
> > 
> > It seems we just create this location using 'tempfile.TemporyDirectory'
> > to get a standard tmp dir.
> > 
> > Do you know why python is choosing
> > 
> >   /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/
> > 
> > as the temp dir ? Is that a standard location on macOS or is it
> > from some env variable you have set ?
> 
> Hmmm yeah it is odd, I'm not really sure why it's created there or if
> standard glibc tmpfile creation goes there too, I'll go experiment and
> report back. And yeah, maybe I'll double check any environment variables or
> other things.
> 
> The macOS system I use happens to be a Facebook work laptop, which could
> also be related now that I think about it.

Hmmm yeah looks like this is because my TMPDIR is weird.

$ echo $TMPDIR
/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/

I didn't think to check this cause I wasn't familiar with TMPDIR. 🤷

Thanks for responding, I'll just use TMPDIR=/tmp for now. It's probably
something wrong with the Facebook development environment.

Peter

> 
> > 
> > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > > index 37191f433b..93451774e3 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"{os.getpid()}{id(self):02x}"
> > 
> > I don't think this is the right fix really, because IMHO the problem
> > is the hugely long path, rather than the final socket name.
> 
> True, yeah let me try to investigate the directory placement.
> 
> > 
> > That said, there is redundancy in the path - avocado is passing in
> > a dierctory created using 'tempfile.TemporyDirectory' so there is no
> > reason why we need to add more entropy via the POD and the 'id(self)'
> > hex string.
> 
> Oh good point, I hadn't thought about that.
> 
> > 
> > IMHO avocado should pass in the 'name' parameter explicitly, using a
> > plain name and thus get a shorter string.
> 
> I see, yeah that makes sense. Maybe I can include a couple patches for this,
> one fixing the directory location, and one refactoring the temporary file
> name template (If I'm understanding your suggestion correctly).
> 
> Thanks for the review! I really appreciate it.
> Peter
> 
> > 
> > >          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}.sock"
> > >              )
> > >  
> > >          self._console_log_path = console_log
> > > -- 
> > > 2.37.0
> > > 
> > > 
> > 
> > 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 RESEND] python/machine: Fix AF_UNIX path too long on macOS

[Peter Delevoryas]

On Wed, Jul 06, 2022 at 05:52:48PM -0700, Peter Delevoryas wrote:
> On Wed, Jul 06, 2022 at 09:46:48AM -0700, Peter Delevoryas wrote:
> > On Wed, Jul 06, 2022 at 09:02:14AM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote:
> > > > I noticed that I can't run any avocado tests on macOS because the QMP
> > > > unix socket path is too long:
> > > 
> > > 
> > > > I think the path limit for unix sockets on macOS might be 104 [1]
> > > 
> > > All platforms have a very limited path limit, so it isn't really
> > > a macOS specific problem, rather....
> > > 
> > > > 
> > > > /*
> > > >  * [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) */
> > > > };
> > > > 
> > > > The path we're using is exactly 105 characters:
> > > > 
> > > > $ python
> > > > Python 2.7.10 (default, Jan 19 2016, 22:24:01)
> > > > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin
> > > > Type "help", "copyright", "credits" or "license" for more information.
> > > > >>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock')
> > > 
> > > It is a problem related to where the test suite is creating the
> > > paths.
> > > 
> > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/
> > > 
> > > is way too deep a directory location.
> > 
> > That's a good point.
> > 
> > > 
> > > It seems we just create this location using 'tempfile.TemporyDirectory'
> > > to get a standard tmp dir.
> > > 
> > > Do you know why python is choosing
> > > 
> > >   /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/
> > > 
> > > as the temp dir ? Is that a standard location on macOS or is it
> > > from some env variable you have set ?
> > 
> > Hmmm yeah it is odd, I'm not really sure why it's created there or if
> > standard glibc tmpfile creation goes there too, I'll go experiment and
> > report back. And yeah, maybe I'll double check any environment variables or
> > other things.
> > 
> > The macOS system I use happens to be a Facebook work laptop, which could
> > also be related now that I think about it.
> 
> Hmmm yeah looks like this is because my TMPDIR is weird.
> 
> $ echo $TMPDIR
> /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/
> 
> I didn't think to check this cause I wasn't familiar with TMPDIR. 🤷
> 
> Thanks for responding, I'll just use TMPDIR=/tmp for now. It's probably
> something wrong with the Facebook development environment.
> 
> Peter

Update: Actually, this might not be a Facebook-work-laptop specific
thing.  I asked my non-engineer friend to print out $TMPDIR on his
macbook and he got the same thing.

https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path

I guess this person suggests it's just to separate the permissions for
each user's /tmp directory, for better isolation.

I'll resubmit this patch with the suggestions you had, because perhaps
this is actually affecting other macOS users too.

> 
> > 
> > > 
> > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > > > index 37191f433b..93451774e3 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"{os.getpid()}{id(self):02x}"
> > > 
> > > I don't think this is the right fix really, because IMHO the problem
> > > is the hugely long path, rather than the final socket name.
> > 
> > True, yeah let me try to investigate the directory placement.
> > 
> > > 
> > > That said, there is redundancy in the path - avocado is passing in
> > > a dierctory created using 'tempfile.TemporyDirectory' so there is no
> > > reason why we need to add more entropy via the POD and the 'id(self)'
> > > hex string.
> > 
> > Oh good point, I hadn't thought about that.
> > 
> > > 
> > > IMHO avocado should pass in the 'name' parameter explicitly, using a
> > > plain name and thus get a shorter string.
> > 
> > I see, yeah that makes sense. Maybe I can include a couple patches for this,
> > one fixing the directory location, and one refactoring the temporary file
> > name template (If I'm understanding your suggestion correctly).
> > 
> > Thanks for the review! I really appreciate it.
> > Peter
> > 
> > > 
> > > >          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}.sock"
> > > >              )
> > > >  
> > > >          self._console_log_path = console_log
> > > > -- 
> > > > 2.37.0
> > > > 
> > > > 
> > > 
> > > 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 RESEND] python/machine: Fix AF_UNIX path too long on macOS

[John Snow]

On Thu, Jul 7, 2022 at 2:46 PM Peter Delevoryas <peter@pjd.dev> wrote:
>
> On Wed, Jul 06, 2022 at 05:52:48PM -0700, Peter Delevoryas wrote:
> > On Wed, Jul 06, 2022 at 09:46:48AM -0700, Peter Delevoryas wrote:
> > > On Wed, Jul 06, 2022 at 09:02:14AM +0100, Daniel P. Berrangé wrote:
> > > > On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote:
> > > > > I noticed that I can't run any avocado tests on macOS because the QMP
> > > > > unix socket path is too long:
> > > >
> > > >
> > > > > I think the path limit for unix sockets on macOS might be 104 [1]
> > > >
> > > > All platforms have a very limited path limit, so it isn't really
> > > > a macOS specific problem, rather....
> > > >
> > > > >
> > > > > /*
> > > > >  * [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) */
> > > > > };
> > > > >
> > > > > The path we're using is exactly 105 characters:
> > > > >
> > > > > $ python
> > > > > Python 2.7.10 (default, Jan 19 2016, 22:24:01)
> > > > > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin
> > > > > Type "help", "copyright", "credits" or "license" for more information.
> > > > > >>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock')
> > > >
> > > > It is a problem related to where the test suite is creating the
> > > > paths.
> > > >
> > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/
> > > >
> > > > is way too deep a directory location.
> > >
> > > That's a good point.
> > >
> > > >
> > > > It seems we just create this location using 'tempfile.TemporyDirectory'
> > > > to get a standard tmp dir.
> > > >
> > > > Do you know why python is choosing
> > > >
> > > >   /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/
> > > >
> > > > as the temp dir ? Is that a standard location on macOS or is it
> > > > from some env variable you have set ?
> > >
> > > Hmmm yeah it is odd, I'm not really sure why it's created there or if
> > > standard glibc tmpfile creation goes there too, I'll go experiment and
> > > report back. And yeah, maybe I'll double check any environment variables or
> > > other things.
> > >
> > > The macOS system I use happens to be a Facebook work laptop, which could
> > > also be related now that I think about it.
> >
> > Hmmm yeah looks like this is because my TMPDIR is weird.
> >
> > $ echo $TMPDIR
> > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/
> >
> > I didn't think to check this cause I wasn't familiar with TMPDIR. 🤷
> >
> > Thanks for responding, I'll just use TMPDIR=/tmp for now. It's probably
> > something wrong with the Facebook development environment.
> >
> > Peter
>
> Update: Actually, this might not be a Facebook-work-laptop specific
> thing.  I asked my non-engineer friend to print out $TMPDIR on his
> macbook and he got the same thing.
>
> https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path
>
> I guess this person suggests it's just to separate the permissions for
> each user's /tmp directory, for better isolation.
>
> I'll resubmit this patch with the suggestions you had, because perhaps
> this is actually affecting other macOS users too.
>
> >
> > >
> > > >
> > > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > > > > index 37191f433b..93451774e3 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"{os.getpid()}{id(self):02x}"
> > > >
> > > > I don't think this is the right fix really, because IMHO the problem
> > > > is the hugely long path, rather than the final socket name.
> > >
> > > True, yeah let me try to investigate the directory placement.
> > >
> > > >
> > > > That said, there is redundancy in the path - avocado is passing in
> > > > a dierctory created using 'tempfile.TemporyDirectory' so there is no
> > > > reason why we need to add more entropy via the POD and the 'id(self)'
> > > > hex string.
> > >
> > > Oh good point, I hadn't thought about that.
> > >
> > > >
> > > > IMHO avocado should pass in the 'name' parameter explicitly, using a
> > > > plain name and thus get a shorter string.
> > >
> > > I see, yeah that makes sense. Maybe I can include a couple patches for this,
> > > one fixing the directory location, and one refactoring the temporary file
> > > name template (If I'm understanding your suggestion correctly).
> > >
> > > Thanks for the review! I really appreciate it.
> > > Peter

I agree with Dan: I believe the correct solution here is for Avocado
to provide its own less redundant name; but the default name that
machine.py provides is not *that* long and provides adequate
protection against collisions with multiple instances of the VM
utility within a single python process. If Avocado is creating its own
directories that guard against that redundancy, Avocado should provide
a shortened name for the VM.

Note that the QEMUMachine process also provides a sock_dir parameter
that was introduced for precisely this reason; it should be possible
to instruct the avocado tests to use a shorter path for sock_dir.

I'm not clear on what the best "just works" solution will be when
certain operating environments choose a tmp dir that's quite long to
begin with; maybe we need a different default sockfile naming strategy
that avoids the instance collision problem in machine.py, too. Ideas?

--js

> > >
> > > >
> > > > >          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}.sock"
> > > > >              )
> > > > >
> > > > >          self._console_log_path = console_log
> > > > > --
> > > > > 2.37.0
> > > > >
> > > > >
> > > >
> > > > 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 RESEND] python/machine: Fix AF_UNIX path too long on macOS

[Peter Delevoryas]

On Mon, Jul 11, 2022 at 04:56:28PM -0400, John Snow wrote:
> On Thu, Jul 7, 2022 at 2:46 PM Peter Delevoryas <peter@pjd.dev> wrote:
> >
> > On Wed, Jul 06, 2022 at 05:52:48PM -0700, Peter Delevoryas wrote:
> > > On Wed, Jul 06, 2022 at 09:46:48AM -0700, Peter Delevoryas wrote:
> > > > On Wed, Jul 06, 2022 at 09:02:14AM +0100, Daniel P. Berrangé wrote:
> > > > > On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote:
> > > > > > I noticed that I can't run any avocado tests on macOS because the QMP
> > > > > > unix socket path is too long:
> > > > >
> > > > >
> > > > > > I think the path limit for unix sockets on macOS might be 104 [1]
> > > > >
> > > > > All platforms have a very limited path limit, so it isn't really
> > > > > a macOS specific problem, rather....
> > > > >
> > > > > >
> > > > > > /*
> > > > > >  * [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) */
> > > > > > };
> > > > > >
> > > > > > The path we're using is exactly 105 characters:
> > > > > >
> > > > > > $ python
> > > > > > Python 2.7.10 (default, Jan 19 2016, 22:24:01)
> > > > > > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin
> > > > > > Type "help", "copyright", "credits" or "license" for more information.
> > > > > > >>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock')
> > > > >
> > > > > It is a problem related to where the test suite is creating the
> > > > > paths.
> > > > >
> > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/
> > > > >
> > > > > is way too deep a directory location.
> > > >
> > > > That's a good point.
> > > >
> > > > >
> > > > > It seems we just create this location using 'tempfile.TemporyDirectory'
> > > > > to get a standard tmp dir.
> > > > >
> > > > > Do you know why python is choosing
> > > > >
> > > > >   /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/
> > > > >
> > > > > as the temp dir ? Is that a standard location on macOS or is it
> > > > > from some env variable you have set ?
> > > >
> > > > Hmmm yeah it is odd, I'm not really sure why it's created there or if
> > > > standard glibc tmpfile creation goes there too, I'll go experiment and
> > > > report back. And yeah, maybe I'll double check any environment variables or
> > > > other things.
> > > >
> > > > The macOS system I use happens to be a Facebook work laptop, which could
> > > > also be related now that I think about it.
> > >
> > > Hmmm yeah looks like this is because my TMPDIR is weird.
> > >
> > > $ echo $TMPDIR
> > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/
> > >
> > > I didn't think to check this cause I wasn't familiar with TMPDIR. 🤷
> > >
> > > Thanks for responding, I'll just use TMPDIR=/tmp for now. It's probably
> > > something wrong with the Facebook development environment.
> > >
> > > Peter
> >
> > Update: Actually, this might not be a Facebook-work-laptop specific
> > thing.  I asked my non-engineer friend to print out $TMPDIR on his
> > macbook and he got the same thing.
> >
> > https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path
> >
> > I guess this person suggests it's just to separate the permissions for
> > each user's /tmp directory, for better isolation.
> >
> > I'll resubmit this patch with the suggestions you had, because perhaps
> > this is actually affecting other macOS users too.
> >
> > >
> > > >
> > > > >
> > > > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > > > > > index 37191f433b..93451774e3 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"{os.getpid()}{id(self):02x}"
> > > > >
> > > > > I don't think this is the right fix really, because IMHO the problem
> > > > > is the hugely long path, rather than the final socket name.
> > > >
> > > > True, yeah let me try to investigate the directory placement.
> > > >
> > > > >
> > > > > That said, there is redundancy in the path - avocado is passing in
> > > > > a dierctory created using 'tempfile.TemporyDirectory' so there is no
> > > > > reason why we need to add more entropy via the POD and the 'id(self)'
> > > > > hex string.
> > > >
> > > > Oh good point, I hadn't thought about that.
> > > >
> > > > >
> > > > > IMHO avocado should pass in the 'name' parameter explicitly, using a
> > > > > plain name and thus get a shorter string.
> > > >
> > > > I see, yeah that makes sense. Maybe I can include a couple patches for this,
> > > > one fixing the directory location, and one refactoring the temporary file
> > > > name template (If I'm understanding your suggestion correctly).
> > > >
> > > > Thanks for the review! I really appreciate it.
> > > > Peter
> 
> I agree with Dan: I believe the correct solution here is for Avocado
> to provide its own less redundant name; but the default name that
> machine.py provides is not *that* long and provides adequate
> protection against collisions with multiple instances of the VM
> utility within a single python process. If Avocado is creating its own
> directories that guard against that redundancy, Avocado should provide
> a shortened name for the VM.

Hmmm, I see.

> 
> Note that the QEMUMachine process also provides a sock_dir parameter
> that was introduced for precisely this reason; it should be possible
> to instruct the avocado tests to use a shorter path for sock_dir.
> 
> I'm not clear on what the best "just works" solution will be when
> certain operating environments choose a tmp dir that's quite long to
> begin with; maybe we need a different default sockfile naming strategy
> that avoids the instance collision problem in machine.py, too. Ideas?

I guess I don't really understand why QEMU is attempting to create a
unique path instead of just using mktemp(), right? This seems like
something that the OS can provide. If we want some semblance of meaning
to the filename, we can provide a short template, right?

-- Peter

> 
> --js
> 
> > > >
> > > > >
> > > > > >          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}.sock"
> > > > > >              )
> > > > > >
> > > > > >          self._console_log_path = console_log
> > > > > > --
> > > > > > 2.37.0
> > > > > >
> > > > > >
> > > > >
> > > > > 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 RESEND] python/machine: Fix AF_UNIX path too long on macOS

[Daniel P. Berrangé]

On Mon, Jul 11, 2022 at 04:56:28PM -0400, John Snow wrote:
> On Thu, Jul 7, 2022 at 2:46 PM Peter Delevoryas <peter@pjd.dev> wrote:
> >
> > On Wed, Jul 06, 2022 at 05:52:48PM -0700, Peter Delevoryas wrote:
> > > On Wed, Jul 06, 2022 at 09:46:48AM -0700, Peter Delevoryas wrote:
> > > > On Wed, Jul 06, 2022 at 09:02:14AM +0100, Daniel P. Berrangé wrote:
> > > > > On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote:
> > > > > > I noticed that I can't run any avocado tests on macOS because the QMP
> > > > > > unix socket path is too long:
> > > > >
> > > > >
> > > > > > I think the path limit for unix sockets on macOS might be 104 [1]
> > > > >
> > > > > All platforms have a very limited path limit, so it isn't really
> > > > > a macOS specific problem, rather....
> > > > >
> > > > > >
> > > > > > /*
> > > > > >  * [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) */
> > > > > > };
> > > > > >
> > > > > > The path we're using is exactly 105 characters:
> > > > > >
> > > > > > $ python
> > > > > > Python 2.7.10 (default, Jan 19 2016, 22:24:01)
> > > > > > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin
> > > > > > Type "help", "copyright", "credits" or "license" for more information.
> > > > > > >>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock')
> > > > >
> > > > > It is a problem related to where the test suite is creating the
> > > > > paths.
> > > > >
> > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/
> > > > >
> > > > > is way too deep a directory location.
> > > >
> > > > That's a good point.
> > > >
> > > > >
> > > > > It seems we just create this location using 'tempfile.TemporyDirectory'
> > > > > to get a standard tmp dir.
> > > > >
> > > > > Do you know why python is choosing
> > > > >
> > > > >   /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/
> > > > >
> > > > > as the temp dir ? Is that a standard location on macOS or is it
> > > > > from some env variable you have set ?
> > > >
> > > > Hmmm yeah it is odd, I'm not really sure why it's created there or if
> > > > standard glibc tmpfile creation goes there too, I'll go experiment and
> > > > report back. And yeah, maybe I'll double check any environment variables or
> > > > other things.
> > > >
> > > > The macOS system I use happens to be a Facebook work laptop, which could
> > > > also be related now that I think about it.
> > >
> > > Hmmm yeah looks like this is because my TMPDIR is weird.
> > >
> > > $ echo $TMPDIR
> > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/
> > >
> > > I didn't think to check this cause I wasn't familiar with TMPDIR. 🤷
> > >
> > > Thanks for responding, I'll just use TMPDIR=/tmp for now. It's probably
> > > something wrong with the Facebook development environment.
> > >
> > > Peter
> >
> > Update: Actually, this might not be a Facebook-work-laptop specific
> > thing.  I asked my non-engineer friend to print out $TMPDIR on his
> > macbook and he got the same thing.
> >
> > https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path
> >
> > I guess this person suggests it's just to separate the permissions for
> > each user's /tmp directory, for better isolation.
> >
> > I'll resubmit this patch with the suggestions you had, because perhaps
> > this is actually affecting other macOS users too.
> >
> > >
> > > >
> > > > >
> > > > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > > > > > index 37191f433b..93451774e3 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"{os.getpid()}{id(self):02x}"
> > > > >
> > > > > I don't think this is the right fix really, because IMHO the problem
> > > > > is the hugely long path, rather than the final socket name.
> > > >
> > > > True, yeah let me try to investigate the directory placement.
> > > >
> > > > >
> > > > > That said, there is redundancy in the path - avocado is passing in
> > > > > a dierctory created using 'tempfile.TemporyDirectory' so there is no
> > > > > reason why we need to add more entropy via the POD and the 'id(self)'
> > > > > hex string.
> > > >
> > > > Oh good point, I hadn't thought about that.
> > > >
> > > > >
> > > > > IMHO avocado should pass in the 'name' parameter explicitly, using a
> > > > > plain name and thus get a shorter string.
> > > >
> > > > I see, yeah that makes sense. Maybe I can include a couple patches for this,
> > > > one fixing the directory location, and one refactoring the temporary file
> > > > name template (If I'm understanding your suggestion correctly).
> > > >
> > > > Thanks for the review! I really appreciate it.
> > > > Peter
> 
> I agree with Dan: I believe the correct solution here is for Avocado
> to provide its own less redundant name; but the default name that
> machine.py provides is not *that* long and provides adequate
> protection against collisions with multiple instances of the VM
> utility within a single python process. If Avocado is creating its own
> directories that guard against that redundancy, Avocado should provide
> a shortened name for the VM.
> 
> Note that the QEMUMachine process also provides a sock_dir parameter
> that was introduced for precisely this reason; it should be possible
> to instruct the avocado tests to use a shorter path for sock_dir.
> 
> I'm not clear on what the best "just works" solution will be when
> certain operating environments choose a tmp dir that's quite long to
> begin with; maybe we need a different default sockfile naming strategy
> that avoids the instance collision problem in machine.py, too. Ideas?

Ultimately there's no solution that is guaranteed to work. If $HOME
or $TMPDIR point to paths that are >= 108 chars long, we're doomed
no matter what. To maximise the liklihood of working though, we need
to be minimal in paths we append. 

Right now, Peter's TMPDIR is

/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T

Onto which QEMU appends

  /avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock

IOW, QEMU is more than 1/2 of the problem here.

QEMU could cut down the size of the temporary directory using a more
concise pattern, changing

  tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")

To

  tempfile.TemporaryDirectory(prefix="qemu_")


And then use a fixed socket name 'qmp.sock', giving us:

  /qemu_uh3w_dgc/qmp.sock

to append to TMPDIR

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

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 8812 bytes --]

On Mon, Jul 11, 2022, 9:46 PM Peter Delevoryas <peter@pjd.dev> wrote:

> On Mon, Jul 11, 2022 at 04:56:28PM -0400, John Snow wrote:
> > On Thu, Jul 7, 2022 at 2:46 PM Peter Delevoryas <peter@pjd.dev> wrote:
> > >
> > > On Wed, Jul 06, 2022 at 05:52:48PM -0700, Peter Delevoryas wrote:
> > > > On Wed, Jul 06, 2022 at 09:46:48AM -0700, Peter Delevoryas wrote:
> > > > > On Wed, Jul 06, 2022 at 09:02:14AM +0100, Daniel P. Berrangé wrote:
> > > > > > On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote:
> > > > > > > I noticed that I can't run any avocado tests on macOS because
> the QMP
> > > > > > > unix socket path is too long:
> > > > > >
> > > > > >
> > > > > > > I think the path limit for unix sockets on macOS might be 104
> [1]
> > > > > >
> > > > > > All platforms have a very limited path limit, so it isn't really
> > > > > > a macOS specific problem, rather....
> > > > > >
> > > > > > >
> > > > > > > /*
> > > > > > >  * [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) */
> > > > > > > };
> > > > > > >
> > > > > > > The path we're using is exactly 105 characters:
> > > > > > >
> > > > > > > $ python
> > > > > > > Python 2.7.10 (default, Jan 19 2016, 22:24:01)
> > > > > > > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on
> darwin
> > > > > > > Type "help", "copyright", "credits" or "license" for more
> information.
> > > > > > > >>>
> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock')
> > > > > >
> > > > > > It is a problem related to where the test suite is creating the
> > > > > > paths.
> > > > > >
> > > > > >
> /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/
> > > > > >
> > > > > > is way too deep a directory location.
> > > > >
> > > > > That's a good point.
> > > > >
> > > > > >
> > > > > > It seems we just create this location using
> 'tempfile.TemporyDirectory'
> > > > > > to get a standard tmp dir.
> > > > > >
> > > > > > Do you know why python is choosing
> > > > > >
> > > > > >   /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/
> > > > > >
> > > > > > as the temp dir ? Is that a standard location on macOS or is it
> > > > > > from some env variable you have set ?
> > > > >
> > > > > Hmmm yeah it is odd, I'm not really sure why it's created there or
> if
> > > > > standard glibc tmpfile creation goes there too, I'll go experiment
> and
> > > > > report back. And yeah, maybe I'll double check any environment
> variables or
> > > > > other things.
> > > > >
> > > > > The macOS system I use happens to be a Facebook work laptop, which
> could
> > > > > also be related now that I think about it.
> > > >
> > > > Hmmm yeah looks like this is because my TMPDIR is weird.
> > > >
> > > > $ echo $TMPDIR
> > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/
> > > >
> > > > I didn't think to check this cause I wasn't familiar with TMPDIR. 🤷
> > > >
> > > > Thanks for responding, I'll just use TMPDIR=/tmp for now. It's
> probably
> > > > something wrong with the Facebook development environment.
> > > >
> > > > Peter
> > >
> > > Update: Actually, this might not be a Facebook-work-laptop specific
> > > thing.  I asked my non-engineer friend to print out $TMPDIR on his
> > > macbook and he got the same thing.
> > >
> > >
> https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path
> > >
> > > I guess this person suggests it's just to separate the permissions for
> > > each user's /tmp directory, for better isolation.
> > >
> > > I'll resubmit this patch with the suggestions you had, because perhaps
> > > this is actually affecting other macOS users too.
> > >
> > > >
> > > > >
> > > > > >
> > > > > > > diff --git a/python/qemu/machine/machine.py
> b/python/qemu/machine/machine.py
> > > > > > > index 37191f433b..93451774e3 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"{os.getpid()}{id(self):02x}"
> > > > > >
> > > > > > I don't think this is the right fix really, because IMHO the
> problem
> > > > > > is the hugely long path, rather than the final socket name.
> > > > >
> > > > > True, yeah let me try to investigate the directory placement.
> > > > >
> > > > > >
> > > > > > That said, there is redundancy in the path - avocado is passing
> in
> > > > > > a dierctory created using 'tempfile.TemporyDirectory' so there
> is no
> > > > > > reason why we need to add more entropy via the POD and the
> 'id(self)'
> > > > > > hex string.
> > > > >
> > > > > Oh good point, I hadn't thought about that.
> > > > >
> > > > > >
> > > > > > IMHO avocado should pass in the 'name' parameter explicitly,
> using a
> > > > > > plain name and thus get a shorter string.
> > > > >
> > > > > I see, yeah that makes sense. Maybe I can include a couple patches
> for this,
> > > > > one fixing the directory location, and one refactoring the
> temporary file
> > > > > name template (If I'm understanding your suggestion correctly).
> > > > >
> > > > > Thanks for the review! I really appreciate it.
> > > > > Peter
> >
> > I agree with Dan: I believe the correct solution here is for Avocado
> > to provide its own less redundant name; but the default name that
> > machine.py provides is not *that* long and provides adequate
> > protection against collisions with multiple instances of the VM
> > utility within a single python process. If Avocado is creating its own
> > directories that guard against that redundancy, Avocado should provide
> > a shortened name for the VM.
>
> Hmmm, I see.
>
> >
> > Note that the QEMUMachine process also provides a sock_dir parameter
> > that was introduced for precisely this reason; it should be possible
> > to instruct the avocado tests to use a shorter path for sock_dir.
> >
> > I'm not clear on what the best "just works" solution will be when
> > certain operating environments choose a tmp dir that's quite long to
> > begin with; maybe we need a different default sockfile naming strategy
> > that avoids the instance collision problem in machine.py, too. Ideas?
>
> I guess I don't really understand why QEMU is attempting to create a
> unique path instead of just using mktemp(), right? This seems like
> something that the OS can provide. If we want some semblance of meaning
> to the filename, we can provide a short template, right?
>

Yeah, should be good. The "name" of the VM should be left unique for
logging purposes, so the name should remain as-is, IMO.

However, if you want to change how we create the sockfile by using a new,
temporary name (and not the vm name), that works for me as far as I can
remember.

I think there was some desire to have the temp file obviously correlate to
the instance, but that's not a hard requirement, so feel free to change
that part.

Just make sure that come hell or high water the sockfile is cleaned up on
exit.



> -- Peter
>
>
> >
> > --js
> >
> > > > >
> > > > > >
> > > > > > >          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}.sock"
> > > > > > >              )
> > > > > > >
> > > > > > >          self._console_log_path = console_log
> > > > > > > --
> > > > > > > 2.37.0
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > 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 :|
> > > > > >
> > > > >
> > > >
> > >
> >
>
>

[-- Attachment #2: Type: text/html, Size: 13039 bytes --]