Re: [PATCH 2/2] monitor: Report EBADFD if fdset contains invalid FD

[Daniel P . Berrangé]

On Tue, Aug 17, 2021 at 01:59:22PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Aug 17, 2021 at 12:56 PM Michal Privoznik <mprivozn@redhat.com>
> wrote:
> 
> > When opening a path that starts with "/dev/fdset/" the control
> > jumps into qemu_parse_fdset() and then into
> > monitor_fdset_dup_fd_add(). In here, corresponding fdset is found
> > and then all FDs from the set are iterated over trying to find an
> > FD that matches expected access mode. For instance, if caller
> > wants O_WRONLY then the FD set has to contain an O_WRONLY FD.
> >
> > If no such FD is found then errno is set to EACCES which results
> > in very misleading error messages, for instance:
> >
> >   Could not dup FD for /dev/fdset/3 flags 441: Permission denied
> >
> > There is no permission issue, the problem is that there was no FD
> > within given fdset that was in expected access mode. Therefore,
> > let's set errno to EBADFD, which gives us somewhat better
> > error messages:
> >
> >   Could not dup FD for /dev/fdset/3 flags 441: File descriptor in bad state
> >
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >
> 
> I am not sure this is any better. If you try to open a read-only file, the
> system also reports EACCES (Permission denied). This is what the current
> code models, I believe.

If we want better error reporting quality for this method we ought
to just stop using errno and wire up a Error **errp parameter.

> 
> 
> > ---
> >  monitor/misc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/monitor/misc.c b/monitor/misc.c
> > index ffe7966870..a0eda0d574 100644
> > --- a/monitor/misc.c
> > +++ b/monitor/misc.c
> > @@ -1347,7 +1347,7 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int
> > flags)
> >          }
> >
> >          if (fd == -1) {
> > -            errno = EACCES;
> > +            errno = EBADFD;
> >              return -1;
> >          }
> >
> > --
> > 2.31.1
> >
> >

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|