Re: [Xen-devel] [PATCH v2 4/6] tools/libxc: Introduce ARRAY_SIZE() and replace handrolled examples

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 10/06/14 13:40, Ian Campbell wrote:
> On Mon, 2014-06-09 at 16:41 +0100, Andrew Cooper wrote:
>> The conditional test is needed because xc_private.h is included into minios
>> for stubdomains, where ARRAY_SIZE is already defined.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
>> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
>>
>> --
>> v2: Find more areas of the codebase which are poking at libxc private 
>> internals
>> ---
>>  tools/libxc/xc_dom_arm.c |    2 +-
>>  tools/libxc/xc_dom_x86.c |    4 ++--
>>  tools/libxc/xc_minios.c  |    2 +-
>>  tools/libxc/xc_private.h |    4 ++++
>>  tools/misc/xen-hptool.c  |    2 --
>>  tools/misc/xen-mfndump.c |    2 --
> Not tools/misc/xenpm.c too?

No - xenpm.c is well behaved and doesn't use private headers.  (and I
discovered this by ripping them all out and xenpm failed to compile)

>
> tools/misc/* really didn't ought to be including xc_private.h I think.

I agree, but it didn't look trivial to fix.

>
> Rather than make this worse how about defining XC_ARRAY_SIZE in the
> public API?

I don't think this should really live in the public API.

>  Or if not perhaps
>    #undef ARRAY_SIZE /* We shouldn't be including xc_private.h */
> in those so that an eventual removal of xc_private from them isn't
> blocked? (I think I slightly prefer the second option)

This is my preferred option.

~Andrew

>
> minios really shouldn't be polluting the namespace like that either.
> Perhaps that's a yack Ian J is going to cover as part of the rump kernel
> stuff...
>
>>  6 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
>> index 75f8363..cc64363 100644
>> --- a/tools/libxc/xc_dom_arm.c
>> +++ b/tools/libxc/xc_dom_arm.c
>> @@ -230,7 +230,7 @@ static int set_mode(xc_interface *xch, domid_t domid, 
>> char *guest_type)
>>  
>>      domctl.domain = domid;
>>      domctl.cmd    = XEN_DOMCTL_set_address_size;
>> -    for ( i = 0; i < sizeof(types)/sizeof(types[0]); i++ )
>> +    for ( i = 0; i < ARRAY_SIZE(types); i++ )
>>          if ( !strcmp(types[i].guest, guest_type) )
>>              domctl.u.address_size.size = types[i].size;
>>      if ( domctl.u.address_size.size == 0 )
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index e034d62..bf06fe4 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -716,7 +716,7 @@ static int x86_compat(xc_interface *xch, domid_t domid, 
>> char *guest_type)
>>      memset(&domctl, 0, sizeof(domctl));
>>      domctl.domain = domid;
>>      domctl.cmd    = XEN_DOMCTL_set_address_size;
>> -    for ( i = 0; i < sizeof(types)/sizeof(types[0]); i++ )
>> +    for ( i = 0; i < ARRAY_SIZE(types); i++ )
>>          if ( !strcmp(types[i].guest, guest_type) )
>>              domctl.u.address_size.size = types[i].size;
>>      if ( domctl.u.address_size.size == 0 )
>> @@ -887,7 +887,7 @@ int arch_setup_bootlate(struct xc_dom_image *dom)
>>      xen_pfn_t shinfo;
>>      int i, rc;
>>  
>> -    for ( i = 0; i < sizeof(types) / sizeof(types[0]); i++ )
>> +    for ( i = 0; i < ARRAY_SIZE(types); i++ )
>>          if ( !strcmp(types[i].guest, dom->guest_type) )
>>              pgd_type = types[i].pgd_type;
>>  
>> diff --git a/tools/libxc/xc_minios.c b/tools/libxc/xc_minios.c
>> index e621417..e703684 100644
>> --- a/tools/libxc/xc_minios.c
>> +++ b/tools/libxc/xc_minios.c
>> @@ -87,7 +87,7 @@ static int minios_privcmd_hypercall(xc_interface *xch, 
>> xc_osdep_handle h, privcm
>>      int i, ret;
>>  
>>      call.op = hypercall->op;
>> -    for (i = 0; i < sizeof(hypercall->arg) / sizeof(*hypercall->arg); i++)
>> +    for (i = 0; i < ARRAY_SIZE(hypercall->arg); i++)
>>      call.args[i] = hypercall->arg[i];
>>  
>>      ret = HYPERVISOR_multicall(&call, 1);
>> diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
>> index 12476ec..213f607 100644
>> --- a/tools/libxc/xc_private.h
>> +++ b/tools/libxc/xc_private.h
>> @@ -54,6 +54,10 @@
>>  #define XC_BUILD_BUG_ON(p) ((void)sizeof(struct { int:-!!(p); }))
>>  #endif
>>  
>> +#ifndef ARRAY_SIZE
>> +#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
>> +#endif
>> +
>>  /*
>>  ** Define max dirty page cache to permit during save/restore -- need to 
>> balance 
>>  ** keeping cache usage down with CPU impact of invalidating too often.
>> diff --git a/tools/misc/xen-hptool.c b/tools/misc/xen-hptool.c
>> index d0e8e90..a16213a 100644
>> --- a/tools/misc/xen-hptool.c
>> +++ b/tools/misc/xen-hptool.c
>> @@ -4,8 +4,6 @@
>>  #include <xenstore.h>
>>  #include <unistd.h>
>>  
>> -#define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
>> -
>>  static xc_interface *xch;
>>  
>>  void show_help(void)
>> diff --git a/tools/misc/xen-mfndump.c b/tools/misc/xen-mfndump.c
>> index 1af3bd8..33e4b91 100644
>> --- a/tools/misc/xen-mfndump.c
>> +++ b/tools/misc/xen-mfndump.c
>> @@ -6,8 +6,6 @@
>>  
>>  #include "xg_save_restore.h"
>>  
>> -#define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
>> -
>>  static xc_interface *xch;
>>  
>>  int help_func(int argc, char *argv[])
>


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