Two chardev with fdset fixes

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

Posted by Michal Privoznik 4 years, 5 months ago

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

Posted by Marc-André Lureau 4 years, 5 months ago

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

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

Posted by Michal Prívozník 4 years, 5 months ago

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

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

Posted by Philippe Mathieu-Daudé 4 years, 5 months ago

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>