* [Virtio-fs] [PATCH] virtiofsd: fix incorrect error handling in lo_do_lookup @ 2019-06-11 13:44 Eric Ren 2019-06-11 17:38 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 5+ messages in thread From: Eric Ren @ 2019-06-11 13:44 UTC (permalink / raw) To: virtio-fs 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) ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: fix incorrect error handling in lo_do_lookup 2019-06-11 13:44 [Virtio-fs] [PATCH] virtiofsd: fix incorrect error handling in lo_do_lookup Eric Ren @ 2019-06-11 17:38 ` Dr. David Alan Gilbert 2019-06-11 18:06 ` Liu Bo 0 siblings, 1 reply; 5+ messages in thread From: Dr. David Alan Gilbert @ 2019-06-11 17:38 UTC (permalink / raw) To: Eric Ren; +Cc: virtio-fs * 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: fix incorrect error handling in lo_do_lookup 2019-06-11 17:38 ` Dr. David Alan Gilbert @ 2019-06-11 18:06 ` Liu Bo 2019-06-11 18:43 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 5+ messages in thread From: Liu Bo @ 2019-06-11 18:06 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: virtio-fs 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: fix incorrect error handling in lo_do_lookup 2019-06-11 18:06 ` Liu Bo @ 2019-06-11 18:43 ` Dr. David Alan Gilbert 2019-06-11 19:11 ` Liu Bo 0 siblings, 1 reply; 5+ messages in thread From: Dr. David Alan Gilbert @ 2019-06-11 18:43 UTC (permalink / raw) To: Liu Bo; +Cc: virtio-fs * 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: fix incorrect error handling in lo_do_lookup 2019-06-11 18:43 ` Dr. David Alan Gilbert @ 2019-06-11 19:11 ` Liu Bo 0 siblings, 0 replies; 5+ messages in thread From: Liu Bo @ 2019-06-11 19:11 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: virtio-fs 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-11 19:11 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-06-11 13:44 [Virtio-fs] [PATCH] virtiofsd: fix incorrect error handling in lo_do_lookup Eric Ren 2019-06-11 17:38 ` Dr. David Alan Gilbert 2019-06-11 18:06 ` Liu Bo 2019-06-11 18:43 ` Dr. David Alan Gilbert 2019-06-11 19:11 ` Liu Bo
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.