Re: [PATCH] libxl: Add missing libxl__virtio_devtype to device_type_tbl array

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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@xxxxxxxx>
> >>
> >> 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@xxxxxxxxxx>

Thanks,

-- 
Anthony PERARD