Re: [Xen-devel] [PATCH 6/9] libxl_disk: Cut libxl_cdrom_insert into steps ..

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("[PATCH 6/9] libxl_disk: Cut libxl_cdrom_insert into 
steps .."):
> This patch cut libxl_cdrom_insert into different step/function but there
> are still called synchronously. A later patch will call them
> asynchronously when QMP is involved.
> 
> The json_lock has been replaced by the qmp_lock for protection against
> concurrent changes to the cdrom. The json_lock is now only used when
> reading/modifying the domain userdata.


Your documentation for the qmp lock, taken roughly from my idea, is
that the qmp lock covers modifying things via qmp.  But I think here
you use it for updates via xenstore ?

I think this is OK because what matters is that any one thing is
always covered by the same lock - and here the cdrom is is a "thing".
But I think this means the commentary is wrong ?


> -    /* Note: CTX lock is already held at this point so lock hierarchy
> -     * is maintained.
> -     */
> -    lock = libxl__lock_domain_userdata(gc, domid);
> -    if (!lock) {
> +    cis->qmp_lock = libxl__lock_domain_qmp(gc, domid);

I think this is in fact precisely backwards.  The lock hierarchy is
*violated* here.  The qmp lock is supposed to be outside the ctx lock.

There will have to be an entrypoint for taking the qmp lock which
takes a gc and which makes a callback later.


AFAICT apart from these two aspects the rest of this code
reorganisation is good.

Thanks,
Ian.

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