Re: [PATCH v4 10/11] 9p: darwin: meson: Allow VirtFS on Darwin

[Christian Schoenebeck]

On Montag, 7. Februar 2022 15:37:00 CET Will Cohen wrote:
> On Mon, Feb 7, 2022 at 9:27 AM Christian Schoenebeck
> <qemu_oss@crudebyte.com>
> wrote:
> > On Sonntag, 6. Februar 2022 21:07:18 CET Will Cohen wrote:
> > > From: Keno Fischer <keno@juliacomputing.com>
> > > 
> > > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > > [Michael Roitzsch: - Rebase for NixOS]
> > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com>
> > > [Will Cohen: - Rebase to master]
> > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > > [Will Cohen: - Add check for pthread_fchdir_np to virtfs]
> > > Signed-off-by: Will Cohen <wwcohen@gmail.com>
> > > ---
> > > 
> > >  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