Lukas Straub <lukasstra...@web.de> writes:
> On Thu, 27 Aug 2020 14:37:00 +0200
> Markus Armbruster <arm...@redhat.com> wrote:
>
>> I apologize for not reviewing this much earlier.
>>
>> Lukas Straub <lukasstra...@web.de> writes:
>>
>> > The yank feature allows to recover from hanging qemu by "yanking"
>> > at various parts. Other qemu systems can register themselves and
>> > multiple yank functions. Then all yank functions for selected
>> > instances can be called by the 'yank' out-of-band qmp command.
>> > Available instances can be queried by a 'query-yank' oob command.
>> >
>> > Signed-off-by: Lukas Straub <lukasstra...@web.de>
>> > Acked-by: Stefan Hajnoczi <stefa...@redhat.com>
[...]
>> > diff --git a/qapi/misc.json b/qapi/misc.json
>> > index 9d32820dc1..0d6a8f20b7 100644
>> > --- a/qapi/misc.json
>> > +++ b/qapi/misc.json
>> > @@ -1615,3 +1615,48 @@
>> > ##
>> > { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
>> >
>> > +##
>> > +# @YankInstances:
>> > +#
>> > +# @instances: List of yank instances.
>> > +#
>> > +# Yank instances are named after the following schema:
>> > +# "blockdev:<node-name>", "chardev:<chardev-name>" and "migration"
>> > +#
>> > +# Since: 5.1
>> > +##
>> > +{ 'struct': 'YankInstances', 'data': {'instances': ['str'] } }
>>
>> I'm afraid this is a problematic QMP interface.
>>
>> By making YankInstances a struct, you keep the door open to adding more
>> members, which is good.
>>
>> But by making its 'instances' member a ['str'], you close the door to
>> using anything but a single string for the individual instances. Not so
>> good.
>>
>> The single string encodes information which QMP client will need to
>> parse from the string. We frown on that in QMP. Use QAPI complex types
>> capabilities for structured data.
>>
>> Could you use something like this instead?
>>
>> { 'enum': 'YankInstanceType',
>> 'data': { 'block-node', 'chardev', 'migration' } }
>>
>> { 'struct': 'YankInstanceBlockNode',
>> 'data': { 'node-name': 'str' } }
>>
>> { 'struct': 'YankInstanceChardev',
>> 'data' { 'label': 'str' } }
>>
>> { 'union': 'YankInstance',
>> 'base': { 'type': 'YankInstanceType' },
>> 'discriminator': 'type',
>> 'data': {
>> 'block-node': 'YankInstanceBlockNode',
>> 'chardev': 'YankInstanceChardev' } }
>>
>> { 'command': 'yank',
>> 'data': { 'instances': ['YankInstance'] },
>> 'allow-oob': true }
>>
>> If you're confident nothing will ever be added to YankInstanceBlockNode
>> and YankInstanceChardev, you could use str instead.
>
> As Daniel said, this has already been discussed.
I'll look up that discussion.
[...]
>> The two QMP commands permit out-of-band execution ('allow-oob': true).
>> OOB is easy to get wrong, but I figure you have a legitimate use case.
>> Let's review the restrictions documented in
>> docs/devel/qapi-code-gen.txt:
>>
>> An OOB-capable command handler must satisfy the following conditions:
>>
>> - It terminates quickly.
>> - It does not invoke system calls that may block.
>> - It does not access guest RAM that may block when userfaultfd is
>> enabled for postcopy live migration.
>> - It takes only "fast" locks, i.e. all critical sections protected by
>> any lock it takes also satisfy the conditions for OOB command
>> handler code.
>>
>> Since the command handlers take &lock, the restrictions apply to the
>> other critical sections protected by &lock as well. I believe these are
>> all okay: they do nothing but allocate, initialize and free memory.
>>
>> The restrictions also apply to the YankFn callbacks, but you documented
>> that. Okay.
>>
>> The one such callback included in this patch is
>> yank_generic_iochannel(), which is a thin wrapper around
>> qio_channel_shutdown(), which in turn runs the io_shutdown method.
>> Thus, the restructions also apply to all the io_shutdown methods.
>> That's not documented.
>>
>> Daniel, should it be documented?
>>
> This is already done in patch 6.
Patch 6 adds "This function is thread-safe" to its contract. The
restrictions on OOB-capable handler code are much more severe than
ordinary thread safety. For instance, blocking system calls outside
critical sections are thread safe, but not permitted in OOB-capable
handler code. The contract needs to be more specific.
> Thank you for you review.
Better late than never... you're welcome!
