On Wed, 17 Jun 2020 15:39:36 +0100 Stefan Hajnoczi <stefa...@gmail.com> wrote:
> On Thu, May 21, 2020 at 04:48:06PM +0100, Daniel P. Berrangé wrote:
> > On Thu, May 21, 2020 at 05:42:41PM +0200, Lukas Straub wrote:
> > > On Thu, 21 May 2020 16:03:35 +0100
> > > Stefan Hajnoczi <stefa...@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.
>
> The code does not document this in any way. In fact, it's an interface
> for registering arbitrary callback functions which execute in this
> special environment.
>
> If you don't document this explicitly it will break when someone changes
> the code, even if it works right now.
>
> Please document this in the yank APIs and also in the implementation.
Hi,
It is documented in yank.h:
/**
* yank_register_function: Register a yank function
*
* This registers a yank function. All limitations of qmp oob commands apply
* to the yank function as well.
*
* This function is thread-safe.
*
* @instance_name: The name of the instance
* @func: The yank function
* @opaque: Will be passed to the yank function
*/
Thanks,
Lukas Straub
> For example, QemuMutex yank has the priority inversion problem: no other
> function may violate the oob rules while holding the mutex, otherwise
> the oob function will hang while waiting for the lock when the other
> function is blocked.
>
> Stefan
pgpTmMFN75mew.pgp
Description: OpenPGP digital signature
