[libvirt] [PATCH 22/53] vircgroup: introduce virCgroupV2GetBlkioIoServiced

[Pavel Hrdina]

On Thu, Oct 04, 2018 at 05:26:30PM +0200, Michal Privoznik wrote:
> On 10/02/2018 10:44 AM, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > ---
> >  src/util/vircgroupv2.c | 66 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> > 
> > diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
> > index 3e2cd16335..30c400f129 100644
> > --- a/src/util/vircgroupv2.c
> > +++ b/src/util/vircgroupv2.c
> > @@ -592,6 +592,71 @@ virCgroupV2GetBlkioWeight(virCgroupPtr group,
> >  }
> >  
> >  
> > +static int
> > +virCgroupV2GetBlkioIoServiced(virCgroupPtr group,
> > +                              long long *bytes_read,
> > +                              long long *bytes_write,
> > +                              long long *requests_read,
> > +                              long long *requests_write)
> > +{
> > +    long long stats_val;
> > +    VIR_AUTOFREE(char *) str1 = NULL;
> > +    char *p1;
> > +    size_t i;
> > +
> > +    const char *value_names[] = {
> > +        "rbytes=",
> > +        "wbytes=",
> > +        "rios=",
> > +        "wios=",
> > +    };
> > +    long long *value_ptrs[] = {
> > +        bytes_read,
> > +        bytes_write,
> > +        requests_read,
> > +        requests_write
> > +    };
> > +
> > +    *bytes_read = 0;
> > +    *bytes_write = 0;
> > +    *requests_read = 0;
> > +    *requests_write = 0;
> > +
> > +    if (virCgroupGetValueStr(group,
> > +                             VIR_CGROUP_CONTROLLER_BLKIO,
> > +                             "io.stat", &str1) < 0) {
> > +        return -1;
> > +    }
> > +
> > +    /* sum up all entries of the same kind, from all devices */
> > +    for (i = 0; i < ARRAY_CARDINALITY(value_names); i++) {
> > +        p1 = str1;
> > +
> > +        while ((p1 = strstr(p1, value_names[i]))) {
> > +            p1 += strlen(value_names[i]);
> > +            if (virStrToLong_ll(p1, &p1, 10, &stats_val) < 0) {
> 
> Rather unusual use, we tend to use two different pointers. Perhaps s/p1/p/?

Actually there is a lot of other uses like this throughout the code so
I would not say it's unusual.  Yes, we can use different pointer for
the endptr and assign to the startptr, which would end up with the same
result.

With two pointer it would look like this:

    p1 += strlen(value_names[i]);
    if (virStrToLong_ll(p1, &p, 10, &stats_val) < 0) {
        ...
    }
    p1 = p;

> > +                virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                               _("Cannot parse byte %sstat '%s'"),
> > +                               value_names[i],
> > +                               p1);
> 
> What do you expect to be printed out here? Because IIUC this will print
> the incorrect string + the rest of the file. Also, "%sstat" will print
> "rwbytes=stat". I'd put a space after %s at least.

That's a good question :) ideally we should print only the part that
we cannot parse as number, however, that might be tricky to do with
this approach and it's not probably worth of handling for the error
case which will most likely never happen.

I'll fix the format strings.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20181005/ca298d50/attachment-0001.sig>