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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Aug 01, 2017 at 02:58:19PM +0300, Oleksandr Grytsov wrote:
> On Mon, Jul 31, 2017 at 5:36 PM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> > 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@xxxxxxxxxx> 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.
> >
> 
> IMHO this the bug I mean wrong naming and it should be fixed.
> 
> > 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.
> 
> Agree, I didn't touch PCI.
> 
> >> 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?
> 
> Do you mean in case to rework PCI to use the device framework?
> 

No, I mean adding the new hook. You said "handle irregular device name
looks not so good"

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