Re: [Libvir] [PATCH] Enhanced stats for fullvirt domains
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.