Re: [PATCH v2 11/18] libxl: synchronise configuration when we hotplug a device

[Ian Campbell <Ian.Campbell@citrix.com>]

On Thu, 2014-08-28 at 12:18 +0100, Wei Liu wrote:
> On Wed, Aug 27, 2014 at 03:00:39AM +0100, Ian Campbell wrote:
> > On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > > As those routines are called both during domain creation and device
> > > hotplug, we add a flag to indicate whether we need to update JSON
> > > config. This flag is only set to true when we hotplug a device. We
> > > cannot update JSON config during domain creation as JSON config is
> > > committed to disk only when domain creation finishes.
> > 
> > Rather than carry a flag around did you consider just checking for the
> > presence of the file? I think you indicated in an earlier patch that you
> > were going to treat lack of the file as meaning creation/destruction was
> > happening.
> > 
> 
> I think a flag is more explict.
> 
> These libxl__device_*_add functions are called under two circumstances,
> a) domain creation, b) device hotplug.
> 
> In b) if the file is not present it could also be in abnormal state --
> file is deleted by accident, file system error etc. Carrying a flag can
> help up distinguish normal and abnormal state. (Though I can see my code
> currently lacks checking this at the moment)

OK.

> > 
> > > + * They take 6 parameters:
> > > + *  type:    the type of the device, say nic, vtpm, disk, pci etc
> > > + *  ptr:     the pointer to array inside libxl_domain_config
> > 
> > To the array or to a specific element of the array? I think the latter.
> 
> To the array of libxl_device_#type. It doesn't point to specific
> element.

OK. "...pointer to the start of the array..." would be unambiguous
(assuming the later comment about pasting num_ prefixes didn't make this
comment go away altogether)

Ian.