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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list