On 09/11/14 19:11, Francesco Romani wrote: > ----- Original Message ----- >> From: "Peter Krempa" <[email protected]> >> To: "Francesco Romani" <[email protected]>, [email protected] >> 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
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
