Re: [PATCH v4 06/13] libxl: change p9 to use generec add function

From: Wei Liu <wei.liu2@citrix.com>
To: Oleksandr Grytsov <al1img@gmail.com>
Cc: xen-devel@lists.xenproject.org, Wei Liu <wei.liu2@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Oleksandr Grytsov <oleksandr_grytsov@epam.com>
Subject: Re: [PATCH v4 06/13] libxl: change p9 to use generec add function
Date: Mon, 31 Jul 2017 15:36:59 +0100	[thread overview]
Message-ID: <20170731143658.y2gyoczel6je6kr5@citrix.com> (raw)
In-Reply-To: <CACvf2oVv_-GBiOvXa9s28An5UHr0nF_ScJ0dv7OGnPcChsqFpg@mail.gmail.com>

On Sun, Jul 30, 2017 at 09:42:09PM +0300, Oleksandr Grytsov wrote:
> On Fri, Jul 28, 2017 at 7:23 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Fri, Jul 28, 2017 at 03:11:34PM +0100, Wei Liu wrote:
> >> On Tue, Jul 18, 2017 at 05:25:23PM +0300, Oleksandr Grytsov wrote:
> >> [...]
> >> >  /* 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
> >> > @@ -3565,6 +3559,7 @@ extern const struct libxl_device_type libxl__usbctrl_devtype;
> >> >  extern const struct libxl_device_type libxl__usbdev_devtype;
> >> >  extern const struct libxl_device_type libxl__pcidev_devtype;
> >> >  extern const struct libxl_device_type libxl__vdispl_devtype;
> >> > +extern const struct libxl_device_type libxl__p9_devtype;
> >> >
> >> >  extern const struct libxl_device_type *device_type_tbl[];
> >> >
> >> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> >> > index 25563cf..96dbaed 100644
> >> > --- a/tools/libxl/libxl_types.idl
> >> > +++ b/tools/libxl/libxl_types.idl
> >> > @@ -804,7 +804,7 @@ libxl_domain_config = Struct("domain_config", [
> >> >      ("vfbs", Array(libxl_device_vfb, "num_vfbs")),
> >> >      ("vkbs", Array(libxl_device_vkb, "num_vkbs")),
> >> >      ("vtpms", Array(libxl_device_vtpm, "num_vtpms")),
> >> > -    ("p9", Array(libxl_device_p9, "num_p9s")),
> >> > +    ("p9s", Array(libxl_device_p9, "num_p9s")),
> >>
> >> Oh, no, please don't do this. We can't change the name of the fields.
> >>
> >> There is already on irregular device type -- the PCI device. I suppose
> >> you probably need another hook somewhere. And please convert PCI devices
> >> if you can.
> >
> > OK, going through the code I think we need to come to a conclusion if we
> > want an extra callback to handle the irregular device names first
> > because that's likely to affect the code of the framework in previous
> > patch.
> 
> Actually creating new callback to handle irregular device name looks
> not so good.
> There is the pattern which all namings should follow. May be it has to
> be documented

The normal pattern is DEVTYPEs.

> somewhere. p9 was added recently we can ask the author to review this rename.

Once it is released we can't change it, of course unless we deem it
unstable. I'm two minded here. P9 was released in 4.9, which was only a
few months old.

But we definitely can't change the PCI type. It has been around since
forever. And there is provision in code to deal with that.

> From other side this rename touches only internals changes: no changes
> in config file
> or CLI interface.
> 

As said, the framework need to be ready to deal with PCI anyway.

What sort of issues do you foresee here?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel