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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Aug 21, 2018 at 10:08:49AM +0100, Wei Liu wrote:
> 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.

Ok, thanks. I'll attempt to fix that in v5.

-- 
Anthony PERARD

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