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

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

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?

Thanks,

-- 
Anthony PERARD

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