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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, May 21, 2020 at 05:42:41PM +0200, Lukas Straub wrote:
> On Thu, 21 May 2020 16:03:35 +0100
> Stefan Hajnoczi <stefanha@gmail.com> 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 :|