[PATCH 0/2] virtiofsd: Enable xattr if xattrmap is used

[PATCH 0/2] virtiofsd: Enable xattr if xattrmap is used

[Carlos Venegas]

Using xattrmap for Kata Containers we found that xattr is should be used
or xattrmap wont work.  These patches enable xattr when -o xattrmap is
used. Also, they add help for the xattrmap  option on `virtiofsd --help` output.

Carlos Venegas (2):
  virtiofsd: Allow use "-o xattrmap" without "-o xattr"
  virtiofsd: Add help for -o xattr-mapping

 tools/virtiofsd/helper.c         | 3 +++
 tools/virtiofsd/passthrough_ll.c | 1 +
 2 files changed, 4 insertions(+)

-- 
2.25.1

[PATCH 1/2] virtiofsd: Allow use "-o xattrmap" without "-o xattr"

[Carlos Venegas]

When -o xattrmap is used, it will not work unless xattr is enabled.

This patch enables xattr when -o xattrmap is used.

Signed-off-by: Carlos Venegas <jose.carlos.venegas.munoz@intel.com>
---
 tools/virtiofsd/passthrough_ll.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index ddaf57305c..2337ea5a58 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -3939,6 +3939,7 @@ int main(int argc, char *argv[])
     }
 
     if (lo.xattrmap) {
+        lo.xattr = 1;
         parse_xattrmap(&lo);
     }
 
-- 
2.25.1

Re: [Virtio-fs] [PATCH 1/2] virtiofsd: Allow use "-o xattrmap" without "-o xattr"

[Greg Kurz]

On Wed, 14 Apr 2021 20:12:06 +0000
Carlos Venegas <jose.carlos.venegas.munoz@intel.com> wrote:

> When -o xattrmap is used, it will not work unless xattr is enabled.
> 
> This patch enables xattr when -o xattrmap is used.
> 
> Signed-off-by: Carlos Venegas <jose.carlos.venegas.munoz@intel.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index ddaf57305c..2337ea5a58 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -3939,6 +3939,7 @@ int main(int argc, char *argv[])
>      }
>  
>      if (lo.xattrmap) {
> +        lo.xattr = 1;
>          parse_xattrmap(&lo);
>      }
>  

This seems reasonable. I'm just wondering if we should also
add an explicit error if the user tries something silly
like -o xattrmap=MAPPING,no_xattr instead of silently
ignoring no_xattr...

Re: [Virtio-fs] [PATCH 1/2] virtiofsd: Allow use "-o xattrmap" without "-o xattr"

[Vivek Goyal]

On Fri, Apr 16, 2021 at 01:05:37PM +0200, Greg Kurz wrote:
> On Wed, 14 Apr 2021 20:12:06 +0000
> Carlos Venegas <jose.carlos.venegas.munoz@intel.com> wrote:
> 
> > When -o xattrmap is used, it will not work unless xattr is enabled.
> > 
> > This patch enables xattr when -o xattrmap is used.
> > 
> > Signed-off-by: Carlos Venegas <jose.carlos.venegas.munoz@intel.com>
> > ---
> >  tools/virtiofsd/passthrough_ll.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index ddaf57305c..2337ea5a58 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -3939,6 +3939,7 @@ int main(int argc, char *argv[])
> >      }
> >  
> >      if (lo.xattrmap) {
> > +        lo.xattr = 1;
> >          parse_xattrmap(&lo);
> >      }
> >  
> 
> This seems reasonable. I'm just wondering if we should also
> add an explicit error if the user tries something silly
> like -o xattrmap=MAPPING,no_xattr instead of silently
> ignoring no_xattr...

That's a good point. Will be nice to error out if user provides
conflicting options.

Vivek

Re: [Virtio-fs] [PATCH 1/2] virtiofsd: Allow use "-o xattrmap" without "-o xattr"

[Dr. David Alan Gilbert]

* Greg Kurz (groug@kaod.org) wrote:
> On Wed, 14 Apr 2021 20:12:06 +0000
> Carlos Venegas <jose.carlos.venegas.munoz@intel.com> wrote:
> 
> > When -o xattrmap is used, it will not work unless xattr is enabled.
> > 
> > This patch enables xattr when -o xattrmap is used.
> > 
> > Signed-off-by: Carlos Venegas <jose.carlos.venegas.munoz@intel.com>
> > ---
> >  tools/virtiofsd/passthrough_ll.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index ddaf57305c..2337ea5a58 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -3939,6 +3939,7 @@ int main(int argc, char *argv[])
> >      }
> >  
> >      if (lo.xattrmap) {
> > +        lo.xattr = 1;
> >          parse_xattrmap(&lo);
> >      }
> >  
> 
> This seems reasonable. I'm just wondering if we should also
> add an explicit error if the user tries something silly
> like -o xattrmap=MAPPING,no_xattr instead of silently
> ignoring no_xattr...

