From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Tue, 11 Jun 2019 19:43:16 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20190611184315.GK2777@work-vm> References: <20190611173833.GH2777@work-vm> <20190611180604.i6dkkhtafdr2zlax@US-160370MP2.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190611180604.i6dkkhtafdr2zlax@US-160370MP2.local> Subject: Re: [Virtio-fs] [PATCH] virtiofsd: fix incorrect error handling in lo_do_lookup List-Id: Development discussions about virtio-fs List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Liu Bo Cc: virtio-fs@redhat.com * 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 > > > --- > > > 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 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