On Montag, 7. Februar 2022 15:37:00 CET Will Cohen wrote:
> On Mon, Feb 7, 2022 at 9:27 AM Christian Schoenebeck
> <[email protected]>
> wrote:
> > On Sonntag, 6. Februar 2022 21:07:18 CET Will Cohen wrote:
> > > From: Keno Fischer <[email protected]>
> > >
> > > Signed-off-by: Keno Fischer <[email protected]>
> > > [Michael Roitzsch: - Rebase for NixOS]
> > > Signed-off-by: Michael Roitzsch <[email protected]>
> > > [Will Cohen: - Rebase to master]
> > > Signed-off-by: Will Cohen <[email protected]>
> > > Reviewed-by: Paolo Bonzini <[email protected]>
> > > [Will Cohen: - Add check for pthread_fchdir_np to virtfs]
> > > Signed-off-by: Will Cohen <[email protected]>
> > > ---
> > >
> > > fsdev/meson.build | 1 +
> > > meson.build | 14 ++++++++++----
> > > 2 files changed, 11 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fsdev/meson.build b/fsdev/meson.build
> > > index adf57cc43e..b632b66348 100644
> > > --- a/fsdev/meson.build
> > > +++ b/fsdev/meson.build
> > > @@ -7,6 +7,7 @@ fsdev_ss.add(when: ['CONFIG_FSDEV_9P'], if_true: files(
> > >
> > > 'qemu-fsdev.c',
> > >
> > > ), if_false: files('qemu-fsdev-dummy.c'))
> > > softmmu_ss.add_all(when: 'CONFIG_LINUX', if_true: fsdev_ss)
> > >
> > > +softmmu_ss.add_all(when: 'CONFIG_DARWIN', if_true: fsdev_ss)
> > >
> > > if have_virtfs_proxy_helper
> > >
> > > executable('virtfs-proxy-helper',
> > >
> > > diff --git a/meson.build b/meson.build
> > > index 5f43355071..6b4adf7e15 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -1421,17 +1421,23 @@ if not get_option('dbus_display').disabled()
> > >
> > > endif
> > >
> > > endif
> > >
> > > -have_virtfs = (targetos == 'linux' and
> > > +if targetos == 'darwin' and cc.has_function('pthread_fchdir_np')
> > > + have_virtfs = have_system
> >
> > As you are going for a v5 anyway: I would add an error message here if
> > pthread_fchdir_np() is not available. Because it is a bit frustrating for
> > users if their options silently got ignored without any indication why.
> >
> > > +else
> > > + have_virtfs = (targetos == 'linux' and
> > >
> > > have_system and
> > > libattr.found() and
> > > libcap_ng.found())
> > >
> > > +endif
> > >
> > > -have_virtfs_proxy_helper = have_virtfs and have_tools
> > > +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and
> > > have_tools
> > >
> > > if get_option('virtfs').enabled()
> > >
> > > if not have_virtfs
> > >
> > > - if targetos != 'linux'
> > > - error('virtio-9p (virtfs) requires Linux')
> > > + if targetos != 'linux' and targetos != 'darwin'
> > > + error('virtio-9p (virtfs) requires Linux or Darwin')
> > > + elif targetos == 'darwin' and not
> >
> > cc.has_function('pthread_fchdir_np')
> >
> > > + error('virtio-9p (virtfs) on Darwin requires the presence of
> > > pthread_fchdir_np')
>
> Does the error message here suffice for that need? Right now if they're
> running a system without pthread_fchdir_np and don't specify the option, I
> think it'll just quietly disable, but if they --enable-virtfs and the
> function isn't there, they should get a note. I assume this is better, so
> that the ability to compile isn't contingent on having the latest OS, even
> if full support for older OSes isn't provided.
Ah, got it. Yes, makes sense.
But what I would definitely change is the precise error message text here:
"Darwin" is a bit awkward for a regular user, because most macOS users never
heard of "Darwin" in the context of Apple systems before. Using the term
"darwin" in code is fine as it can be assumed that developers know the
background, but as for regular users I would make it more clear that this is
actually about macOS, e.g:
error('virtio-9p (virtfs) requires either Linux or Darwin (macOS)')
I don't mind how to write that exactly; braces, slash, replacing Darwin by
macOS or whatever, but it should mention 'macOS' here in some form.
> > elif not libcap_ng.found() or not libattr.found()
> >
> > > error('virtio-9p (virtfs) requires libcap-ng-devel and
> > >
> > > libattr-devel') elif not have_system
