From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roger Pau Monne Subject: Re: [PATCH v9 08/17] libxl: convert libxl_device_disk_add to an async op Date: Thu, 19 Jul 2012 17:14:32 +0100 Message-ID: <50083268.5020005@citrix.com> References: <1342172696-88347-1-git-send-email-roger.pau@citrix.com> <1342172696-88347-9-git-send-email-roger.pau@citrix.com> <20488.11968.333845.24701@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20488.11968.333845.24701@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: "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org Ian Jackson wrote: > Roger Pau Monne writes ("[PATCH v9 08/17] libxl: convert libxl_device_disk_add to an async op"): >> This patch converts libxl_device_disk_add to an ao operation that >> waits for device backend to reach state XenbusStateInitWait and then >> marks the operation as completed. This is not really useful now, but >> will be used by later patches that will launch hotplug scripts after >> we reached the desired xenbus state. > > This looks pretty good. I have just a couple of questions: > >> +/* Waits for the passed device to reach state XenbusStateInitWait. >> + * This is not really useful by itself, but is important when executing >> + * hotplug scripts, since we need to be sure the device is in the correct >> + * state before executing them. >> + * >> + * Once finished, aodev->callback will be executed. >> + */ >> +_hidden void libxl__wait_device_connection(libxl__egc*, >> + libxl__ao_device *aodev); > > This comment seems to be wrong ? We set the callback > to device_backend_callback but perhaps that calls aodev->callback ? Yes, device_backend_callback is the callback for the devstate event, but not the callback for the global aodev operation, which is stored in aodev->callback. I think the caller doesn't need to know that internally we set another callback, for the devstate event, since what matters is that aodev->callback will be called when the operation is finished. > It's hard for me to see the context without applying all of these > patches; could you perhaps provide a public git ref too next time ? > >> @@ -1808,6 +1829,8 @@ struct libxl__ao_devices { >> * The libxl__ao_device passed to this function should be >> * prepared using libxl__prepare_ao_device prior to calling >> * this function. >> + * >> + * Once finished, aodev->callback will be executed. >> */ >> _hidden void libxl__initiate_device_remove(libxl__egc *egc, >> libxl__ao_device *aodev); >> @@ -2162,6 +2185,19 @@ _hidden void libxl__destroy_domid(libxl__egc *egc, > > Is this hunk in the wrong patch ? Yes, it is. > Ian.