[PATCH 0/2] Two chardev with fdset fixes

[PATCH 0/2] Two chardev with fdset fixes

[Michal Privoznik]

I was working on libvirt, trying to make it pass a pre-opened FD for a
chardev's logfile. I've spent non-negligible amount of time trying to
figure out why I was getting EACCESS error when QEMU definitely was able
to access the FD. Well, found the problem and here is my attempt to save
future me from figuring it out again.

NB, I'm not fully convinced that EBADFD is the best value, but anything
is better than EACCESS.

Michal Privoznik (2):
  chardev: Propagate error from logfile opening
  monitor: Report EBADFD if fdset contains invalid FD

 chardev/char.c | 7 ++-----
 monitor/misc.c | 2 +-
 2 files changed, 3 insertions(+), 6 deletions(-)

-- 
2.31.1

[PATCH 1/2] chardev: Propagate error from logfile opening

[Michal Privoznik]

If a chardev has a logfile the file is opened using
qemu_open_old() which does the job, but since @errp is not
propagated into qemu_open_internal() we lose much more accurate
error and just report "Unable to open logfile $errno".  When
using plain files, it's probably okay as nothing complex is
happening behind the curtains. But the problem becomes more
prominent when passing an "/dev/fdset/XXX" path since much more
needs to be done.

The fix is to use qemu_create() which passes @errp further down.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 chardev/char.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 4595a8d430..0169d8dde4 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -241,18 +241,15 @@ static void qemu_char_open(Chardev *chr, ChardevBackend *backend,
     ChardevCommon *common = backend ? backend->u.null.data : NULL;
 
     if (common && common->has_logfile) {
-        int flags = O_WRONLY | O_CREAT;
+        int flags = O_WRONLY;
         if (common->has_logappend &&
             common->logappend) {
             flags |= O_APPEND;
         } else {
             flags |= O_TRUNC;
         }
-        chr->logfd = qemu_open_old(common->logfile, flags, 0666);
+        chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
         if (chr->logfd < 0) {
-            error_setg_errno(errp, errno,
-                             "Unable to open logfile %s",
-                             common->logfile);
             return;
         }
     }
-- 
2.31.1

Re: [PATCH 1/2] chardev: Propagate error from logfile opening

[Philippe Mathieu-Daudé]

On 8/17/21 10:56 AM, Michal Privoznik wrote:
> If a chardev has a logfile the file is opened using
> qemu_open_old() which does the job, but since @errp is not
> propagated into qemu_open_internal() we lose much more accurate
> error and just report "Unable to open logfile $errno".  When
> using plain files, it's probably okay as nothing complex is
> happening behind the curtains. But the problem becomes more
> prominent when passing an "/dev/fdset/XXX" path since much more
> needs to be done.
> 
> The fix is to use qemu_create() which passes @errp further down.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  chardev/char.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 1/2] chardev: Propagate error from logfile opening

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]

On Tue, Aug 17, 2021 at 12:56 PM Michal Privoznik <mprivozn@redhat.com>
wrote:

> If a chardev has a logfile the file is opened using
> qemu_open_old() which does the job, but since @errp is not
> propagated into qemu_open_internal() we lose much more accurate
> error and just report "Unable to open logfile $errno".  When
> using plain files, it's probably okay as nothing complex is
> happening behind the curtains. But the problem becomes more
> prominent when passing an "/dev/fdset/XXX" path since much more
> needs to be done.
>
> The fix is to use qemu_create() which passes @errp further down.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  chardev/char.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 4595a8d430..0169d8dde4 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -241,18 +241,15 @@ static void qemu_char_open(Chardev *chr,
> ChardevBackend *backend,
>      ChardevCommon *common = backend ? backend->u.null.data : NULL;
>
>      if (common && common->has_logfile) {
> -        int flags = O_WRONLY | O_CREAT;
> +        int flags = O_WRONLY;
>          if (common->has_logappend &&
>              common->logappend) {
>              flags |= O_APPEND;
>          } else {
>              flags |= O_TRUNC;
>          }
> -        chr->logfd = qemu_open_old(common->logfile, flags, 0666);
> +        chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
>          if (chr->logfd < 0) {
> -            error_setg_errno(errp, errno,
> -                             "Unable to open logfile %s",
> -                             common->logfile);
>              return;
>          }
>      }
> --
> 2.31.1
>
>

[-- Attachment #2: Type: text/html, Size: 2730 bytes --]

Re: [PATCH 1/2] chardev: Propagate error from logfile opening

[Michal Prívozník]

On 8/17/21 11:54 AM, Marc-André Lureau wrote:
> On Tue, Aug 17, 2021 at 12:56 PM Michal Privoznik <mprivozn@redhat.com>
> wrote:
> 
>> If a chardev has a logfile the file is opened using
>> qemu_open_old() which does the job, but since @errp is not
>> propagated into qemu_open_internal() we lose much more accurate
>> error and just report "Unable to open logfile $errno".  When
>> using plain files, it's probably okay as nothing complex is
>> happening behind the curtains. But the problem becomes more
>> prominent when passing an "/dev/fdset/XXX" path since much more
>> needs to be done.
>>
>> The fix is to use qemu_create() which passes @errp further down.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks, can you add it to your pull queue please?

Michal

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

[Michal Privoznik]

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>
---
 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

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

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]

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.


> ---
>  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
>
>

[-- Attachment #2: Type: text/html, Size: 2343 bytes --]

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

[Michal Prívozník]

On 8/17/21 11:59 AM, 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.

Fair enough. Another idea I had was that if an FD that's O_RDWR was
passed but only read or only write access was requested then such FD
could be accepted because it is capable of reading/writing.

But since patch 1/2 was accepted then I guess 2/2 is not that much needed.

Michal

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 :|