That would be a nice addition, although I'll take this set is for now.

Dave

> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 2/2] virtiofsd: Add help for -o xattr-mapping

[Carlos Venegas]

The option is not documented in help.

Add small help about the option.

Signed-off-by: Carlos Venegas <jose.carlos.venegas.munoz@intel.com>
---
 tools/virtiofsd/helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
index 28243b51b2..5e98ed702b 100644
--- a/tools/virtiofsd/helper.c
+++ b/tools/virtiofsd/helper.c
@@ -172,6 +172,9 @@ void fuse_cmdline_help(void)
            "                               default: no_writeback\n"
            "    -o xattr|no_xattr          enable/disable xattr\n"
            "                               default: no_xattr\n"
+           "    -o xattrmap=<mapping>      Enable xattr mapping (enables xattr)\n"
+           "                               <mapping> is a string consists of a series of rules\n"
+           "                               e.g. -o xattrmap=:map::user.virtiofs.:\n"
            "    -o modcaps=CAPLIST         Modify the list of capabilities\n"
            "                               e.g. -o modcaps=+sys_admin:-chown\n"
            "    --rlimit-nofile=<num>      set maximum number of file descriptors\n"
-- 
2.25.1

Re: [Virtio-fs] [PATCH 2/2] virtiofsd: Add help for -o xattr-mapping

[Connor Kuehl]

On Wed Apr 14, 2021 at 3:12 PM CDT, Carlos Venegas wrote:
> The option is not documented in help.
>
> Add small help about the option.
>
> Signed-off-by: Carlos Venegas <jose.carlos.venegas.munoz@intel.com>
> ---
> tools/virtiofsd/helper.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
> index 28243b51b2..5e98ed702b 100644
> --- a/tools/virtiofsd/helper.c
> +++ b/tools/virtiofsd/helper.c
> @@ -172,6 +172,9 @@ void fuse_cmdline_help(void)
> " default: no_writeback\n"
> " -o xattr|no_xattr enable/disable xattr\n"
> " default: no_xattr\n"
> + " -o xattrmap=<mapping> Enable xattr mapping (enables xattr)\n"
> + " <mapping> is a string consists of a series of rules\n"
> + " e.g. -o xattrmap=:map::user.virtiofs.:\n"

This is a helpful note, but it doesn't tell the whole story. I think
it'd be helpful to add one last note to this option which is to
recommend reading the virtiofsd(1) man-page for more information on
xattrmap rules.

Connor

Re: [Virtio-fs] [PATCH 2/2] virtiofsd: Add help for -o xattr-mapping

[Vivek Goyal]

On Wed, Apr 14, 2021 at 04:44:26PM -0500, Connor Kuehl wrote:
> On Wed Apr 14, 2021 at 3:12 PM CDT, Carlos Venegas wrote:
> > The option is not documented in help.
> >
> > Add small help about the option.
> >
> > Signed-off-by: Carlos Venegas <jose.carlos.venegas.munoz@intel.com>
> > ---
> > tools/virtiofsd/helper.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
> > index 28243b51b2..5e98ed702b 100644
> > --- a/tools/virtiofsd/helper.c
> > +++ b/tools/virtiofsd/helper.c
> > @@ -172,6 +172,9 @@ void fuse_cmdline_help(void)
> > " default: no_writeback\n"
> > " -o xattr|no_xattr enable/disable xattr\n"
> > " default: no_xattr\n"
> > + " -o xattrmap=<mapping> Enable xattr mapping (enables xattr)\n"
> > + " <mapping> is a string consists of a series of rules\n"
> > + " e.g. -o xattrmap=:map::user.virtiofs.:\n"
> 
> This is a helpful note, but it doesn't tell the whole story. I think
> it'd be helpful to add one last note to this option which is to
> recommend reading the virtiofsd(1) man-page for more information on
> xattrmap rules.

Is there a virtiofsd man page as well? All I see is
docs/tools/virtiofsd.rst.

Vivek

Re: [Virtio-fs] [PATCH 2/2] virtiofsd: Add help for -o xattr-mapping

[Connor Kuehl]

