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 19:07:40 +0200 Message-ID: <4FD0DFDC.4020602@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> <4FD0DD03.2020704@citrix.com> <20432.57154.896582.606950@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.57154.896582.606950@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 ("Re: [PATCH v5 04/10] libxl: convert libxl_device_disk_add to an asyn op"): >> Ian Jackson wrote: >>> 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. > > Yes. > >> Well the order is the following: >> >> libxl__device_{disk/nic}_add (device type dependant function) -> >> libxl__initiate_device_add (device type independent). > > Yes. > >> 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". > > Perhaps so. > >> 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. > > But libxl__initiate_device_remove _is_ the function which actually > initiates a device removal. It isn't an internal helper for a > family of libxl__device_FOO_remove functions. > > If it were symmetrical, libxl__initate_device_add would be the > single entrypoint and it would call something kind-specific as a helper. > That's what the comments and names seem to suggest to me, apart from > the explicit statement that it's the other way around. > > Hence my feeling that it needs to be clarified and perhaps this > function renamed. What about libxl__initiate_device_connection? and thanks for reviewing the code :).