From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roger Pau Monne Subject: Re: [PATCH v5 04/10] libxl: convert libxl_device_disk_add to an asyn op Date: Thu, 7 Jun 2012 18:55:31 +0200 Message-ID: <4FD0DD03.2020704@citrix.com> References: <1338383275-21315-1-git-send-email-roger.pau@citrix.com> <1338383275-21315-5-git-send-email-roger.pau@citrix.com> <20432.47604.849028.717366@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: <20432.47604.849028.717366@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 v5 04/10] libxl: convert libxl_device_disk_add to an asyn 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 latter patches that will launch hotplug scripts after >> we reached the desired xenbus state. > > And: > >> +/* Internal AO operation to connect a disk device, called by >> + * libxl_device_disk_add and libxl__add_disks. This function calls >> + * libxl__initiate_device_add */ >> +_hidden void libxl__device_disk_add(libxl__egc *egc, uint32_t domid, >> + libxl_device_disk *disk, >> + libxl__ao_device *aodev); >> + >> +/* Arranges that dev will be added to the guest, and the >> + * hotplug scripts will be executed (if necessary). When >> + * this is done (or an error happens), the callback in >> + * aodev->callback will be called. >> + */ >> +_hidden void libxl__initiate_device_add(libxl__egc*, libxl__ao_device *aodev); > > I don't think these comments are coherent. > > I think a function which "Arranges that dev will be added to the > guest" is one which does everything necessary - ie, the outermost > entrypoint. > > And you're using `internal' here to mean `internal to libxl'. I think > that's clear from the name (which has `__'). Whereas on first reading > it sounds like you mean `internal to the device adding machinery' - > but `libxl__device_disk_add' is the main entrypoint to that > machinery. I've used 'internal' here to reflex the difference between libxl_device_disk_add and libxl__device_disk_add, but probably leads to confusion. > And `libxl__initiate_device_add' is the internal helper function. > > I think these comments need to be clarified and perhaps > libxl__initiate_device_add needs to be renamed to be clear about what > exactly it is for. Eg > /* Initiates the device-kind-independent operations blah blah > hidden void libxl__initiate_device_add_core(libxl__egc*, > libxl__ao_device *aodev); > or something. > > Please do reply suggesting alternative wording > > Ian. Well the order is the following: libxl__device_{disk/nic}_add (device type dependant function) -> libxl__initiate_device_add (device type independent). I'm really bad about this naming thing. But the fact is that libxl__initiate_device_add is not a good name, since when we call this function the device is already added (to xenstore), this merely waits for it to reach the desired state and performs the necessary actions to "add" it to the guest (call hotplug scripts). Maybe "connect" is a more appropriate name than "add". I liked the nomenclature because it follows the same style as libxl__initiate_device_remove, and I think (and I might be wrong) it makes things easier to understand.