From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roger Pau Monne Subject: Re: [PATCH v6 05/13] libxl: convert libxl__device_disk_local_attach to an async op Date: Tue, 26 Jun 2012 11:27:14 +0100 Message-ID: <4FE98E82.9060801@citrix.com> References: <1339676475-33265-1-git-send-email-roger.pau@citrix.com> <1339676475-33265-6-git-send-email-roger.pau@citrix.com> <20451.24777.528406.191318@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20451.24777.528406.191318@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: Stefano Stabellini , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org Ian Jackson wrote: > Roger Pau Monne writes ("[PATCH v6 05/13] libxl: convert libxl__device_disk_local_attach to an async op"): >> This will be needed in future patches, when libxl__device_disk_add >> becomes async also. Create a new status structure that defines the >> local attach of a disk device and use it in >> libxl__device_disk_local_attach. > > This is looking good. I have a couple of comments: Thanks for the review! > >> diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c >> index 7ebc0df..182a975 100644 >> --- a/tools/libxl/libxl_bootloader.c >> +++ b/tools/libxl/libxl_bootloader.c > ... >> +static void bootloader_fisnihed_cb(libxl__egc *egc, > ^^^^^^^^ > `finished'. You have apparently managed to misspell this everywhere > you mention it! Probably copy-pasted it wrong everywhere, fixed. >> + libxl__disk_local_state *dls, >> + int rc); >> + >> static void bootloader_callback(libxl__egc *egc, libxl__bootloader_state *bl, >> int rc) >> { >> bootloader_cleanup(egc, bl); >> + >> + if (bl->diskpath) { >> + bl->dls.callback = bootloader_fisnihed_cb; >> + libxl__device_disk_local_detach(egc,&bl->dls); >> + return; >> + } > > Can we make libxl__device_disk_local_detach idempotent (and perhaps > call it `libxl__device_disk_local_attached_initiate_cleanup' or > something, if you can think of a name that's less than a paragraph) ? I'm not sure the "attached" in the name is correct, because the device might have failed to attach, and still we are going to call this function. How about "libxl__device_disk_local_initiate_detach"? (still quite long). Then the attach function should be renamed to libxl__device_disk_local_initiate_attach. > If so then this would be simpler and wouldn't need to test > bl->diskpath. Ok, I've moved the test for bl->dls.diskpath to libxl__device_disk_local_detach, so we have a more linear flow of callbacks. > >> +static void bootloader_disk_attached_cb(libxl__egc *egc, >> + libxl__disk_local_state *dls, >> + int rc) > ... >> + bl->diskpath = bl->dls.diskpath; > > Now that we have a bl->dls.diskpath, surely we can do away with > bl->diskpath ? I'm not a fan of having multiple variables with the > same information in them, unless it's essential. It just leads to > confusion and error. Done. >> +/* >> + * Make a disk available in this (the control) domain. Calls dls->callback >> + * when finished. >> + */ >> +_hidden void libxl__device_disk_local_attach(libxl__egc *egc, >> + libxl__disk_local_state *dls); >> +_hidden void libxl__device_disk_local_detach(libxl__egc *egc, >> + libxl__disk_local_state *dls); > > You really need to explain the rules for one of these dls's. > > Is it something like this: > > * A libxl__disk_local_state may be in the following states: > * Undefined, Idle, Attaching, Attached, Detaching Ok, I've added the init function and the description of what this functions do, together with the rename it should be a little bit more clear. > typedef void libxl__disk_local_state_callback(libxl__egc*, > libxl__disk_local_state*, > int rc); > > _hidden void libxl__device_disk_local_attach(libxl__egc *egc, > libxl__disk_local_state *dls); > /* Undefined/Idle -> Attaching. Will call callback. > * On entry to callback, if rc!=0 dls is Idle; > * if rc==0 it is Attached and dls->diskpath is usable. */ > > _hidden void libxl__device_disk_local_detach(libxl__egc *egc, > libxl__disk_local_state *dls); > /* Attached -> Detaching. Will call callback. > * On entry to callback, dls is Idle, regardless of > * success or failure. */ > > And my suggestion about idempotency would change this latter comment > to: > /* Idle/Attached -> Detaching. Will call callback. > * On entry to callback, dls is Idle, regardless of > * success or failure. */ > > And the bootloader code might want this: > > _hidden void libxl__device_disk_local_init(libxl__disk_local_state *dls); > /* Undefined/Idle -> Idle */ > > Or you can explain it in prose if you like. > > Ian.