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

[Francesco Romani]

----- 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:

#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.


-- 
Francesco Romani
RedHat Engineering Virtualization R & D
Phone: 8261328
IRC: fromani