| Message ID | 20230726141459.985463-1-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libxl: Add missing libxl__virtio_devtype to device_type_tbl array | expand |
On 26.07.2023 16:14, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Without it being present it won't be possible to use some > libxl__device_type's callbacks for virtio devices as the common code > can only invoke these callbacks (by dereferencing a pointer) for valid > libxl__device_type's elements when iterating over device_type_tbl[]. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > tools/libs/light/libxl_create.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c > index 393c535579..c91059d713 100644 > --- a/tools/libs/light/libxl_create.c > +++ b/tools/libs/light/libxl_create.c > @@ -1887,6 +1887,7 @@ const libxl__device_type *device_type_tbl[] = { > &libxl__dtdev_devtype, > &libxl__vdispl_devtype, > &libxl__vsnd_devtype, > + &libxl__virtio_devtype, > NULL > }; From description and nature of the change this looks like a Fixes: tag would be warranted. Jan
On 26.07.23 17:50, Jan Beulich wrote: Hello Jan > On 26.07.2023 16:14, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> Without it being present it won't be possible to use some >> libxl__device_type's callbacks for virtio devices as the common code >> can only invoke these callbacks (by dereferencing a pointer) for valid >> libxl__device_type's elements when iterating over device_type_tbl[]. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> --- >> tools/libs/light/libxl_create.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c >> index 393c535579..c91059d713 100644 >> --- a/tools/libs/light/libxl_create.c >> +++ b/tools/libs/light/libxl_create.c >> @@ -1887,6 +1887,7 @@ const libxl__device_type *device_type_tbl[] = { >> &libxl__dtdev_devtype, >> &libxl__vdispl_devtype, >> &libxl__vsnd_devtype, >> + &libxl__virtio_devtype, >> NULL >> }; > > From description and nature of the change this looks like a Fixes: > tag would be warranted. Looks like, yes. Thanks. I guess, this should point to the commit that introduced libxl__virtio_devtype Fixes: 43ba5202e2ee ('libxl: add support for generic virtio device') > > Jan
On 26-07-23, 17:14, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Without it being present it won't be possible to use some > libxl__device_type's callbacks for virtio devices as the common code > can only invoke these callbacks (by dereferencing a pointer) for valid > libxl__device_type's elements when iterating over device_type_tbl[]. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > tools/libs/light/libxl_create.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c > index 393c535579..c91059d713 100644 > --- a/tools/libs/light/libxl_create.c > +++ b/tools/libs/light/libxl_create.c > @@ -1887,6 +1887,7 @@ const libxl__device_type *device_type_tbl[] = { > &libxl__dtdev_devtype, > &libxl__vdispl_devtype, > &libxl__vsnd_devtype, > + &libxl__virtio_devtype, > NULL > }; Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
On Wed, Jul 26, 2023 at 05:14:59PM +0300, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Without it being present it won't be possible to use some > libxl__device_type's callbacks for virtio devices as the common code > can only invoke these callbacks (by dereferencing a pointer) for valid > libxl__device_type's elements when iterating over device_type_tbl[]. Did you notice an issue with it been missing from device_type_tbl[] ? Because to me it looks like all the functions that are using device_type_tbl will just skip over virtio devtype. domcreate_attach_devices: skip virtio because ".skip_attach = 1" libxl__need_xenpv_qemu: skip virtio because "dm_needed" is NULL retrieve_domain_configuration_end: skip because "compare" is "libxl_device_virtio_compare" which is NULL libxl__update_domain_configuration: skip because "update_config" is NULL. So, I think the patch is fine, adding virtio to the device_type_tbl array is good for completeness, but the patch description may be misleading. Did I miss something? Thanks,
On 27.07.23 12:50, Anthony PERARD wrote: Hello Anthony > On Wed, Jul 26, 2023 at 05:14:59PM +0300, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> Without it being present it won't be possible to use some >> libxl__device_type's callbacks for virtio devices as the common code >> can only invoke these callbacks (by dereferencing a pointer) for valid >> libxl__device_type's elements when iterating over device_type_tbl[]. > > Did you notice an issue with it been missing from device_type_tbl[] ? > Because to me it looks like all the functions that are using > device_type_tbl will just skip over virtio devtype. > > domcreate_attach_devices: > skip virtio because ".skip_attach = 1" > > libxl__need_xenpv_qemu: > skip virtio because "dm_needed" is NULL > > retrieve_domain_configuration_end: > skip because "compare" is "libxl_device_virtio_compare" which is NULL > > libxl__update_domain_configuration: > skip because "update_config" is NULL. > > So, I think the patch is fine, adding virtio to the device_type_tbl > array is good for completeness, but the patch description may be > misleading. > > Did I miss something? No, you didn't. Just to be clear, there is no issue within *current* the code base, I am experimenting with using device-model bits, so I implemented libxl__device_virtio_dm_needed() locally and noticed that it didn't get called at all, the reason was in absence of libxl__virtio_devtype in the said array. Do you agree with the following addition to the commit description? "Please note, there is no issue within current the code base as virtio devices don't use callbacks that depend on libxl__virtio_devtype presence in device_type_tbl[]. The issue will appear as soon as we start using these callbacks (for example, dm_needed)." > > Thanks, >
On Thu, Jul 27, 2023 at 10:38:03AM +0000, Oleksandr Tyshchenko wrote: > > > On 27.07.23 12:50, Anthony PERARD wrote: > > Hello Anthony > > > On Wed, Jul 26, 2023 at 05:14:59PM +0300, Oleksandr Tyshchenko wrote: > >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > >> > >> Without it being present it won't be possible to use some > >> libxl__device_type's callbacks for virtio devices as the common code > >> can only invoke these callbacks (by dereferencing a pointer) for valid > >> libxl__device_type's elements when iterating over device_type_tbl[]. > > > > Did you notice an issue with it been missing from device_type_tbl[] ? > > Because to me it looks like all the functions that are using > > device_type_tbl will just skip over virtio devtype. > > > > domcreate_attach_devices: > > skip virtio because ".skip_attach = 1" > > > > libxl__need_xenpv_qemu: > > skip virtio because "dm_needed" is NULL > > > > retrieve_domain_configuration_end: > > skip because "compare" is "libxl_device_virtio_compare" which is NULL > > > > libxl__update_domain_configuration: > > skip because "update_config" is NULL. > > > > So, I think the patch is fine, adding virtio to the device_type_tbl > > array is good for completeness, but the patch description may be > > misleading. > > > > Did I miss something? > > No, you didn't. > > Just to be clear, there is no issue within *current* the code base, I am > experimenting with using device-model bits, so I implemented > libxl__device_virtio_dm_needed() locally and noticed that it didn't get > called at all, the reason was in absence of libxl__virtio_devtype in the > said array. > > Do you agree with the following addition to the commit description? > > "Please note, there is no issue within current the code base as virtio > devices don't use callbacks that depend on libxl__virtio_devtype > presence in device_type_tbl[]. The issue will appear as soon as we start > using these callbacks (for example, dm_needed)." Yes, that would be fine. With that addition: Acked-by: Anthony PERARD <anthony.perard@citrix.com> Thanks,
On 26.07.2023 17:13, Oleksandr Tyshchenko wrote: > On 26.07.23 17:50, Jan Beulich wrote: >> On 26.07.2023 16:14, Oleksandr Tyshchenko wrote: >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> >>> Without it being present it won't be possible to use some >>> libxl__device_type's callbacks for virtio devices as the common code >>> can only invoke these callbacks (by dereferencing a pointer) for valid >>> libxl__device_type's elements when iterating over device_type_tbl[]. >>> >>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> --- >>> tools/libs/light/libxl_create.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c >>> index 393c535579..c91059d713 100644 >>> --- a/tools/libs/light/libxl_create.c >>> +++ b/tools/libs/light/libxl_create.c >>> @@ -1887,6 +1887,7 @@ const libxl__device_type *device_type_tbl[] = { >>> &libxl__dtdev_devtype, >>> &libxl__vdispl_devtype, >>> &libxl__vsnd_devtype, >>> + &libxl__virtio_devtype, >>> NULL >>> }; >> >> From description and nature of the change this looks like a Fixes: >> tag would be warranted. > > Looks like, yes. Thanks. > > I guess, this should point to the commit that introduced > libxl__virtio_devtype > > Fixes: 43ba5202e2ee ('libxl: add support for generic virtio device') In light of Anthony's feedback I'm now thinking that no Fixes: tag should be here, as is being clarified by the addition to the description (which I guess can be folded in while committing). Jan
On 27.07.23 16:45, Anthony PERARD wrote: Hello Anthony > On Thu, Jul 27, 2023 at 10:38:03AM +0000, Oleksandr Tyshchenko wrote: >> >> >> On 27.07.23 12:50, Anthony PERARD wrote: >> >> Hello Anthony >> >>> On Wed, Jul 26, 2023 at 05:14:59PM +0300, Oleksandr Tyshchenko wrote: >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> Without it being present it won't be possible to use some >>>> libxl__device_type's callbacks for virtio devices as the common code >>>> can only invoke these callbacks (by dereferencing a pointer) for valid >>>> libxl__device_type's elements when iterating over device_type_tbl[]. >>> >>> Did you notice an issue with it been missing from device_type_tbl[] ? >>> Because to me it looks like all the functions that are using >>> device_type_tbl will just skip over virtio devtype. >>> >>> domcreate_attach_devices: >>> skip virtio because ".skip_attach = 1" >>> >>> libxl__need_xenpv_qemu: >>> skip virtio because "dm_needed" is NULL >>> >>> retrieve_domain_configuration_end: >>> skip because "compare" is "libxl_device_virtio_compare" which is NULL >>> >>> libxl__update_domain_configuration: >>> skip because "update_config" is NULL. >>> >>> So, I think the patch is fine, adding virtio to the device_type_tbl >>> array is good for completeness, but the patch description may be >>> misleading. >>> >>> Did I miss something? >> >> No, you didn't. >> >> Just to be clear, there is no issue within *current* the code base, I am >> experimenting with using device-model bits, so I implemented >> libxl__device_virtio_dm_needed() locally and noticed that it didn't get >> called at all, the reason was in absence of libxl__virtio_devtype in the >> said array. >> >> Do you agree with the following addition to the commit description? >> >> "Please note, there is no issue within current the code base as virtio >> devices don't use callbacks that depend on libxl__virtio_devtype >> presence in device_type_tbl[]. The issue will appear as soon as we start >> using these callbacks (for example, dm_needed)." > > Yes, that would be fine. With that addition: > Acked-by: Anthony PERARD <anthony.perard@citrix.com> Thanks for the clarification and A-b. > > Thanks, >
On 27.07.23 17:33, Jan Beulich wrote: Hello Jan > On 26.07.2023 17:13, Oleksandr Tyshchenko wrote: >> On 26.07.23 17:50, Jan Beulich wrote: >>> On 26.07.2023 16:14, Oleksandr Tyshchenko wrote: >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> Without it being present it won't be possible to use some >>>> libxl__device_type's callbacks for virtio devices as the common code >>>> can only invoke these callbacks (by dereferencing a pointer) for valid >>>> libxl__device_type's elements when iterating over device_type_tbl[]. >>>> >>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> --- >>>> tools/libs/light/libxl_create.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c >>>> index 393c535579..c91059d713 100644 >>>> --- a/tools/libs/light/libxl_create.c >>>> +++ b/tools/libs/light/libxl_create.c >>>> @@ -1887,6 +1887,7 @@ const libxl__device_type *device_type_tbl[] = { >>>> &libxl__dtdev_devtype, >>>> &libxl__vdispl_devtype, >>>> &libxl__vsnd_devtype, >>>> + &libxl__virtio_devtype, >>>> NULL >>>> }; >>> >>> From description and nature of the change this looks like a Fixes: >>> tag would be warranted. >> >> Looks like, yes. Thanks. >> >> I guess, this should point to the commit that introduced >> libxl__virtio_devtype >> >> Fixes: 43ba5202e2ee ('libxl: add support for generic virtio device') > > In light of Anthony's feedback I'm now thinking that no Fixes: tag > should be here, as is being clarified by the addition to the > description I was about to send V2 with the addition + Fixes tag and noticed your reply. Basically, I agree to not append Fixes tag, there is nothing broken within current code base regarding that, an addition clarifies the state and describes what/how may be broken. I should have mentioned that from the very beginning. (which I guess can be folded in while committing). It would be really good. > > Jan
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c index 393c535579..c91059d713 100644 --- a/tools/libs/light/libxl_create.c +++ b/tools/libs/light/libxl_create.c @@ -1887,6 +1887,7 @@ const libxl__device_type *device_type_tbl[] = { &libxl__dtdev_devtype, &libxl__vdispl_devtype, &libxl__vsnd_devtype, + &libxl__virtio_devtype, NULL };