Re: [PATCH v2 1/4] Introduce yank feature

[Lukas Straub]

On Thu, 21 May 2020 16:03:35 +0100
Stefan Hajnoczi <address@hidden> wrote:

> On Wed, May 20, 2020 at 11:05:39PM +0200, Lukas Straub wrote:
> > +void yank_generic_iochannel(void *opaque)
> > +{
> > +    QIOChannel *ioc = QIO_CHANNEL(opaque);
> > +
> > +    qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> > +}
> > +
> > +void qmp_yank(strList *instances, Error **errp)
> > +{
> > +    strList *tmp;
> > +    struct YankInstance *instance;
> > +    struct YankFuncAndParam *entry;
> > +
> > +    qemu_mutex_lock(&lock);
> > +    tmp = instances;
> > +    for (; tmp; tmp = tmp->next) {
> > +        instance = yank_find_instance(tmp->value);
> > +        if (!instance) {
> > +            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> > +                      "Instance '%s' not found", tmp->value);
> > +            qemu_mutex_unlock(&lock);
> > +            return;
> > +        }
> > +    }
> > +    tmp = instances;
> > +    for (; tmp; tmp = tmp->next) {
> > +        instance = yank_find_instance(tmp->value);
> > +        assert(instance);
> > +        QLIST_FOREACH(entry, &instance->yankfns, next) {
> > +            entry->func(entry->opaque);
> > +        }
> > +    }
> > +    qemu_mutex_unlock(&lock);
> > +}  
> 
> From docs/devel/qapi-code-gen.txt:
> 
>   An OOB-capable command handler must satisfy the following conditions:
> 
>   - It terminates quickly.
Check.

>   - It does not invoke system calls that may block.
brk/sbrk (malloc and friends):
The manpage doesn't say anything about blocking, but malloc is already used 
while handling the qmp command.

shutdown():
The manpage doesn't say anything about blocking, but this is already used in 
migration oob qmp commands.

There are no other syscalls involved to my knowledge.

>   - It does not access guest RAM that may block when userfaultfd is
>     enabled for postcopy live migration.
Check.

>   - 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.

The lock in yank.c satisfies this requirement.

qio_channel_shutdown doesn't take any locks.

Regards,
Lukas Straub

> This patch series violates these rules and calls existing functions that
> were not designed for OOB execution.
> 
> Please explain why it is safe to do this.
> 
> Stefan

pgpkNmo_studI.pgp
Description: OpenPGP digital signature