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

[Anthony PERARD <anthony.perard@citrix.com>]

On Tue, Sep 17, 2019 at 04:44:30PM +0100, Ian Jackson wrote:
> 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.

"devlock" sounds fine. So we'll have "libxl__ev_devlock"
and "libxl__ev_devlock_get".

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

Yes, simple copy-paste error, I'll remove the errno value.

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

It would be probably fine. If the comment gets removed or the function
gets renamed, one can `git blame` to figure out what the reference is
for.

I'll replace the comment by this new one:
    /* See the rationale in libxl__unlock_domain_userdata()
     * about why we do unlink() before unlock(). */

Thanks,

-- 
Anthony PERARD

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