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

[Peter Delevoryas <peter@pjd.dev>]

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