[PATCH v4 5/7] chardev/char: introduce .init() + .connect() initialization interface

Vladimir Sementsov-Ogievskiy posted 7 patches 4 weeks, 1 day ago
Maintainers: "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <eduardo@habkost.net>
There is a newer version of this series
[PATCH v4 5/7] chardev/char: introduce .init() + .connect() initialization interface
Posted by Vladimir Sementsov-Ogievskiy 4 weeks, 1 day ago
We'll need a possibility to postpone connect step to later point in
time to implement backend-transfer migration feature for vhost-user-blk
in further commits. Let's start with new char interface for backends.

.init() takes QAPI parameters and should parse them, called early

.connect() should actually establish a connection, and postponed to
the point of attaching to frontend. Called at later point, either
at time of attaching frontend, either from qemu_chr_wait_connected().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-fe.c      |  4 ++++
 chardev/char.c         | 39 +++++++++++++++++++++++++++++++++++++--
 include/chardev/char.h | 28 +++++++++++++++++++++++++++-
 3 files changed, 68 insertions(+), 3 deletions(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index 158a5f4f55..973fed5bea 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -193,6 +193,10 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
 {
     unsigned int tag = 0;
 
+    if (!qemu_chr_connect(s, errp)) {
+        return false;
+    }
+
     if (s) {
         if (CHARDEV_IS_MUX(s)) {
             MuxChardev *d = MUX_CHARDEV(s);
diff --git a/chardev/char.c b/chardev/char.c
index 27290e26fb..409f3aac1c 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -33,6 +33,7 @@
 #include "qapi/error.h"
 #include "qapi/qapi-commands-char.h"
 #include "qapi/qmp/qerror.h"
+#include "qom/object.h"
 #include "system/replay.h"
 #include "qemu/help_option.h"
 #include "qemu/module.h"
@@ -338,10 +339,29 @@ static bool qemu_chr_is_busy(Chardev *s)
     }
 }
 
+bool qemu_chr_connect(Chardev *chr, Error **errp)
+{
+    ChardevClass *cc = CHARDEV_GET_CLASS(chr);
+
+    if (chr->connect_postponed) {
+        assert(cc->connect);
+        chr->connect_postponed = false;
+        if (!cc->connect(chr, errp)) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 int qemu_chr_wait_connected(Chardev *chr, Error **errp)
 {
     ChardevClass *cc = CHARDEV_GET_CLASS(chr);
 
+    if (!qemu_chr_connect(chr, errp)) {
+        return -1;
+    }
+
     if (cc->chr_wait_connected) {
         return cc->chr_wait_connected(chr, errp);
     }
@@ -1030,6 +1050,7 @@ static Chardev *chardev_new(const char *id, const char *typename,
 {
     Object *obj;
     Chardev *chr = NULL;
+    ChardevClass *cc;
 
     assert(g_str_has_prefix(typename, "chardev-"));
     assert(id);
@@ -1044,8 +1065,22 @@ static Chardev *chardev_new(const char *id, const char *typename,
         goto fail;
     }
 
-    if (!qemu_char_open(chr, backend, typename + 8, errp)) {
-        goto fail;
+    cc = CHARDEV_GET_CLASS(chr);
+
+    if (cc->init) {
+        assert(!cc->open);
+        assert(cc->connect);
+
+        if (!cc->init(chr, backend, errp)) {
+            goto fail;
+        }
+        assert(chr->filename);
+
+        chr->connect_postponed = true;
+    } else {
+        if (!qemu_char_open(chr, backend, typename + 8, errp)) {
+            goto fail;
+        }
     }
 
     return chr;
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 429852f8d9..d2e01f0f9c 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -63,6 +63,7 @@ struct Chardev {
     CharBackend *be;
     char *label;
     char *filename;
+    bool connect_postponed;
     int logfd;
     int be_open;
     /* used to coordinate the chardev-change special-case: */
@@ -225,6 +226,7 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
                                 bool permit_mux_mon);
 int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all);
 #define qemu_chr_write_all(s, buf, len) qemu_chr_write(s, buf, len, true)
+bool qemu_chr_connect(Chardev *chr, Error **errp);
 int qemu_chr_wait_connected(Chardev *chr, Error **errp);
 
 #define TYPE_CHARDEV "chardev"
@@ -259,7 +261,31 @@ struct ChardevClass {
     /* parse command line options and populate QAPI @backend */
     void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
 
-    /* called after construction, open/starts the backend */
+    /*
+     * Called after construction, create the backend, mutually exclusive
+     * with @open, and should be followed by @connect().
+     * Must set the Chardev's chr->filename on success.
+     */
+    bool (*init)(Chardev *chr, ChardevBackend *backend,
+                 Error **errp);
+
+    /*
+     * Called after @init(), starts the backend, mutually exclusive
+     * with @open. Should care to send CHR_EVENT_OPENED when connected.
+     */
+    bool (*connect)(Chardev *chr, Error **errp);
+
+    /*
+     * Called after construction, an alternative to @init + @connect
+     * and should do the work for both: create and start the backend.
+     * Mutual exclusive with @init and @connect.
+     *
+     * May not set the Chardev's chr->filename (generic code will care),
+     * and may not send CHR_EVENT_OPENED when connected (@be_opened
+     * should not be touched in this case, to signal the generic code
+     * to care about CHR_EVENT_OPENED). If backend care about
+     * CHR_EVENT_OPENED, it should set @be_opened to false.
+     */
     void (*open)(Chardev *chr, ChardevBackend *backend,
                  bool *be_opened, Error **errp);
 
-- 
2.48.1


Re: [PATCH v4 5/7] chardev/char: introduce .init() + .connect() initialization interface
Posted by Markus Armbruster 4 weeks, 1 day ago
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> We'll need a possibility to postpone connect step to later point in
> time to implement backend-transfer migration feature for vhost-user-blk
> in further commits. Let's start with new char interface for backends.
>
> .init() takes QAPI parameters and should parse them, called early
>
> .connect() should actually establish a connection, and postponed to
> the point of attaching to frontend. Called at later point, either
> at time of attaching frontend, either from qemu_chr_wait_connected().

s/either/or/

>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

[...]

> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 429852f8d9..d2e01f0f9c 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -63,6 +63,7 @@ struct Chardev {
>      CharBackend *be;
>      char *label;
>      char *filename;
> +    bool connect_postponed;
>      int logfd;
>      int be_open;
>      /* used to coordinate the chardev-change special-case: */
> @@ -225,6 +226,7 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
>                                  bool permit_mux_mon);
>  int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all);
>  #define qemu_chr_write_all(s, buf, len) qemu_chr_write(s, buf, len, true)
> +bool qemu_chr_connect(Chardev *chr, Error **errp);
>  int qemu_chr_wait_connected(Chardev *chr, Error **errp);
>  
>  #define TYPE_CHARDEV "chardev"
> @@ -259,7 +261,31 @@ struct ChardevClass {
>      /* parse command line options and populate QAPI @backend */
>      void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
>  
> -    /* called after construction, open/starts the backend */
> +    /*
> +     * Called after construction, create the backend, mutually exclusive
> +     * with @open, and should be followed by @connect().
> +     * Must set the Chardev's chr->filename on success.
> +     */
> +    bool (*init)(Chardev *chr, ChardevBackend *backend,
> +                 Error **errp);
> +
> +    /*
> +     * Called after @init(), starts the backend, mutually exclusive
> +     * with @open. Should care to send CHR_EVENT_OPENED when connected.

Would "Must send CHR_EVENT_OPENED on success" be clearer?

> +     */
> +    bool (*connect)(Chardev *chr, Error **errp);
> +
> +    /*
> +     * Called after construction, an alternative to @init + @connect
> +     * and should do the work for both: create and start the backend.
> +     * Mutual exclusive with @init and @connect.

Mutually

> +     *
> +     * May not set the Chardev's chr->filename (generic code will care),
> +     * and may not send CHR_EVENT_OPENED when connected (@be_opened
> +     * should not be touched in this case, to signal the generic code
> +     * to care about CHR_EVENT_OPENED). If backend care about

If the backend cares

> +     * CHR_EVENT_OPENED, it should set @be_opened to false.
> +     */
>      void (*open)(Chardev *chr, ChardevBackend *backend,
>                   bool *be_opened, Error **errp);
Re: [PATCH v4 5/7] chardev/char: introduce .init() + .connect() initialization interface
Posted by Vladimir Sementsov-Ogievskiy 4 weeks, 1 day ago
On 16.10.25 09:20, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> We'll need a possibility to postpone connect step to later point in
>> time to implement backend-transfer migration feature for vhost-user-blk
>> in further commits. Let's start with new char interface for backends.
>>
>> .init() takes QAPI parameters and should parse them, called early
>>
>> .connect() should actually establish a connection, and postponed to
>> the point of attaching to frontend. Called at later point, either
>> at time of attaching frontend, either from qemu_chr_wait_connected().
> 
> s/either/or/
> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> [...]
> 
>> diff --git a/include/chardev/char.h b/include/chardev/char.h
>> index 429852f8d9..d2e01f0f9c 100644
>> --- a/include/chardev/char.h
>> +++ b/include/chardev/char.h
>> @@ -63,6 +63,7 @@ struct Chardev {
>>       CharBackend *be;
>>       char *label;
>>       char *filename;
>> +    bool connect_postponed;
>>       int logfd;
>>       int be_open;
>>       /* used to coordinate the chardev-change special-case: */
>> @@ -225,6 +226,7 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
>>                                   bool permit_mux_mon);
>>   int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all);
>>   #define qemu_chr_write_all(s, buf, len) qemu_chr_write(s, buf, len, true)
>> +bool qemu_chr_connect(Chardev *chr, Error **errp);
>>   int qemu_chr_wait_connected(Chardev *chr, Error **errp);
>>   
>>   #define TYPE_CHARDEV "chardev"
>> @@ -259,7 +261,31 @@ struct ChardevClass {
>>       /* parse command line options and populate QAPI @backend */
>>       void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
>>   
>> -    /* called after construction, open/starts the backend */
>> +    /*
>> +     * Called after construction, create the backend, mutually exclusive
>> +     * with @open, and should be followed by @connect().
>> +     * Must set the Chardev's chr->filename on success.
>> +     */
>> +    bool (*init)(Chardev *chr, ChardevBackend *backend,
>> +                 Error **errp);
>> +
>> +    /*
>> +     * Called after @init(), starts the backend, mutually exclusive
>> +     * with @open. Should care to send CHR_EVENT_OPENED when connected.
> 
> Would "Must send CHR_EVENT_OPENED on success" be clearer?

No, because it may start background connecting process instead, which
should finish with CHR_EVENT_OPENED.

> 
>> +     */
>> +    bool (*connect)(Chardev *chr, Error **errp);
>> +
>> +    /*
>> +     * Called after construction, an alternative to @init + @connect
>> +     * and should do the work for both: create and start the backend.
>> +     * Mutual exclusive with @init and @connect.
> 
> Mutually
> 
>> +     *
>> +     * May not set the Chardev's chr->filename (generic code will care),
>> +     * and may not send CHR_EVENT_OPENED when connected (@be_opened
>> +     * should not be touched in this case, to signal the generic code
>> +     * to care about CHR_EVENT_OPENED). If backend care about
> 
> If the backend cares
> 
>> +     * CHR_EVENT_OPENED, it should set @be_opened to false.
>> +     */
>>       void (*open)(Chardev *chr, ChardevBackend *backend,
>>                    bool *be_opened, Error **errp);
> 


-- 
Best regards,
Vladimir