[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 at linux.alibaba.com) wrote:
> > On Tue, Jun 11, 2019 at 06:38:33PM +0100, Dr. David Alan Gilbert wrote:
> > > * Eric Ren (renzhen at linux.alibaba.com) wrote:
> > > > Signed-off-by: Eric Ren <renzhen at 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 at 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 at redhat.com
> > > > https://www.redhat.com/mailman/listinfo/virtio-fs
> > > --
> > > Dr. David Alan Gilbert / dgilbert at redhat.com / Manchester, UK
> > > 
> > > _______________________________________________
> > > Virtio-fs mailing list
> > > Virtio-fs at redhat.com
> > > https://www.redhat.com/mailman/listinfo/virtio-fs
> --
> Dr. David Alan Gilbert / dgilbert at redhat.com / Manchester, UK