[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv3 5/8] qemu: bulk stats: implement interface group



On 09/11/14 19:11, Francesco Romani wrote:
> ----- Original Message -----
>> From: "Peter Krempa" <pkrempa redhat com>
>> To: "Francesco Romani" <fromani redhat com>, libvir-list redhat com
>> Sent: Tuesday, September 9, 2014 1:42:15 PM
>> Subject: Re: [libvirt] [PATCHv3 5/8] qemu: bulk stats: implement interface group
> 
>>> + * VIR_DOMAIN_STATS_INTERFACE: Return network interface statistics.
>>> + * The typed parameter keys are in this format:
>>> + * "net.count" - number of network interfaces on this domain
>>> + *               as unsigned int.
>>> + * "net.<num>.name" - name of the interface <num> as string.
>>> + * "net.<num>.rx.bytes" - bytes received as long long.
>>> + * "net.<num>.rx.pkts" - packets received as long long.
>>> + * "net.<num>.rx.errs" - receive errors as long long.
>>> + * "net.<num>.rx.drop" - receive packets dropped as long long.
>>> + * "net.<num>.tx.bytes" - bytes transmitted as long long.
>>> + * "net.<num>.tx.pkts" - packets transmitted as long long.
>>> + * "net.<num>.tx.errs" - transmission errors as long long.
>>> + * "net.<num>.tx.drop" - transmit packets dropped as long long.
>>
>> Why are all of those represented as long long instead of unsigned long
>> long? I don't see how these could be negative. If we need to express
>> that the value is unsupported we can just drop it from here and not
>> waste half of the range here.
>>
>> Any other opinions on this?
> 
> I used long long because of this:
> 
> struct _virDomainInterfaceStats {
>     long long rx_bytes;
>     long long rx_packets;
>     long long rx_errs;
>     long long rx_drop;
>     long long tx_bytes;
>     long long tx_packets;
>     long long tx_errs;
>     long long tx_drop;
> };
> 
> But I don't have any problem to cast them as unsigned, with something like:

That will be fine with me as long as:
> 
> #define QEMU_ADD_NET_PARAM(record, maxparams, num, name, value) \
> do { \
>     char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>     snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
>              "net.%u.%s", num, name); \
>     if (virTypedParamsAddULLong(&(record)->params, \

... you don't add the param if value is < 0. Thus rather than expressing
the missing information via -1 just omit the parameter.

>                                 &(record)->nparams, \
>                                 maxparams, \
>                                 param_name, \
>                                 (unsigned long long)value) < 0) \
>         return -1; \
> } while (0)
> 
> 
>>
>>> + *
>>>   * Using 0 for @stats returns all stats groups supported by the given
>>>   * hypervisor.
>>>   *
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 6bcbfb5..989eb3e 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -17537,6 +17537,92 @@ qemuDomainGetStatsVcpu(virConnectPtr conn
>>> ATTRIBUTE_UNUSED,
>>>      return ret;
>>>  }
>>>  
>>> +#define QEMU_ADD_COUNT_PARAM(record, maxparams, type, count) \
>>> +do { \
>>> +    char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>>> +    snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "%s.count", type);
>>> \
>>> +    if (virTypedParamsAddUInt(&(record)->params, \
>>> +                              &(record)->nparams, \
>>> +                              maxparams, \
>>> +                              param_name, \
>>> +                              count) < 0) \
>>> +        return -1; \
>>> +} while (0)
>>> +
>>> +#define QEMU_ADD_NAME_PARAM(record, maxparams, type, num, name) \
>>> +do { \
>>> +    char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>>> +    snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
>>> +             "%s.%lu.name", type, num); \
>>> +    if (virTypedParamsAddString(&(record)->params, \
>>> +                                &(record)->nparams, \
>>> +                                maxparams, \
>>> +                                param_name, \
>>> +                                name) < 0) \
>>> +        return -1; \
>>> +} while (0)
>>> +
>>> +#define QEMU_ADD_NET_PARAM(record, maxparams, num, name, value) \
>>> +do { \
>>> +    char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>>> +    snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
>>> +             "net.%lu.%s", num, name); \
>>
>> %lu? the count is unsigned int so you should be fine with %d
> 
> Yep but the cycle counter is size_t and then...
> 
> $ git diff
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9d53883..e90a8c6 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -17487,7 +17487,7 @@ do { \
>  do { \
>      char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>      snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> -             "net.%lu.%s", num, name); \
> +             "net.%u.%s", num, name); \
>      if (virTypedParamsAddLLong(&(record)->params, \
>                                 &(record)->nparams, \
>                                 maxparams, \
> $ make
> [...]
> make[1]: Entering directory `/home/fromani/Projects/libvirt/src'
>   CC       qemu/libvirt_driver_qemu_impl_la-qemu_driver.lo
> qemu/qemu_driver.c: In function 'qemuDomainGetStatsInterface':
> qemu/qemu_driver.c:17521:9: error: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'size_t' [-Werror=format=]
>          QEMU_ADD_NET_PARAM(record, maxparams, i,
>          ^
> qemu/qemu_driver.c:17521:9: error: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'size_t' [-Werror=format=]
> $ gcc --version
> gcc (GCC) 4.8.3 20140624 (Red Hat 4.8.3-1)
> 
> same story with '%d'. Keeping '%lu' for this reason.

If it's size_t, then you should use %zu as a formatter.

Peter




Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]