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

[Daniel P . Berrangé]

On Thu, May 21, 2020 at 05:42:41PM +0200, Lukas Straub wrote:
> 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.

It just marks the socket state in local kernel side. It doesn't involve
any blocking roundtrips over the wire, so this is fine.

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

Agreed, I think the yank code is compliant with all the points
listed above.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|