On 09/11/14 19:11, Francesco Romani wrote: > ----- Original Message ----- >> From: "Peter Krempa" <pkrempa@xxxxxxxxxx> >> To: "Francesco Romani" <fromani@xxxxxxxxxx>, libvir-list@xxxxxxxxxx >> Sent: Tuesday, September 9, 2014 1:42:15 PM >> Subject: Re: [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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list