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

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 15.10.25 09:50, 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().
>>>
>>> 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..ebadaf3482 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,10 +261,28 @@ 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 and open/starts the backend,
>>
>> What to do mean by "create and open/starts"?
>> Maybe "create and start"?
>
> Yes, sounds good.
>
>>> +     * mutual exclusive with .init. .connect must not be defined when
>>
>> mutually
>> 
>>> +     * .open is defined.
>>> +     */
>>
>> Suggest to use @name to refer to a member name.  We do that elsewhere,
>> and it's easier on the eyes than dots.
>> 
>>>      void (*open)(Chardev *chr, ChardevBackend *backend,
>>>                   bool *be_opened, Error **errp);
>>> +    /*
>>> +     * Called after construction, create the backend, mutual exclusive
>>
>> mutually
>> 
>>> +     * with .open, and must be accompanied by .connect.
>>
>> Is it okay to destroy after init() without connect()?
>> If yes, "must" is misleading.
>
> Hmm. "should" is OK then?

"Should be followed by connect()" would work.

Or something like "The backend still needs to be started with init()."

>>> +     * Must set chr-filename.
>>
>> What's chr-filename?
>
> Type. That should be chr->filename. Or better @chr->filename ?

There is no member @chr.  I figure it's CharBackend member @chr.
Perhaps "the CharBackend's chr->filename". 

[...]

> Thanks for reviewing!

You're welcome!