On Wednesday, July 30, 2025 4:32:22 PM CEST Mark Johnston wrote:
> On Tue, Jul 29, 2025 at 06:09:35PM +0200, Christian Schoenebeck wrote:
> > On Wednesday, July 23, 2025 5:55:58 PM CEST Mark Johnston wrote:
[...]
> Thank you for taking a look.
>
> I'll certainly be around to help deal with issues and patches relating
> to 9pfs+FreeBSD hosts. It's hard to prove that, but for what it's worth
> I use QEMU fairly extensively for FreeBSD development when I can't use
> the native hypervisor, and that's not likely to change anytime soon.
>
> Would adding myself to MAINTAINERS for virtio-9pfs (or a new
> virtio-9pfs-freebsd category) be appropriate in that case?
Good to hear that you will be around!
I leave it to you whether you want to add yourself as reviewer of patches to
MAINTAINER's 9pfs section.
For other things like adding a virtio-9pfs-freebsd subsection or something,
it's probably a bit too early.
> > > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> > > index b9dae8c84c..b85c9934de 100644
> > > --- a/fsdev/file-op-9p.h
> > > +++ b/fsdev/file-op-9p.h
> > > @@ -21,9 +21,11 @@
> > >
> > > #ifdef CONFIG_LINUX
> > > # include <sys/vfs.h>
> > > -#endif
> > > -#ifdef CONFIG_DARWIN
> > > +#elif defined(CONFIG_DARWIN) || defined(CONFIG_FREEBSD)
> > > # include <sys/param.h>
> > > +# ifdef CONFIG_FREEBSD
> > > +# undef MACHINE /* work around some unfortunate namespace pollution */
> > > +# endif
> >
> > Details? :)
>
> We need sys/mount.h for struct statfs. sys/mount.h implicitly includes
> sys/param.h, which is really sloppy about polluting the namespace with
> identifiers that only the kernel cares about 99% of the time. In
> particular, it includes a platform-specific param.h which defines
>
> #define MACHINE "amd64"
> #define MACHINE_ARCH "amd64"
>
> among other things. This conflicts with QEMU's MACHINE macro.
>
> It's a mess. I have some local FreeBSD patches to avoid including
> param.h from sys/mount.h, but of course a number of applications have
> come to rely on the pollution, so they all need to be fixed first.
>
> At some point, I think the block above can become something like
>
> #elif defined(CONFIG_DARWIN) || defined(CONFIG_FREEBSD)
> # ifndef CONFIG_FREEBSD
> # include <sys/param.h>
> # endif
> # include <sys/mount.h>
>
> but for now I need this workaround.
OK.
> > > # include <sys/mount.h>
> > > #endif
> > >
> > > diff --git a/fsdev/meson.build b/fsdev/meson.build
> > > index c751d8cb62..95fe816604 100644
> > > --- a/fsdev/meson.build
> > > +++ b/fsdev/meson.build
> > > @@ -5,6 +5,6 @@ fsdev_ss.add(when: ['CONFIG_FSDEV_9P'], if_true: files(
> > > '9p-marshal.c',
> > > 'qemu-fsdev.c',
> > > ), if_false: files('qemu-fsdev-dummy.c'))
> > > -if host_os in ['linux', 'darwin']
> > > +if host_os in ['linux', 'darwin', 'freebsd']
> > > system_ss.add_all(fsdev_ss)
> > > endif
> > > diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> > > index 9cd1884224..b3743f6169 100644
> > > --- a/hw/9pfs/9p-synth.c
> > > +++ b/hw/9pfs/9p-synth.c
> > > @@ -451,7 +451,7 @@ static int synth_statfs(FsContext *s, V9fsPath
> > > *fs_path,
> > > stbuf->f_bsize = 512;
> > > stbuf->f_blocks = 0;
> > > stbuf->f_files = synth_node_count;
> > > -#ifndef CONFIG_DARWIN
> > > +#if !defined(CONFIG_DARWIN) && !defined(CONFIG_FREEBSD)
> > > stbuf->f_namelen = NAME_MAX;
> > > #endif
> > > return 0;
> > > diff --git a/hw/9pfs/9p-util-freebsd.c b/hw/9pfs/9p-util-freebsd.c
> > > new file mode 100644
> > > index 0000000000..e649f79d4b
> > > --- /dev/null
> > > +++ b/hw/9pfs/9p-util-freebsd.c
> > > @@ -0,0 +1,124 @@
> > > +/*
> > > + * 9p utilities (FreeBSD Implementation)
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > > later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> >
> > I think for new source files in QEMU the policy is to use
> > SPDX-License-Identifier: ... now?
>
> checkpatch.pl does complain about that, yes, but it also qualifies the
> warning with, "unless this file was copied from existing code already
> having such text." I used 9p-util-darwin.c as a starting point for this
> file, so kept the existing license text. I can certainly change it
> though.
Both ways are fine with me in this case. Do as you find appropriate.
> > Is this source file really specific to exactly FreeBSD? From the name it
> > suggests that potential future support for other BSD flavours would need
> > their
> > own source file.
>
> Hmm, not really. Other BSDs seem to implement a compatible syscall
> interface when they implement it at all. (NetBSD's interface seems to
> be compatible; OpenBSD doesn't appear to implement extattrs at all, and
> DragonflyBSD is missing the extattr_*_fd() variants.)
>
> FreeBSD appears to be the only one that implements O_PATH, but that
> seems to be optional if I'm reading correctly.
>
> So, I could preemptively change it to 9p-util-bsd.c, or wait for someone
> to come along and add support for another BSD.
Yeah, that was on nit level as well. It's fine to leave the name, as FreeBSD
is currently the only supported one.
> > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> > > index a1924fe3f0..7315b32591 100644
> > > --- a/hw/9pfs/9p-util.h
> > > +++ b/hw/9pfs/9p-util.h
> > > @@ -21,6 +21,14 @@
> > > #define O_PATH_9P_UTIL 0
> > > #endif
> > >
> > > +#ifdef CONFIG_FREEBSD
> > > +/*
> > > + * FreeBSD does not have these flags, so we can only emulate them
> > > (racily).
> > > + */
> > > +#define XATTR_CREATE 0x1
> > > +#define XATTR_REPLACE 0x2
> > > +#endif
> > > +
> >
> > What do you mean with "racily" here?
>
> FreeBSD cannot atomically check for the existence of and set an extattr,
> the system call interface simply doesn't support it today. This means
> that fsetxattrat_nofollow() needs back-to-back system calls to check for
> the existence of an attribute and then potentially set it.
Ah, I was misinterpreting your comment as if it were "racily defining" the
macros. Maybe change the comment to something like "... can only emulate their
intended behaviour (racily) ...".
Thanks!
/Christian
