Re: [Xen-devel] [PATCH 1/6] libxl: add "merge" function to generic device type support

[Juergen Gross]

On 19/01/17 17:19, Wei Liu wrote:
> On Thu, Jan 19, 2017 at 05:14:35PM +0100, Olaf Hering wrote:
>> On Tue, Jul 12, Juergen Gross wrote:
>>
>>> Instead of using a macro generating the code to merge xenstore and
>>> json configuration data, use the generic device type support for
>>> this purpose.
>>> This requires to add some accessor functions to the framework and
>>> a structure for disks (as disks are added separately they didn't need
>>> such a structure up to now).
>>
>>> +++ b/tools/libxl/libxl.c
>>> @@ -7371,93 +7371,68 @@ int libxl_retrieve_domain_configuration(libxl_ctx 
>>> *ctx, uint32_t domid,
>>
>>> +if (!dt->list || !dt->compare)
>>> +continue;
>>
>>
>> This makes libxl_device__compare optional ...
> 
> Actually this makes both list and compare optional.
> 
> I would say both should be mandatory -- why would you have a device type
> that can't be listed or compared?
> 
> Juergen?

I think above lines predate the introduction of
DEFINE_DEVICE_TYPE_STRUCT_X().

They can probably be removed.


Juergen


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

Re: [Xen-devel] [PATCH 1/6] libxl: add "merge" function to generic device type support

[Wei Liu]

On Thu, Jan 19, 2017 at 05:14:35PM +0100, Olaf Hering wrote:
> On Tue, Jul 12, Juergen Gross wrote:
> 
> > Instead of using a macro generating the code to merge xenstore and
> > json configuration data, use the generic device type support for
> > this purpose.
> > This requires to add some accessor functions to the framework and
> > a structure for disks (as disks are added separately they didn't need
> > such a structure up to now).
> 
> > +++ b/tools/libxl/libxl.c
> > @@ -7371,93 +7371,68 @@ int libxl_retrieve_domain_configuration(libxl_ctx 
> > *ctx, uint32_t domid,
> 
> > +if (!dt->list || !dt->compare)
> > +continue;
> 
> 
> This makes libxl_device__compare optional ...

Actually this makes both list and compare optional.

I would say both should be mandatory -- why would you have a device type
that can't be listed or compared?

Juergen?

Wei.

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

Re: [Xen-devel] [PATCH 1/6] libxl: add "merge" function to generic device type support

[Olaf Hering]

On Tue, Jul 12, Juergen Gross wrote:

> Instead of using a macro generating the code to merge xenstore and
> json configuration data, use the generic device type support for
> this purpose.
> This requires to add some accessor functions to the framework and
> a structure for disks (as disks are added separately they didn't need
> such a structure up to now).

> +++ b/tools/libxl/libxl.c
> @@ -7371,93 +7371,68 @@ int libxl_retrieve_domain_configuration(libxl_ctx 
> *ctx, uint32_t domid,

> +if (!dt->list || !dt->compare)
> +continue;


This makes libxl_device__compare optional ...

> +#define DEFINE_DEVICE_TYPE_STRUCT_X(name, sname, ...)
>   \
> +const struct libxl_device_type libxl__ ## name ## _devtype = {   
>   \
> +.type  = #sname, 
>   \
> +.ptr_offset= offsetof(libxl_domain_config, name ## s),   
>   \
> +.num_offset= offsetof(libxl_domain_config, num_ ## name ## s),   
>   \
> +.dev_elem_size = sizeof(libxl_device_ ## sname), 
>   \
> +.add   = libxl__add_ ## name ## s,   
>   \
> +.list  = (void *(*)(libxl_ctx *, uint32_t, int *))   
>   \
> + libxl_device_ ## sname ## _list,
>   \
> +.dispose   = (void (*)(void *))libxl_device_ ## sname ## 
> _dispose, \
> +.compare   = (int (*)(void *, void *))   
>   \
> + libxl_device_ ## sname ## _compare, 
>   \

... and this makes libxl_device__compare mandatory.

Which one is correct?

Olaf


signature.asc
Description: PGP signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/6] libxl: add "merge" function to generic device type support

[Wei Liu]

On Tue, Jul 12, 2016 at 05:30:39PM +0200, Juergen Gross wrote:
> Instead of using a macro generating the code to merge xenstore and
> json configuration data, use the generic device type support for
> this purpose.
> 
> This requires to add some accessor functions to the framework and
> a structure for disks (as disks are added separately they didn't need
> such a structure up to now).
> 
> Signed-off-by: Juergen Gross 

I definitely think this is a good idea.

The code looks good to me:

Acked-by: Wei Liu 

I didn't do a line-by-line review though because most code looks very
mechanical. I'm confident that we can sort out issues should they arise.

Wei.

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