WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH] xend,sparse: move vcpu-hotplug to xenbus/store

To: Ryan Harper <ryanh@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] xend,sparse: move vcpu-hotplug to xenbus/store
From: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
Date: Sat, 06 Aug 2005 18:39:53 +1000
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Sat, 06 Aug 2005 08:38:13 +0000
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20050805192156.GJ7538@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <20050805192156.GJ7538@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
On Fri, 2005-08-05 at 14:21 -0500, Ryan Harper wrote:
> +/* xenbus watch struct */
> +static struct xenbus_watch cpus_watch = {
> +     .node = "cpus",
> +     .callback = handle_cpus_watch,
> +};

Everywhere else we've used singular
names: /domain, /domain/<uuid>/device, /tool, /domain/<uuid>/backend.
So I'd suggest either using "cpu" or consider putting this under
"device" and actually using the xenbus system, although I'm not sure
that's all that useful for you.  Perhaps revisit after Christian has
done latest merge.

> +static void handle_cpus_watch(struct xenbus_watch *watch, const char *node)

Just a preference, I prefer the watch callback to be something like
"handle_XXX_event", which seems clearer to me.

> +     /* get a pointer to start of cpus/cpu string */
> +     if ((cpustr = strstr(node, "cpus/cpu")) != NULL) {
> +
> +             /* find which cpu state changed, note vcpu for handler */
> +             sscanf(cpustr, "cpus/cpu%d", &cpu);

Please don't repeat "cpu", just use "cpu/0", "cpu/1" which matches what
block interfaces are doing.

The guidelines I've been working on with store layout are as follows (no
doubt we'll all come up with more as we use this thing):

1) Redundant information in the store is bad.  Don't put information in
two places then insist they be the same.

2) Multiple entries should be in directory names, unless the hierarchy
is getting completely out of control.

3) Directory names have to be unique, obviously.  If there is a useful
handle to use, do so, otherwise simply use numbers starting from 0,
which makes it fairly clear to the reader the tags are arbitrary.

I'm still not sure about binary flags.  For the block devices, Christian
and I chose "read-only": if it exists, the device is read only, if not,
it's read-write.  That's preferable to a "writability" flag which
contains either "read-only" or "read-write", since the default is
fairly-clear.

In the case of online vs. offline cpus, you could either have an
"offline" entry (not existing meaning "online", which is probably the
clearer default), or a "availability" which contains the string "online"
or "offline".  Please do not have a node called "online" which contains
0 to mean "NOT"!

Thanks!
Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel