We are going to share new chardev_init_logfd() with further
alternative initialization interface. Let qemu_char_open() be
a wrapper for .open(), and its artifacts (handle be_opened if
was not set to false by backend, and filename).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 chardev/char.c | 49 +++++++++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index a43b7e5481..d5a2533e8e 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -250,22 +250,6 @@ static void qemu_char_open(Chardev *chr, ChardevBackend *backend,
                            bool *be_opened, Error **errp)
 {
     ChardevClass *cc = CHARDEV_GET_CLASS(chr);
-    /* Any ChardevCommon member would work */
-    ChardevCommon *common = backend ? backend->u.null.data : NULL;
-
-    if (common && common->logfile) {
-        int flags = O_WRONLY;
-        if (common->has_logappend &&
-            common->logappend) {
-            flags |= O_APPEND;
-        } else {
-            flags |= O_TRUNC;
-        }
-        chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
-        if (chr->logfd < 0) {
-            return;
-        }
-    }
 
     if (cc->open) {
         cc->open(chr, backend, be_opened, errp);
@@ -1000,6 +984,28 @@ void qemu_chr_set_feature(Chardev *chr,
     return set_bit(feature, chr->features);
 }
 
+static bool chardev_init_logfd(Chardev *chr, ChardevBackend *backend,
+                                Error **errp)
+{
+    ChardevCommon *common = backend ? backend->u.null.data : NULL;
+
+    if (common && common->logfile) {
+        int flags = O_WRONLY;
+        if (common->has_logappend &&
+            common->logappend) {
+            flags |= O_APPEND;
+        } else {
+            flags |= O_TRUNC;
+        }
+        chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
+        if (chr->logfd < 0) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 static Chardev *chardev_new(const char *id, const char *typename,
                             ChardevBackend *backend,
                             GMainContext *gcontext,
@@ -1020,11 +1026,14 @@ static Chardev *chardev_new(const char *id, const char *typename,
     chr->label = g_strdup(id);
     chr->gcontext = gcontext;
 
+    if (!chardev_init_logfd(chr, backend, errp)) {
+        goto fail;
+    }
+
     qemu_char_open(chr, backend, &be_opened, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        object_unref(obj);
-        return NULL;
+        goto fail;
     }
 
     if (!chr->filename) {
@@ -1035,6 +1044,10 @@ static Chardev *chardev_new(const char *id, const char *typename,
     }
 
     return chr;
+
+fail:
+    object_unref(obj);
+    return NULL;
 }
 
 Chardev *qemu_chardev_new(const char *id, const char *typename,
-- 
2.48.1

On Mon, Oct 13, 2025 at 5:41 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> We are going to share new chardev_init_logfd() with further
> alternative initialization interface. Let qemu_char_open() be
> a wrapper for .open(), and its artifacts (handle be_opened if
> was not set to false by backend, and filename).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

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

> ---
>  chardev/char.c | 49 +++++++++++++++++++++++++++++++------------------
>  1 file changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index a43b7e5481..d5a2533e8e 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -250,22 +250,6 @@ static void qemu_char_open(Chardev *chr, ChardevBackend *backend,
>                             bool *be_opened, Error **errp)
>  {
>      ChardevClass *cc = CHARDEV_GET_CLASS(chr);
> -    /* Any ChardevCommon member would work */

maybe keep that comment?


> -    ChardevCommon *common = backend ? backend->u.null.data : NULL;
> -
> -    if (common && common->logfile) {
> -        int flags = O_WRONLY;
> -        if (common->has_logappend &&
> -            common->logappend) {
> -            flags |= O_APPEND;
> -        } else {
> -            flags |= O_TRUNC;
> -        }
> -        chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
> -        if (chr->logfd < 0) {
> -            return;
> -        }
> -    }
>
>      if (cc->open) {
>          cc->open(chr, backend, be_opened, errp);
> @@ -1000,6 +984,28 @@ void qemu_chr_set_feature(Chardev *chr,
>      return set_bit(feature, chr->features);
>  }
>
> +static bool chardev_init_logfd(Chardev *chr, ChardevBackend *backend,
> +                                Error **errp)
> +{
> +    ChardevCommon *common = backend ? backend->u.null.data : NULL;
> +
> +    if (common && common->logfile) {
> +        int flags = O_WRONLY;
> +        if (common->has_logappend &&
> +            common->logappend) {
> +            flags |= O_APPEND;
> +        } else {
> +            flags |= O_TRUNC;
> +        }
> +        chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
> +        if (chr->logfd < 0) {
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
>  static Chardev *chardev_new(const char *id, const char *typename,
>                              ChardevBackend *backend,
>                              GMainContext *gcontext,
> @@ -1020,11 +1026,14 @@ static Chardev *chardev_new(const char *id, const char *typename,
>      chr->label = g_strdup(id);
>      chr->gcontext = gcontext;
>
> +    if (!chardev_init_logfd(chr, backend, errp)) {
> +        goto fail;
> +    }
> +
>      qemu_char_open(chr, backend, &be_opened, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> -        object_unref(obj);
> -        return NULL;
> +        goto fail;
>      }
>
>      if (!chr->filename) {
> @@ -1035,6 +1044,10 @@ static Chardev *chardev_new(const char *id, const char *typename,
>      }
>
>      return chr;
> +
> +fail:
> +    object_unref(obj);
> +    return NULL;
>  }
>
>  Chardev *qemu_chardev_new(const char *id, const char *typename,
> --
> 2.48.1
>
>


--
Marc-André Lureau

On 14.10.25 10:30, Marc-André Lureau wrote:
> On Mon, Oct 13, 2025 at 5:41 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> We are going to share new chardev_init_logfd() with further
>> alternative initialization interface. Let qemu_char_open() be
>> a wrapper for .open(), and its artifacts (handle be_opened if
>> was not set to false by backend, and filename).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
>> ---
>>   chardev/char.c | 49 +++++++++++++++++++++++++++++++------------------
>>   1 file changed, 31 insertions(+), 18 deletions(-)
>>
>> diff --git a/chardev/char.c b/chardev/char.c
>> index a43b7e5481..d5a2533e8e 100644
>> --- a/chardev/char.c
>> +++ b/chardev/char.c
>> @@ -250,22 +250,6 @@ static void qemu_char_open(Chardev *chr, ChardevBackend *backend,
>>                              bool *be_opened, Error **errp)
>>   {
>>       ChardevClass *cc = CHARDEV_GET_CLASS(chr);
>> -    /* Any ChardevCommon member would work */
> 
> maybe keep that comment?

I a bit don't follow it, honestly.. What it mean? That we should
handle members of common structure here?

Not a problem to put it back to chardev_init_logfd().. But maybe, it
then should be renamed to chardev_init_common()

> 
> 
>> -    ChardevCommon *common = backend ? backend->u.null.data : NULL;
>> -
>> -    if (common && common->logfile) {
>> -        int flags = O_WRONLY;
>> -        if (common->has_logappend &&
>> -            common->logappend) {
>> -            flags |= O_APPEND;
>> -        } else {
>> -            flags |= O_TRUNC;
>> -        }
>> -        chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
>> -        if (chr->logfd < 0) {
>> -            return;
>> -        }
>> -    }
>>
>>       if (cc->open) {
>>           cc->open(chr, backend, be_opened, errp);
>> @@ -1000,6 +984,28 @@ void qemu_chr_set_feature(Chardev *chr,
>>       return set_bit(feature, chr->features);
>>   }
>>
>> +static bool chardev_init_logfd(Chardev *chr, ChardevBackend *backend,
>> +                                Error **errp)
>> +{
>> +    ChardevCommon *common = backend ? backend->u.null.data : NULL;
>> +
>> +    if (common && common->logfile) {
>> +        int flags = O_WRONLY;
>> +        if (common->has_logappend &&
>> +            common->logappend) {
>> +            flags |= O_APPEND;
>> +        } else {
>> +            flags |= O_TRUNC;
>> +        }
>> +        chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
>> +        if (chr->logfd < 0) {
>> +            return false;
>> +        }
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>   static Chardev *chardev_new(const char *id, const char *typename,
>>                               ChardevBackend *backend,
>>                               GMainContext *gcontext,
>> @@ -1020,11 +1026,14 @@ static Chardev *chardev_new(const char *id, const char *typename,
>>       chr->label = g_strdup(id);
>>       chr->gcontext = gcontext;
>>
>> +    if (!chardev_init_logfd(chr, backend, errp)) {
>> +        goto fail;
>> +    }
>> +
>>       qemu_char_open(chr, backend, &be_opened, &local_err);
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>> -        object_unref(obj);
>> -        return NULL;
>> +        goto fail;
>>       }
>>
>>       if (!chr->filename) {
>> @@ -1035,6 +1044,10 @@ static Chardev *chardev_new(const char *id, const char *typename,
>>       }
>>
>>       return chr;
>> +
>> +fail:
>> +    object_unref(obj);
>> +    return NULL;
>>   }
>>
>>   Chardev *qemu_chardev_new(const char *id, const char *typename,
>> --
>> 2.48.1
>>
>>
> 
> 
> --
> Marc-André Lureau


-- 
Best regards,
Vladimir

Hi

On Tue, Oct 14, 2025 at 1:21 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> On 14.10.25 10:30, Marc-André Lureau wrote:
> > On Mon, Oct 13, 2025 at 5:41 PM Vladimir Sementsov-Ogievskiy
> > <vsementsov@yandex-team.ru> wrote:
> >>
> >> We are going to share new chardev_init_logfd() with further
> >> alternative initialization interface. Let qemu_char_open() be
> >> a wrapper for .open(), and its artifacts (handle be_opened if
> >> was not set to false by backend, and filename).
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> >> ---
> >>   chardev/char.c | 49 +++++++++++++++++++++++++++++++------------------
> >>   1 file changed, 31 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/chardev/char.c b/chardev/char.c
> >> index a43b7e5481..d5a2533e8e 100644
> >> --- a/chardev/char.c
> >> +++ b/chardev/char.c
> >> @@ -250,22 +250,6 @@ static void qemu_char_open(Chardev *chr, ChardevBackend *backend,
> >>                              bool *be_opened, Error **errp)
> >>   {
> >>       ChardevClass *cc = CHARDEV_GET_CLASS(chr);
> >> -    /* Any ChardevCommon member would work */
> >
> > maybe keep that comment?
>
> I a bit don't follow it, honestly.. What it mean? That we should
> handle members of common structure here?

The ChardevBackend type is a union, and all the members inherit from
ChardevCommon. Ideally, we wouldn't cast this way, but that would
require some changes in code generator...

>
> Not a problem to put it back to chardev_init_logfd().. But maybe, it
> then should be renamed to chardev_init_common()
>
> >
> >
> >> -    ChardevCommon *common = backend ? backend->u.null.data : NULL;
> >> -
> >> -    if (common && common->logfile) {
> >> -        int flags = O_WRONLY;
> >> -        if (common->has_logappend &&
> >> -            common->logappend) {
> >> -            flags |= O_APPEND;
> >> -        } else {
> >> -            flags |= O_TRUNC;
> >> -        }
> >> -        chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
> >> -        if (chr->logfd < 0) {
> >> -            return;
> >> -        }
> >> -    }
> >>
> >>       if (cc->open) {
> >>           cc->open(chr, backend, be_opened, errp);
> >> @@ -1000,6 +984,28 @@ void qemu_chr_set_feature(Chardev *chr,
> >>       return set_bit(feature, chr->features);
> >>   }
> >>
> >> +static bool chardev_init_logfd(Chardev *chr, ChardevBackend *backend,
> >> +                                Error **errp)
> >> +{
> >> +    ChardevCommon *common = backend ? backend->u.null.data : NULL;
> >> +
> >> +    if (common && common->logfile) {
> >> +        int flags = O_WRONLY;
> >> +        if (common->has_logappend &&
> >> +            common->logappend) {
> >> +            flags |= O_APPEND;
> >> +        } else {
> >> +            flags |= O_TRUNC;
> >> +        }
> >> +        chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
> >> +        if (chr->logfd < 0) {
> >> +            return false;
> >> +        }
> >> +    }
> >> +
> >> +    return true;
> >> +}
> >> +
> >>   static Chardev *chardev_new(const char *id, const char *typename,
> >>                               ChardevBackend *backend,
> >>                               GMainContext *gcontext,
> >> @@ -1020,11 +1026,14 @@ static Chardev *chardev_new(const char *id, const char *typename,
> >>       chr->label = g_strdup(id);
> >>       chr->gcontext = gcontext;
> >>
> >> +    if (!chardev_init_logfd(chr, backend, errp)) {
> >> +        goto fail;
> >> +    }
> >> +
> >>       qemu_char_open(chr, backend, &be_opened, &local_err);
> >>       if (local_err) {
> >>           error_propagate(errp, local_err);
> >> -        object_unref(obj);
> >> -        return NULL;
> >> +        goto fail;
> >>       }
> >>
> >>       if (!chr->filename) {
> >> @@ -1035,6 +1044,10 @@ static Chardev *chardev_new(const char *id, const char *typename,
> >>       }
> >>
> >>       return chr;
> >> +
> >> +fail:
> >> +    object_unref(obj);
> >> +    return NULL;
> >>   }
> >>
> >>   Chardev *qemu_chardev_new(const char *id, const char *typename,
> >> --
> >> 2.48.1
> >>
> >>
> >
> >
> > --
> > Marc-André Lureau
>
>
> --
> Best regards,
> Vladimir



-- 
Marc-André Lureau