diff mbox series

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

Message ID 20220206200719.74464-11-wwcohen@gmail.com (mailing list archive)
State New, archived
Headers show
Series 9p: Add support for darwin | expand

Commit Message

Will Cohen Feb. 6, 2022, 8:07 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé Feb. 6, 2022, 9:22 p.m. UTC | #1
On 6/2/22 21:07, 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(-)

> -have_virtfs_proxy_helper = have_virtfs and have_tools
> +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and have_tools

Why do you restrict the proxy-helper to Linux?
Will Cohen Feb. 7, 2022, 1:05 a.m. UTC | #2
Only because porting the proxy-helper to macOS is outside the scope of this
particular patch. While some initial concepts around it have been
considered by some of the contributors to this patch, those implementations
weren't tested enough and the security implications weren't considered in
full. We assume that this could be an additional implementation later on,
if the functionality is considered important down the road.

On Sun, Feb 6, 2022 at 4:22 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> On 6/2/22 21:07, 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(-)
>
> > -have_virtfs_proxy_helper = have_virtfs and have_tools
> > +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and
> have_tools
>
> Why do you restrict the proxy-helper to Linux?
>
Christian Schoenebeck Feb. 7, 2022, 2:15 p.m. UTC | #3
On Montag, 7. Februar 2022 02:05:32 CET Will Cohen wrote:
> On Sun, Feb 6, 2022 at 4:22 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> wrote:
> > On 6/2/22 21:07, 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(-)
> > > 
> > > -have_virtfs_proxy_helper = have_virtfs and have_tools
> > > +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and
> > 
> > have_tools
> > 
> > Why do you restrict the proxy-helper to Linux?
>
> Only because porting the proxy-helper to macOS is outside the scope of this
> particular patch. While some initial concepts around it have been
> considered by some of the contributors to this patch, those implementations
> weren't tested enough and the security implications weren't considered in
> full. We assume that this could be an additional implementation later on,
> if the functionality is considered important down the road.

In general that's fine with me. I would have probably made that
"targetos != 'darwin'" instead of "targetos == 'linux'", but I leave that up 
to you.

On the long term we will probably deprecate the 9p 'proxy' fs driver anyway. 
While it had some good ideas, being realistic though: nobody has worked on the 
9p proxy driver/backend for many years and it is not in good shape.

I can imagine that due to the ground being laid by these series, that we will 
also open 9p for BSD, but that should be done a bit later and hence does not 
belong into these series.

But once again: it would not have hurt to make your intentions clear either in 
the commit log or by in-source comment. :)

Best regards,
Christian Schoenebeck
Will Cohen Feb. 7, 2022, 2:18 p.m. UTC | #4
On Mon, Feb 7, 2022 at 9:15 AM Christian Schoenebeck <qemu_oss@crudebyte.com>
wrote:

