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

[Oleksandr Grytsov <al1img@xxxxxxxxx>]

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?


-- 
Best Regards,
Oleksandr Grytsov.

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