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

[Oleksandr Grytsov <al1img@xxxxxxxxx>]

On Tue, Aug 1, 2017 at 4:00 PM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> 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"

As for me when only internal name of structure or fields are changed
then it should not break anyone configs, setup etc.
Creating hooks in this case makes code hard to read and maintain.

-- 
Best Regards,
Oleksandr Grytsov.

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