Re: [PATCHv2 1/1] Support monitor chardev hotswap with QMP

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Fri, Apr 16, 2021 at 6:59 PM Marc-André Lureau <
> marcandre.lureau@gmail.com> wrote:
>
>> Hi
>>
>> On Fri, Apr 16, 2021 at 6:51 PM Markus Armbruster <armbru@redhat.com>
>> wrote:
>>
>>> Marc-André, I'd like your opinion for this one, in particular the use of
>>> g_source_remove().
>>>
>>
>> My opinion isn't really worth much, my review would have a bit more value.
>>
>> GSource has indeed some peculiar lifetime management, that I got wrong in
>> the past. So I would be extra careful.
>>
>> But before spending time on review, I would also clarify the motivation
>> and ask for testing.
>>
>> Markus, hot-adding/removing monitors isn't supported?
>>
>>
> I realize you answered my question below. That's surprising me. Wouldn't it
> make more sense to support it rather than having a pre-opened null-based
> monitor that can have its chardev swapped?

Yes, it would.  Patches welcome.

This patch is a somewhat ham-fisted and limited solution to the problem
stated in the commit message.  However, it might *also* be a reasonable
improvement to chardev-change on its own.  Not for me to judge.

chardev-change comes with a number of restrictions.  Let's have a closer
look.  It fails

1. when no such character device exists (d'oh)

2. for chardev-mux devices

3. in record/replay mode

4. when a backend is connected that doesn't implement the chr_be_change()
   method

5. when chr_be_change() fails

6. when creating the new chardev fails[*]

Items 2, 3, 4 are restrictions.  I figure 2 and 4 are simply not
implemented, yet.  I'm not sure about 3.

Whether we want to accept patches lifting restrictions is up to the
chardev maintainers.

This patch lifts restriction 4 for QMP monitor backends.  Its monitor
part looks acceptable to me, but I dislike its code duplication.  Before
we spend time on cleaning that up (or on deciding to clean it up later),
I'd like to hear the chardev mantainers' judgement, because that's about
more serious matters than cleanliness.

Do I make sense?

[...]


[*] The code for creating the new chardev in the "no backend connected"
case

    be = chr->be;
    if (!be) {
        /* easy case */
        object_unparent(OBJECT(chr));
        return qmp_chardev_add(id, backend, errp);
    }

is problematic: when qmp_chardev_add() fails, we already destroyed the
old chardev.  It should destroy the old chardev only when it can create
its replacement.