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