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

[Rusty Russell <rusty@xxxxxxxxxxxxxxx>]

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