[Virtio-fs] [PATCH] virtiofsd/passthrough_ll: Pass errno to fuse_reply_err()

[Virtio-fs] [PATCH] virtiofsd/passthrough_ll: Pass errno to fuse_reply_err()

[Xiao Yang]

lo_copy_file_range() passes -errno to fuse_reply_err() and then fuse_reply_err()
changes it to errno again, so that subsequent fuse_send_reply_iov_nofree() catches
the wrong errno.(i.e. reports "fuse: bad error value: ...").

Make fuse_send_reply_iov_nofree() accept the correct -errno by passing errno
directly in lo_copy_file_range().

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 tools/virtiofsd/passthrough_ll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 83dd0084b4..a77bed655e 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2615,7 +2615,7 @@ static void lo_copy_file_range(fuse_req_t req, fuse_ino_t ino_in, off_t off_in,
 
     res = copy_file_range(in_fd, &off_in, out_fd, &off_out, len, flags);
     if (res < 0) {
-        fuse_reply_err(req, -errno);
+        fuse_reply_err(req, errno);
     } else {
         fuse_reply_write(req, res);
     }
-- 
2.21.0

Re: [Virtio-fs] [PATCH] virtiofsd/passthrough_ll: Pass errno to fuse_reply_err()

[Eryu Guan]

On Thu, Jan 02, 2020 at 11:53:12AM +0800, Xiao Yang wrote:
> lo_copy_file_range() passes -errno to fuse_reply_err() and then fuse_reply_err()
> changes it to errno again, so that subsequent fuse_send_reply_iov_nofree() catches
> the wrong errno.(i.e. reports "fuse: bad error value: ...").
> 
> Make fuse_send_reply_iov_nofree() accept the correct -errno by passing errno
> directly in lo_copy_file_range().
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>

Looks good to me.

Reviewed-by: Eryu Guan <eguan@linux.alibaba.com>

I also went through the other fuse_reply_err() calls and they all pass
positive error number to the second arg.

Thanks,
Eryu

> ---
>  tools/virtiofsd/passthrough_ll.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 83dd0084b4..a77bed655e 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -2615,7 +2615,7 @@ static void lo_copy_file_range(fuse_req_t req, fuse_ino_t ino_in, off_t off_in,
>  
>      res = copy_file_range(in_fd, &off_in, out_fd, &off_out, len, flags);
>      if (res < 0) {
> -        fuse_reply_err(req, -errno);
> +        fuse_reply_err(req, errno);
>      } else {
>          fuse_reply_write(req, res);
>      }
> -- 
> 2.21.0
> 
> 
> 
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs

Re: [Virtio-fs] [PATCH] virtiofsd/passthrough_ll: Pass errno to fuse_reply_err()

[Xiao Yang]

On 2020/1/2 12:50, Eryu Guan wrote:
> On Thu, Jan 02, 2020 at 11:53:12AM +0800, Xiao Yang wrote:
>> >  lo_copy_file_range() passes -errno to fuse_reply_err() and then fuse_reply_err()
>> >  changes it to errno again, so that subsequent fuse_send_reply_iov_nofree() catches
>> >  the wrong errno.(i.e. reports "fuse: bad error value: ...").
>> >  
>> >  Make fuse_send_reply_iov_nofree() accept the correct -errno by passing errno
>> >  directly in lo_copy_file_range().
>> >  
>> >  Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> Looks good to me.
>
> Reviewed-by: Eryu Guan<eguan@linux.alibaba.com>
>
> I also went through the other fuse_reply_err() calls and they all pass
> positive error number to the second arg.
Hi Eryu,

Thanks for your review. :-)

Best Regards,
Xiao Yang
> Thanks,
> Eryu
>

Re: [Virtio-fs] [PATCH] virtiofsd/passthrough_ll: Pass errno to fuse_reply_err()

[Dr. David Alan Gilbert]

* Xiao Yang (yangx.jy@cn.fujitsu.com) wrote:
> lo_copy_file_range() passes -errno to fuse_reply_err() and then fuse_reply_err()
> changes it to errno again, so that subsequent fuse_send_reply_iov_nofree() catches
> the wrong errno.(i.e. reports "fuse: bad error value: ...").
> 
> Make fuse_send_reply_iov_nofree() accept the correct -errno by passing errno
> directly in lo_copy_file_range().
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>

Yes, I think you're right - FUSE does use +ve errno's for some things,
normally the failure of the fuse channel itself; but in this case it
looks like 'errno' is consistent with the other calls; this comes
straight from upstream libfuse commit 2548c4b8, so lets cc in Niels.

I'll take it into virtiofsd for the moment; thanks!

Dave

> ---
>  tools/virtiofsd/passthrough_ll.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 83dd0084b4..a77bed655e 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -2615,7 +2615,7 @@ static void lo_copy_file_range(fuse_req_t req, fuse_ino_t ino_in, off_t off_in,
>  
>      res = copy_file_range(in_fd, &off_in, out_fd, &off_out, len, flags);
>      if (res < 0) {
> -        fuse_reply_err(req, -errno);
> +        fuse_reply_err(req, errno);
>      } else {
>          fuse_reply_write(req, res);
>      }
> -- 
> 2.21.0
> 
> 
> 
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH] virtiofsd/passthrough_ll: Pass errno to fuse_reply_err()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 628 bytes --]

