On Wed, Jan 11, 2023 at 4:07 AM Daniel P. Berrangé <[email protected]> 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 <[email protected]> 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 <[email protected]> > > > > 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.
