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

----- Original Message ----- > From: "Peter Krempa" <pkrempa(a)redhat.com&gt; > To: "Francesco Romani" <fromani(a)redhat.com&gt;, libvir-list(a)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:

#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, \

&(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.