On Thu, Jan 02, 2020 at 11:53:12AM +0800, Xiao Yang wrote:
> lo_copy_file_range() passes -errno to fuse_reply_err() and then fuse_reply_err()
> changes it to errno again, so that subsequent fuse_send_reply_iov_nofree() catches
> the wrong errno.(i.e. reports "fuse: bad error value: ...").
> 
> Make fuse_send_reply_iov_nofree() accept the correct -errno by passing errno
> directly in lo_copy_file_range().
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Virtio-fs] [PATCH] virtiofsd/passthrough_ll: Pass errno to fuse_reply_err()

[Niels de Vos]

On Thu, Jan 02, 2020 at 06:55:40PM +0000, Dr. David Alan Gilbert wrote:
> * Xiao Yang (yangx.jy@cn.fujitsu.com) wrote:
> > lo_copy_file_range() passes -errno to fuse_reply_err() and then fuse_reply_err()
> > changes it to errno again, so that subsequent fuse_send_reply_iov_nofree() catches
> > the wrong errno.(i.e. reports "fuse: bad error value: ...").
> > 
> > Make fuse_send_reply_iov_nofree() accept the correct -errno by passing errno
> > directly in lo_copy_file_range().
> > 
> > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> 
> Yes, I think you're right - FUSE does use +ve errno's for some things,
> normally the failure of the fuse channel itself; but in this case it
> looks like 'errno' is consistent with the other calls; this comes
> straight from upstream libfuse commit 2548c4b8, so lets cc in Niels.

Indeed, setting a negative errno is a mistake. Could you send the fix to
https://github.com/libfuse/libfuse (example/passthrough_ll.c) as well? I
am happy to do so if you prefer, just let me know.

Thanks,
Niels

> 
> I'll take it into virtiofsd for the moment; thanks!
> 
> Dave
> 
> > ---
> >  tools/virtiofsd/passthrough_ll.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index 83dd0084b4..a77bed655e 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -2615,7 +2615,7 @@ static void lo_copy_file_range(fuse_req_t req, fuse_ino_t ino_in, off_t off_in,
> >  
> >      res = copy_file_range(in_fd, &off_in, out_fd, &off_out, len, flags);
> >      if (res < 0) {
> > -        fuse_reply_err(req, -errno);
> > +        fuse_reply_err(req, errno);
> >      } else {
> >          fuse_reply_write(req, res);
> >      }
> > -- 
> > 2.21.0
> > 
> > 
> > 
> > 
> > _______________________________________________
> > Virtio-fs mailing list
> > Virtio-fs@redhat.com
> > https://www.redhat.com/mailman/listinfo/virtio-fs
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH] virtiofsd/passthrough_ll: Pass errno to fuse_reply_err()

[Dr. David Alan Gilbert]

