Re: [Xen-devel] [PATCH v4 31/32] libxl_disk: Cut libxl_cdrom_insert into step

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Aug 07, 2018 at 03:40:17PM +0100, Anthony PERARD wrote:
> On Tue, Aug 07, 2018 at 04:18:21PM +0200, Roger Pau Monné wrote:
> > On Mon, Aug 06, 2018 at 06:20:50PM +0100, Anthony PERARD wrote:
> > > On Thu, Aug 02, 2018 at 05:50:52PM +0200, Roger Pau Monné wrote:
> > > > On Fri, Jul 27, 2018 at 03:06:13PM +0100, Anthony PERARD wrote:
> > > > Can you provide a comment explaining how is this supposed to work? The
> > > > current code is already quite convoluted IMO (maybe because I'm not
> > > > familiar with it), so I think a comment would help reviewers.
> > > 
> > > Have you read the CODING_STYLE file? I think you'll find that the
> > > section "ASYNCHRONOUS/LONG-RUNNING OPERATIONS" will give you some answer
> > > you are looking for. I'm not sure I'm following it perfectly, but at
> > > least partially.
> > 
> > Oh, I didn't mean comments about the async operations, but I would add
> > a note to clarify that you first empty the CDROM and then you attach
> > the specified image.
> 
> I don't have better comments to provide than what's is already there. I
> don't know what all the xenstore stuff is about, if it can be removed or
> not. If the function was only to change media with qemu upstream almost
> none of those stuff is usefull, only one qmp command is enough. There is
> no need to insert an empty media before calling a command that would
> simply swap medias.
> 
> The patch is awful, but the mechanic that create the patch isn't too
> complicated, I simply cut the function after each qmp calls, use a
> struct to store the variables instead of local variable, and sometime
> initialise them later. There is also cdrom_insert_done() that can be
> call at anytime to do the cleanup and stop the AO, but that just move
> what was in out: before.
> 
> I don't think this patch can be review as a patch. I think one would
> need to look at what was before, and what is after, side-by-side, in
> order to have a good picture.
> 
> Something else to look after, can I keep a lock from
> libxl__lock_domain_userdata() across the different step of an ao?

Probably not.  AO_INPROGRESS and others unlock ctx.  The locking
hierarchy mandates you have to have ctx lock before you can have
userdata lock.

Wei.

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