> On Montag, 7. Februar 2022 02:05:32 CET Will Cohen wrote:
> > On Sun, Feb 6, 2022 at 4:22 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
> >
> > wrote:
> > > On 6/2/22 21:07, 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(-)
> > > >
> > > > -have_virtfs_proxy_helper = have_virtfs and have_tools
> > > > +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and
> > >
> > > have_tools
> > >
> > > Why do you restrict the proxy-helper to Linux?
> >
> > Only because porting the proxy-helper to macOS is outside the scope of
> this
> > particular patch. While some initial concepts around it have been
> > considered by some of the contributors to this patch, those
> implementations
> > weren't tested enough and the security implications weren't considered in
> > full. We assume that this could be an additional implementation later on,
> > if the functionality is considered important down the road.
>
> In general that's fine with me. I would have probably made that
> "targetos != 'darwin'" instead of "targetos == 'linux'", but I leave that
> up
> to you.
>
> On the long term we will probably deprecate the 9p 'proxy' fs driver
> anyway.
> While it had some good ideas, being realistic though: nobody has worked on
> the
> 9p proxy driver/backend for many years and it is not in good shape.
>
> I can imagine that due to the ground being laid by these series, that we
> will
> also open 9p for BSD, but that should be done a bit later and hence does
> not
> belong into these series.
>
> But once again: it would not have hurt to make your intentions clear
> either in
> the commit log or by in-source comment. :)
>
> Best regards,
> Christian Schoenebeck
>
>
>
Acknowledged! For v5 will change it to != 'darwin' and note it as well, for
clarity.
Christian Schoenebeck Feb. 7, 2022, 2:27 p.m. UTC | #5
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') elif not libcap_ng.found() or not libattr.found()
>        error('virtio-9p (virtfs) requires libcap-ng-devel and
> libattr-devel') elif not have_system
Will Cohen Feb. 7, 2022, 2:37 p.m. UTC | #6
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.


> elif not libcap_ng.found() or not libattr.found()
> >        error('virtio-9p (virtfs) requires libcap-ng-devel and
> > libattr-devel') elif not have_system
>
>
>
Greg Kurz Feb. 7, 2022, 2:39 p.m. UTC | #7
On Mon, 07 Feb 2022 15:15:46 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Montag, 7. Februar 2022 02:05:32 CET Will Cohen wrote:
> > On Sun, Feb 6, 2022 at 4:22 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
> > 
> > wrote:
> > > On 6/2/22 21:07, 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(-)
> > > > 
> > > > -have_virtfs_proxy_helper = have_virtfs and have_tools
> > > > +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and
> > > 
> > > have_tools
> > > 
> > > Why do you restrict the proxy-helper to Linux?
> >
> > Only because porting the proxy-helper to macOS is outside the scope of this
> > particular patch. While some initial concepts around it have been
> > considered by some of the contributors to this patch, those implementations
> > weren't tested enough and the security implications weren't considered in
> > full. We assume that this could be an additional implementation later on,
> > if the functionality is considered important down the road.
> 
> In general that's fine with me. I would have probably made that
> "targetos != 'darwin'" instead of "targetos == 'linux'", but I leave that up 
> to you.
> 
> On the long term we will probably deprecate the 9p 'proxy' fs driver anyway. 
> While it had some good ideas, being realistic though: nobody has worked on the 
> 9p proxy driver/backend for many years and it is not in good shape.
> 

It definitely isn't indeed. Also it is super slow by design
since the round trip of a 9p request involves QEMU on both entry
and exit:

   [guest] --> [QEMU]--> [virtfs-proxy-helper]-->[QEMU]-->[guest]

A more modern and efficient approach would be to have a vhost-user-9p
implementation : requests would be directly handled by the external
process, without QEMU hops. But this a fair amount of work.

> I can imagine that due to the ground being laid by these series, that we will 
> also open 9p for BSD, but that should be done a bit later and hence does not 
> belong into these series.
> 
> But once again: it would not have hurt to make your intentions clear either in 
> the commit log or by in-source comment. :)
> 
> Best regards,
> Christian Schoenebeck
> 
>
Christian Schoenebeck Feb. 7, 2022, 3:38 p.m. UTC | #8
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
Christian Schoenebeck Feb. 7, 2022, 4:04 p.m. UTC | #9
On Montag, 7. Februar 2022 15:39:30 CET Greg Kurz wrote:
> On Mon, 07 Feb 2022 15:15:46 +0100
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Montag, 7. Februar 2022 02:05:32 CET Will Cohen wrote:
> > > On Sun, Feb 6, 2022 at 4:22 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > 
> > > wrote:
> > > > On 6/2/22 21:07, 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(-)
> > > > > 
> > > > > -have_virtfs_proxy_helper = have_virtfs and have_tools
> > > > > +have_virtfs_proxy_helper = targetos == 'linux' and have_virtfs and
> > > > 
> > > > have_tools
> > > > 
> > > > Why do you restrict the proxy-helper to Linux?
> > > 
> > > Only because porting the proxy-helper to macOS is outside the scope of
> > > this
> > > particular patch. While some initial concepts around it have been
> > > considered by some of the contributors to this patch, those
> > > implementations
> > > weren't tested enough and the security implications weren't considered
> > > in
> > > full. We assume that this could be an additional implementation later
> > > on,
> > > if the functionality is considered important down the road.
> > 
> > In general that's fine with me. I would have probably made that
> > "targetos != 'darwin'" instead of "targetos == 'linux'", but I leave that
> > up to you.
> > 
> > On the long term we will probably deprecate the 9p 'proxy' fs driver
> > anyway. While it had some good ideas, being realistic though: nobody has
> > worked on the 9p proxy driver/backend for many years and it is not in
> > good shape.
> It definitely isn't indeed. Also it is super slow by design
> since the round trip of a 9p request involves QEMU on both entry
> and exit:
> 
>    [guest] --> [QEMU]--> [virtfs-proxy-helper]-->[QEMU]-->[guest]
> 
> A more modern and efficient approach would be to have a vhost-user-9p
> implementation : requests would be directly handled by the external
> process, without QEMU hops. But this a fair amount of work.

That's already a bit offtopic, but how would you imagine that to work? You 
mean a system dependent solution that e.g. plugs in into KVM or something?

> > I can imagine that due to the ground being laid by these series, that we
> > will also open 9p for BSD, but that should be done a bit later and hence
> > does not belong into these series.
> > 
> > But once again: it would not have hurt to make your intentions clear
> > either in the commit log or by in-source comment. :)
> > 
> > Best regards,
> > Christian Schoenebeck
diff mbox series

Patch

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
+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')
     elif not libcap_ng.found() or not libattr.found()
       error('virtio-9p (virtfs) requires libcap-ng-devel and libattr-devel')
     elif not have_system