* Niels de Vos (ndevos@redhat.com) wrote:
> On Thu, Jan 02, 2020 at 06:55:40PM +0000, Dr. David Alan Gilbert wrote:
> > * Xiao Yang (yangx.jy@cn.fujitsu.com) wrote:
> > > lo_copy_file_range() passes -errno to fuse_reply_err() and then fuse_reply_err()
> > > changes it to errno again, so that subsequent fuse_send_reply_iov_nofree() catches
> > > the wrong errno.(i.e. reports "fuse: bad error value: ...").
> > > 
> > > Make fuse_send_reply_iov_nofree() accept the correct -errno by passing errno
> > > directly in lo_copy_file_range().
> > > 
> > > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> > 
> > Yes, I think you're right - FUSE does use +ve errno's for some things,
> > normally the failure of the fuse channel itself; but in this case it
> > looks like 'errno' is consistent with the other calls; this comes
> > straight from upstream libfuse commit 2548c4b8, so lets cc in Niels.
> 
> Indeed, setting a negative errno is a mistake. Could you send the fix to
> https://github.com/libfuse/libfuse (example/passthrough_ll.c) as well? I
> am happy to do so if you prefer, just let me know.

I'll do a pull with Xiao Yang's fix.

Dave

> Thanks,
> Niels
> 
> > 
> > I'll take it into virtiofsd for the moment; thanks!
> > 
> > Dave
> > 
> > > ---
> > >  tools/virtiofsd/passthrough_ll.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > index 83dd0084b4..a77bed655e 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -2615,7 +2615,7 @@ static void lo_copy_file_range(fuse_req_t req, fuse_ino_t ino_in, off_t off_in,
> > >  
> > >      res = copy_file_range(in_fd, &off_in, out_fd, &off_out, len, flags);
> > >      if (res < 0) {
> > > -        fuse_reply_err(req, -errno);
> > > +        fuse_reply_err(req, errno);
> > >      } else {
> > >          fuse_reply_write(req, res);
> > >      }
> > > -- 
> > > 2.21.0
> > > 
> > > 
> > > 
> > > 
> > > _______________________________________________
> > > Virtio-fs mailing list
> > > Virtio-fs@redhat.com
> > > https://www.redhat.com/mailman/listinfo/virtio-fs
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH] virtiofsd/passthrough_ll: Pass errno to fuse_reply_err()

[Dr. David Alan Gilbert]

* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> * Niels de Vos (ndevos@redhat.com) wrote:
> > On Thu, Jan 02, 2020 at 06:55:40PM +0000, Dr. David Alan Gilbert wrote:
> > > * Xiao Yang (yangx.jy@cn.fujitsu.com) wrote:
> > > > lo_copy_file_range() passes -errno to fuse_reply_err() and then fuse_reply_err()
> > > > changes it to errno again, so that subsequent fuse_send_reply_iov_nofree() catches
> > > > the wrong errno.(i.e. reports "fuse: bad error value: ...").
> > > > 
> > > > Make fuse_send_reply_iov_nofree() accept the correct -errno by passing errno
> > > > directly in lo_copy_file_range().
> > > > 
> > > > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> > > 
> > > Yes, I think you're right - FUSE does use +ve errno's for some things,
> > > normally the failure of the fuse channel itself; but in this case it
> > > looks like 'errno' is consistent with the other calls; this comes
> > > straight from upstream libfuse commit 2548c4b8, so lets cc in Niels.
> > 
> > Indeed, setting a negative errno is a mistake. Could you send the fix to
> > https://github.com/libfuse/libfuse (example/passthrough_ll.c) as well? I
> > am happy to do so if you prefer, just let me know.
> 
> I'll do a pull with Xiao Yang's fix.

https://github.com/libfuse/libfuse/pull/486

> Dave
> 
> > Thanks,
> > Niels
> > 
> > > 
> > > I'll take it into virtiofsd for the moment; thanks!
> > > 
> > > Dave
> > > 
> > > > ---
> > > >  tools/virtiofsd/passthrough_ll.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > > index 83dd0084b4..a77bed655e 100644
> > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > @@ -2615,7 +2615,7 @@ static void lo_copy_file_range(fuse_req_t req, fuse_ino_t ino_in, off_t off_in,
> > > >  
> > > >      res = copy_file_range(in_fd, &off_in, out_fd, &off_out, len, flags);
> > > >      if (res < 0) {
> > > > -        fuse_reply_err(req, -errno);
> > > > +        fuse_reply_err(req, errno);
> > > >      } else {
> > > >          fuse_reply_write(req, res);
> > > >      }
> > > > -- 
> > > > 2.21.0
> > > > 
> > > > 
> > > > 
> > > > 
> > > > _______________________________________________
> > > > Virtio-fs mailing list
> > > > Virtio-fs@redhat.com
> > > > https://www.redhat.com/mailman/listinfo/virtio-fs
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK