[Libvir] [PATCH] Enhanced stats for fullvirt domains

[Richard W.M. Jones]

Jim Meyering wrote:
>> +/* In Xenstore, /local/domain/0/backend/vbd/<domid>/<device>/state,
>> + * if available, must be XenbusStateConnected (= 4), otherwise there
>> + * is no connected device.
>> + */
>> +static int
>> +check_bd_connected (xenUnifiedPrivatePtr priv, int device, int domid)
>> +{
>> +    char s[256], *rs;
>> +    int r;
>> +    unsigned len = 0;
>> +
>> +    /* This code assumes we're connected if we can't get to
>> +     * xenstore, etc.
>> +     */
>> +    if (!priv->xshandle) return 1;
>> +    snprintf (s, sizeof s, "/local/domain/0/backend/vbd/%d/%d/state",
>> +              domid, device);
>> +    s[sizeof s - 1] = '\0';
>> +
>> +    rs = xs_read (priv->xshandle, 0, s, &len);
>> +    if (!rs) return 1;
>> +
>> +    r = STREQ (rs, "4");
>> +    free (rs);
>> +    return r;
>> +}
> 
> That function should also check LEN (i.e., what if it's 0 or 1?).
> Otherwise, STREQ might read uninitialized memory.

I've addressed that by doing something, hopefully the right thing, if 
len == 0.  (We would expect len == 1).

> The "unsigned len = 0;" initialization looks unnecessary.

I left that because I can't see it doing any harm in this case.

> ==================================================================
>> +int
>> +xenLinuxDomainBlockStats (xenUnifiedPrivatePtr priv,
>> +                          virDomainPtr dom,
>> +                          const char *path,
>> +                          struct _virDomainBlockStats *stats)
>> +{
>> +    int minor, device;
>> +
>> +    /* Paravirt domains:
>> +     * Paths have the form "xvd[a-]" and map to paths
>> +     * /sys/devices/xen-backend/(vbd|tap)-domid-major:minor/
>> +     * statistics/(rd|wr|oo)_req.
>> +     * The major:minor is in this case fixed as 202*256 + minor*16
>> +     * where minor is 0 for xvda, 1 for xvdb and so on.
>> +     */
>> +    if (strlen (path) == 4 &&
>> +        STREQLEN (path, "xvd", 3)) {
>> +        if ((minor = path[3] - 'a') < 0 || minor > 26) {
>> +            statsErrorFunc (VIR_ERR_INVALID_ARG, __FUNCTION__,
>> +                            "invalid path, should be xvda, xvdb, etc.",
>> +                            dom->id);
>> +            return -1;
>> +        }
>> +        device = XENVBD_MAJOR * 256 + minor * 16;
>> +
>> +        return read_bd_stats (priv, device, dom->id, stats);
>> +    }
>> +    /* Fullvirt domains:
>> +     * hda, hdb etc map to major = HD_MAJOR*256 + minor*16.
>> +     */
>> +    else if (strlen (path) == 3 &&
>> +             STREQLEN (path, "hd", 2)) {
>> +        if ((minor = path[2] - 'a') < 0 || minor > 26) {
>> +            statsErrorFunc (VIR_ERR_INVALID_ARG, __FUNCTION__,
>> +                            "invalid path, should be hda, hdb, etc.",
>> +                            dom->id);
>> +            return -1;
>> +        }
>> +        device = HD_MAJOR * 256 + minor * 16;
> 
> Probably won't ever happen, but it looks like the code
> should require that minor "letters" be in [a-o] (aka ['a'..'a'+15]).
> If it were to allow larger ones, the "minor * 16" part
> would be large enough to modify the major number bits of "device".
> That sounds undesirable.
> 
> So, if this is something to worry about, you could change
> "minor > 26" to "minor > 15" in the two tests above.

Oh dear, that's a mess isn't it.  I can't find out how the devices above 
xvdo should be numbered, so I took your advice and limited the minor to 
<= 15.  The same for devices >= hdo too.

> ==================================================================
>> +static void
>> +statsErrorFunc(virErrorNumber error, const char *func, const char *info,
>> +               int value)
>> +{
>> +    char fullinfo[1000];
>> +    const char *errmsg;
>> +
>> +    errmsg = __virErrorMsg(error, info);
>> +    if (func != NULL) {
>> +        snprintf(fullinfo, 999, "%s: %s", func, info);
>> +        fullinfo[999] = 0;
>> +        __virRaiseError(NULL, NULL, NULL, VIR_FROM_XEN, error, VIR_ERR_ERROR,
>> +                        errmsg, fullinfo, NULL, value, 0, errmsg, fullinfo,
>> +                        value);
>> +    } else {
>> +        __virRaiseError(NULL, NULL, NULL, VIR_FROM_XEN, error, VIR_ERR_ERROR,
>> +                        errmsg, info, NULL, value, 0, errmsg, info,
>> +                        value);
> 
> Use "sizeof fullinfo - 1", not 999.
> Rather than duplicating the __virRaiseError call,
> how about calling it just once, with info or fullinfo?

Yes, the perils of copying and pasting code.  Fixed both.

> ==================================================================
>> +static int
>> +read_bd_stats (xenUnifiedPrivatePtr priv,
>> ...
>> +    /* 'Bytes' was really sectors when we read it.  Scale up by
>> +     * an assumed sector size.
>> +     */
>> +    if (stats->rd_bytes > 0) stats->rd_bytes *= 512;
>> +    if (stats->wr_bytes > 0) stats->wr_bytes *= 512;
> 
> For large starting values of rd_bytes and wr_bytes,
> it'd be nice if rather than wrapping around, the result
> were the maximum value for that type -- or maybe a sentinel value.

It would have to be really really large (these are 64 bit numbers), but 
yes I have fixed this too.

Updated patch is attached.

Thanks,

Rich.

-- 
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom.  Registered in
England and Wales under Company Registration No. 03798903
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libvirt-fv-block-stats-3.patch
Type: text/x-patch
Size: 19150 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20071015/a426608f/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3237 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20071015/a426608f/attachment-0003.bin>