1:21 p.m.
This patch does a couple of primary things:
Firstly it allows you to use "hda", etc. as a path for getting block
device stats from fullvirt domains.
Secondly it separates out the stats code into a new file called
'stats_linux.c'. The reasoning behind the name is that this code can be
shared between Xen & QEMU, and that the code is Linux-specific (it never
worked on Solaris, but now this is explicit). I anticipate a
'stats_solaris.c' file once I can get Solaris + Xen going on a test machine.
Also we try to detect the case where the block dev stats of a fullvirt
domain are stuck at 0 -- caused by there being no frontend driver
connected. We detect the condition by a query to xenstore.
XENVBD_MAJOR is no longer hard-coded if we can get it instead from Linux
header files.
This patch adds bytes written/read to block devices for Xen PV domains
if available.
This also corrects a bug where stats from xvdb, xvdc, .. could not be
read out because the device number was being miscalculated.
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
1:50 a.m.
Hi Rich,
I know this patch just moved the code below,
and the probability of data corruption and file I/O errors
here is low, but...
"Richard W.M. Jones" <rjones(a)redhat.com> wrote:
+static int64_t
+read_stat (const char *path)
+{
+ char str[64];
+ int64_t r;
+ int i;
+ FILE *fp;
+
+ fp = fopen (path, "r");
+ if (!fp) return -1;
+ /* stupid GCC warning */ i = fread (str, sizeof str, 1, fp);
+ r = strtoll (str, NULL, 10);
+ fclose (fp);
+ return r;
+}
Since all of fread, strtoll, and fclose can fail, and since the 64 bytes
from fread might be a valid prefix, but not terminated (i.e., strtoll
could overrun the STR buffer -- yeah, it's far-fetched, but still) the
above should probably be rewritten something like e.g.,
WARNING: the following may not even compile
/* Convert NUL-or-NL-terminated string to int64_t, detecting overflow,
invalid string (i.e., non-digit), or a long long value that doesn't
fit in int64_t (probably only theoretical). */
static int
xstrtoint64 (char const *s, int base, int64_t *result)
{
long long int lli;
char *p;
errno = 0;
lli = strtoll (s, &p, base);
if (errno || !(*p == 0 || *p == '\n') || p == s || (int64_t) lli != lli)
return -1;
*result = lli;
return 0;
}
static int64_t
read_stat (const char *path)
{
char str[64];
int64_t r;
int i;
FILE *fp;
fp = fopen (path, "r");
if (!fp)
return -1;
/* read, but don't bail out before closing */
i = fread (str, sizeof str, 1, fp);
if (fclose (fp) != 0
|| i < 2 /* ensure we read at least two bytes */
|| str[i - 1] != 0 /* the last byte must be zero */
|| xstrtoint64 (str, 10, &r) != 0)
return -1;
return r;
}
4:26 a.m.
Thanks Jim. Here is an updated patch which fixes this issue, and is
tested against Xen 3.1.
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
5:15 a.m.
On Fri, Sep 28, 2007 at 10:26:05AM +0100, Richard W.M. Jones wrote:
Thanks Jim. Here is an updated patch which fixes this issue, and is
tested against Xen 3.1.
The principle of the patch sounds good to me, I didn't spot anything
obvious, but I didn't try to test it.
+1
Once it's in CVS and in a release we will see more testing of those APIs
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
5:34 a.m.
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.
10:03 a.m.
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
11:25 a.m.
"Richard W.M. Jones" <rjones(a)redhat.com> wrote:
...
> 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).
Yeah, you're right. len==0 was the only missed case.
I didn't realize at the time that "len" doesn't include
the trailing NUL byte.
> The "unsigned len = 0;" initialization looks
unnecessary.
I left that because I can't see it doing any harm in this case.
It's not a big deal.
True, it causes no run-time harm, but anything unnecessary
can make the reviewer/maintainer wonder "why?".
...
It would have to be really really large (these are 64 bit numbers),
but yes I have fixed this too.
Updated patch is attached.
Looks fine, now.
5:58 a.m.
This patch has been applied.
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
6031
days inactive
6085
days old
7 comments
3 participants
participants (3)
-
Daniel Veillard -
Jim Meyering -
Richard W.M. Jones