On 4/19/21 2:07 PM, Vivek Goyal wrote:
>> This is a helpful note, but it doesn't tell the whole story. I think
>> it'd be helpful to add one last note to this option which is to
>> recommend reading the virtiofsd(1) man-page for more information on
>> xattrmap rules.
> 
> Is there a virtiofsd man page as well? All I see is
> docs/tools/virtiofsd.rst.

Yes, it's generated from that file. Should be located in
qemu/build/docs/virtiofsd.1 after building QEMU.

Connor

Re: [Virtio-fs] [PATCH 2/2] virtiofsd: Add help for -o xattr-mapping

[Vivek Goyal]

On Mon, Apr 19, 2021 at 02:21:11PM -0500, Connor Kuehl wrote:
> On 4/19/21 2:07 PM, Vivek Goyal wrote:
> >> This is a helpful note, but it doesn't tell the whole story. I think
> >> it'd be helpful to add one last note to this option which is to
> >> recommend reading the virtiofsd(1) man-page for more information on
> >> xattrmap rules.
> > 
> > Is there a virtiofsd man page as well? All I see is
> > docs/tools/virtiofsd.rst.
> 
> Yes, it's generated from that file. Should be located in
> qemu/build/docs/virtiofsd.1 after building QEMU.

Ok thanks. I guess that gets build only if I pass option --enable-docs. 

Thanks
Vivek

Re: [PATCH 0/2] virtiofsd: Enable xattr if xattrmap is used

[Connor Kuehl]

On Wed Apr 14, 2021 at 3:12 PM CDT, Carlos Venegas wrote:
>
> Using xattrmap for Kata Containers we found that xattr is should be used
> or xattrmap wont work. These patches enable xattr when -o xattrmap is
> used. Also, they add help for the xattrmap option on `virtiofsd --help`
> output.
>
> Carlos Venegas (2):
> virtiofsd: Allow use "-o xattrmap" without "-o xattr"
> virtiofsd: Add help for -o xattr-mapping

Good usability improvement.

For the series:

Reviewed-by: Connor Kuehl <ckuehl@redhat.com>

Re: [PATCH 0/2] virtiofsd: Enable xattr if xattrmap is used

[Dr. David Alan Gilbert]

* Carlos Venegas (jose.carlos.venegas.munoz@intel.com) wrote:
> 
> Using xattrmap for Kata Containers we found that xattr is should be used
> or xattrmap wont work.  These patches enable xattr when -o xattrmap is
> used. Also, they add help for the xattrmap  option on `virtiofsd --help` output.

Queued.  You might like to submit some more patches to give the error
that Greg was suggesting and/or update some docs.

Dave

> Carlos Venegas (2):
>   virtiofsd: Allow use "-o xattrmap" without "-o xattr"
>   virtiofsd: Add help for -o xattr-mapping
> 
>  tools/virtiofsd/helper.c         | 3 +++
>  tools/virtiofsd/passthrough_ll.c | 1 +
>  2 files changed, 4 insertions(+)
> 
> -- 
> 2.25.1
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

RE: [PATCH 0/2] virtiofsd: Enable xattr if xattrmap is used

[Venegas Munoz, Jose Carlos]

Thank you both for the suggestions, I had not took a look recently, I will check today in my afternoon.

Cheers.

> -----Original Message-----
> From: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Sent: Thursday, May 6, 2021 10:53 AM
> To: Venegas Munoz, Jose Carlos <jose.carlos.venegas.munoz@intel.com>
> Cc: virtio-fs@redhat.com; qemu-devel@nongnu.org; vgoyal@redhat.com
> Subject: Re: [PATCH 0/2] virtiofsd: Enable xattr if xattrmap is used
> 
> * Carlos Venegas (jose.carlos.venegas.munoz@intel.com) wrote:
> >
> > Using xattrmap for Kata Containers we found that xattr is should be
> > used or xattrmap wont work.  These patches enable xattr when -o
> > xattrmap is used. Also, they add help for the xattrmap  option on `virtiofsd -
> -help` output.
> 
> Queued.  You might like to submit some more patches to give the error that
> Greg was suggesting and/or update some docs.
> 
> Dave
> 
> > Carlos Venegas (2):
> >   virtiofsd: Allow use "-o xattrmap" without "-o xattr"
> >   virtiofsd: Add help for -o xattr-mapping
> >
> >  tools/virtiofsd/helper.c         | 3 +++
> >  tools/virtiofsd/passthrough_ll.c | 1 +
> >  2 files changed, 4 insertions(+)
> >
> > --
> > 2.25.1
> >
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK