[libvirt] [PATCHv3 5/8] qemu: bulk stats: implement interface group
Peter Krempa
pkrempa at redhat.com
Thu Sep 11 17:16:51 UTC 2014
On 09/11/14 19:11, Francesco Romani wrote:
> ----- Original Message -----
>> From: "Peter Krempa" <pkrempa at redhat.com>
>> To: "Francesco Romani" <fromani at redhat.com>, libvir-list at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140911/8af4054c/attachment-0001.sig>
More information about the libvir-list
mailing list