[Libvir] [PATCH] Enhanced stats for fullvirt domains

[Jim Meyering]

Hi Rich,

[I didn't look through the whole patch, initially.
 Here's the result of a more thorough review.]

> +/* 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.

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

==================================================================
> +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.

==================================================================
> +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?

==================================================================
> +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.