Re: [Xen-devel] [PATCH v2 3/9] libxl_internal: Introduce libxl__ev_lock for devices hotplug via QMP

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("[PATCH v2 3/9] libxl_internal: Introduce libxl__ev_lock 
for devices hotplug via QMP"):
> The current lock `domain_userdata_lock' can't be used when modification
> to a guest is done by sending command to QEMU, this is a slow process
> and requires to call CTX_UNLOCK, which is not possible while holding
> the `domain_userdata_lock'.
> 
> To resolve this issue, we create a new lock which can take over part
> of the job of the json_lock.

Thanks.  This is basically fine.  I have only trivial comments.

> +void libxl__ev_lock_get(libxl__egc *egc, libxl__ev_lock *lock)

I wonder if this is the right name for this.  Effectively you have
called this lock "lock".  Maybe "dlock" or "devlock" or "sdlock" (slow
device lock) or something ?  Sorry for bikeshedding but hopefully
seddery will be easy.

> +static void ev_lock_prepare_fork(libxl__egc *egc, libxl__ev_lock *lock)
> +{
...
> +                /* All other errno: EBADF, EINVAL, ENOLCK, EWOULDBLOCK */
> +                LOGED(ERROR, domid,
> +                      "unexpected error while trying to lock %s, fd=%d, 
> errno=%d",
> +                      lockfile, fd, errno);

LOGED prints strerror(errno) so you don't need to print the numeric
value with %d too.  That's what the E in its name is.

> +void libxl__ev_unlock(libxl__gc *gc, libxl__ev_lock *lock)
> +{
> +    int r;
> +
> +    assert(!libxl__ev_child_inuse(&lock->child));
> +
> +    /* It's important to unlink the file before releasing the lock to avoid
> +     * the following race (if unlock/close before unlink):
> +     *
> +     *   P1 LOCK                         P2 UNLOCK
> +     *   fd1 = open(lockfile)
> +     *                                   unlock(fd2)
> +     *   flock(fd1)
> +     *   fstat and stat check success
> +     *                                   unlink(lockfile)
> +     *   return lock
> +     *
> +     * In above case P1 thinks it has got hold of the lock but
> +     * actually lock is released by P2 (lockfile unlinked).
> +     */

I wonder if it would be better to refer to the other copy of this
comment by libxl__unlock_domain_userdata.

Regards,
Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel