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
 };
 
-- 
2.34.1

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,

-- 
Anthony PERARD

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,

-- 
Anthony PERARD

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 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>

-- 
viresh

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.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 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