[Virtio-fs] [PATCH] virtiofsd: fix incorrect error handling in lo_do_lookup

[Virtio-fs] [PATCH] virtiofsd: fix incorrect error handling in lo_do_lookup

[Eric Ren]

Signed-off-by: Eric Ren <renzhen@linux.alibaba.com>
---
 contrib/virtiofsd/passthrough_ll.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index 574e209d95..78716c8aca 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -670,7 +670,6 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
 		close(newfd);
 		newfd = -1;
 	} else {
-		saverr = ENOMEM;
 		inode = calloc(1, sizeof(struct lo_inode));
 		if (!inode)
 			goto out_err;
@@ -691,7 +690,9 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
 	res = fstatat(inode->fd, "", &e->attr,
 		      AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
 	if (res == -1) {
+		saverr = errno;
 		unref_inode(lo, inode, 1);
+		errno = saverr;
 		goto out_err;
 	}
 
-- 
2.17.2 (Apple Git-113)

Re: [Virtio-fs] [PATCH] virtiofsd: fix incorrect error handling in lo_do_lookup

[Dr. David Alan Gilbert]

* Eric Ren (renzhen@linux.alibaba.com) wrote:
> Signed-off-by: Eric Ren <renzhen@linux.alibaba.com>
> ---
>  contrib/virtiofsd/passthrough_ll.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index 574e209d95..78716c8aca 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -670,7 +670,6 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
>  		close(newfd);
>  		newfd = -1;
>  	} else {
> -		saverr = ENOMEM;
>  		inode = calloc(1, sizeof(struct lo_inode));
>  		if (!inode)
>  			goto out_err;

Can you explain why you remove that?   How does it
get a sensible errno in this case?

> @@ -691,7 +690,9 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
>  	res = fstatat(inode->fd, "", &e->attr,
>  		      AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
>  	if (res == -1) {
> +		saverr = errno;
>  		unref_inode(lo, inode, 1);
> +		errno = saverr;

That one I agree with.

Dave

>  		goto out_err;
>  	}
>  
> -- 
> 2.17.2 (Apple Git-113)
> 
> _______________________________________________
> 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: fix incorrect error handling in lo_do_lookup

[Liu Bo]

On Tue, Jun 11, 2019 at 06:38:33PM +0100, Dr. David Alan Gilbert wrote:
> * Eric Ren (renzhen@linux.alibaba.com) wrote:
> > Signed-off-by: Eric Ren <renzhen@linux.alibaba.com>
> > ---
> >  contrib/virtiofsd/passthrough_ll.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> > index 574e209d95..78716c8aca 100644
> > --- a/contrib/virtiofsd/passthrough_ll.c
> > +++ b/contrib/virtiofsd/passthrough_ll.c
> > @@ -670,7 +670,6 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> >  		close(newfd);
> >  		newfd = -1;
> >  	} else {
> > -		saverr = ENOMEM;
> >  		inode = calloc(1, sizeof(struct lo_inode));
> >  		if (!inode)
> >  			goto out_err;
> 
> Can you explain why you remove that?   How does it
> get a sensible errno in this case?
>

Are you suggesting that calloc() doesn't set errno on failure?

the manual reads that

"The UNIX 98 standard requires malloc(), calloc(), and realloc() to
set errno to ENOMEM upon failure. Glibc assumes that this is done (and
the glibc versions of these routines do this); if you use a private
malloc implementation that does not set errno, then certain library
routines may fail without having a reason in errno."



> > @@ -691,7 +690,9 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> >  	res = fstatat(inode->fd, "", &e->attr,
> >  		      AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
> >  	if (res == -1) {
> > +		saverr = errno;
> >  		unref_inode(lo, inode, 1);
> > +		errno = saverr;
> 
> That one I agree with.
>

Well...can we instead set errno in place to avoid any surprise in the future?

thanks,
-liubo

> 
> >  		goto out_err;
> >  	}
> >  
> > -- 
> > 2.17.2 (Apple Git-113)
> > 
> > _______________________________________________
> > 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
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs

Re: [Virtio-fs] [PATCH] virtiofsd: fix incorrect error handling in lo_do_lookup

[Dr. David Alan Gilbert]

* Liu Bo (bo.liu@linux.alibaba.com) wrote:
> On Tue, Jun 11, 2019 at 06:38:33PM +0100, Dr. David Alan Gilbert wrote:
> > * Eric Ren (renzhen@linux.alibaba.com) wrote:
> > > Signed-off-by: Eric Ren <renzhen@linux.alibaba.com>
> > > ---
> > >  contrib/virtiofsd/passthrough_ll.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> > > index 574e209d95..78716c8aca 100644
> > > --- a/contrib/virtiofsd/passthrough_ll.c
> > > +++ b/contrib/virtiofsd/passthrough_ll.c
> > > @@ -670,7 +670,6 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > >  		close(newfd);
> > >  		newfd = -1;
> > >  	} else {
> > > -		saverr = ENOMEM;
> > >  		inode = calloc(1, sizeof(struct lo_inode));
> > >  		if (!inode)
> > >  			goto out_err;
> > 
> > Can you explain why you remove that?   How does it
> > get a sensible errno in this case?
> >
> 
> Are you suggesting that calloc() doesn't set errno on failure?
> 
> the manual reads that
> 
> "The UNIX 98 standard requires malloc(), calloc(), and realloc() to
> set errno to ENOMEM upon failure. Glibc assumes that this is done (and
> the glibc versions of these routines do this); if you use a private
> malloc implementation that does not set errno, then certain library
> routines may fail without having a reason in errno."

Oh OK, and the spec I just found also says it should; the linux manpage
wasn't clear to me.

In that case,

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

and I'll merge it.

> 
> 
> > > @@ -691,7 +690,9 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > >  	res = fstatat(inode->fd, "", &e->attr,
> > >  		      AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
> > >  	if (res == -1) {
> > > +		saverr = errno;
> > >  		unref_inode(lo, inode, 1);
> > > +		errno = saverr;
> > 
> > That one I agree with.
> >
> 
> Well...can we instead set errno in place to avoid any surprise in the future?
> 
> thanks,
> -liubo
> 
> > 
> > >  		goto out_err;
> > >  	}
> > >  
> > > -- 
> > > 2.17.2 (Apple Git-113)
> > > 
> > > _______________________________________________
> > > 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
> > 
> > _______________________________________________
> > 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: fix incorrect error handling in lo_do_lookup

[Liu Bo]

On Tue, Jun 11, 2019 at 07:43:16PM +0100, Dr. David Alan Gilbert wrote:
> * Liu Bo (bo.liu@linux.alibaba.com) wrote:
> > On Tue, Jun 11, 2019 at 06:38:33PM +0100, Dr. David Alan Gilbert wrote:
> > > * Eric Ren (renzhen@linux.alibaba.com) wrote:
> > > > Signed-off-by: Eric Ren <renzhen@linux.alibaba.com>
> > > > ---
> > > >  contrib/virtiofsd/passthrough_ll.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> > > > index 574e209d95..78716c8aca 100644
> > > > --- a/contrib/virtiofsd/passthrough_ll.c
> > > > +++ b/contrib/virtiofsd/passthrough_ll.c
> > > > @@ -670,7 +670,6 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > >  		close(newfd);
> > > >  		newfd = -1;
> > > >  	} else {
> > > > -		saverr = ENOMEM;
> > > >  		inode = calloc(1, sizeof(struct lo_inode));
> > > >  		if (!inode)
> > > >  			goto out_err;
> > > 
> > > Can you explain why you remove that?   How does it
> > > get a sensible errno in this case?
> > >
> > 
> > Are you suggesting that calloc() doesn't set errno on failure?
> > 
> > the manual reads that
> > 
> > "The UNIX 98 standard requires malloc(), calloc(), and realloc() to
> > set errno to ENOMEM upon failure. Glibc assumes that this is done (and
> > the glibc versions of these routines do this); if you use a private
> > malloc implementation that does not set errno, then certain library
> > routines may fail without having a reason in errno."
> 
> Oh OK, and the spec I just found also says it should; the linux manpage
> wasn't clear to me.
> 
> In that case,
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> and I'll merge it.
>

Although the patch fixed the problem we've observed, do you think we
should instead set errno in place to avoid any surprise in the future?

thanks,
-liubo

> > 
> > 
> > > > @@ -691,7 +690,9 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > >  	res = fstatat(inode->fd, "", &e->attr,
> > > >  		      AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
> > > >  	if (res == -1) {
> > > > +		saverr = errno;
> > > >  		unref_inode(lo, inode, 1);
> > > > +		errno = saverr;
> > > 
> > > That one I agree with.
> > >
> > 
> > Well...can we instead set errno in place to avoid any surprise in the future?
> > 
> > thanks,
> > -liubo
> > 
> > > 
> > > >  		goto out_err;
> > > >  	}
> > > >  
> > > > -- 
> > > > 2.17.2 (Apple Git-113)
> > > > 
> > > > _______________________________________________
> > > > 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
> > > 
> > > _______________________________________________
> > > 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