* Rusty Russell <rusty@xxxxxxxxxxxxxxx> [2005-08-08 08:58]:
> 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.
OK.
>
> > +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.
>
Sure.
> > + /* 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.
Good to know.
>
> 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"!
In this case, I was just following (with the exception of cpus) the
sysfs example which already exists:
/sys/devices/system/cpu/cpu0/online
If we don't care that the xenbus entries _do_ the exact same thing, but
aren't similarly named, I'm fine with that.
Just to be clear, it sounds like you want:
/domain/uuid/cpu/0/online # cpu 0 is online
/domain/uuid/cpu/1/offline # cpu 1 is offline
/domain/uuid/cpu/2/online # etc.
/domain/uuid/cpu/3/online # etc.
I don't like that. What about this?
/domain/uuid/cpu/0/state
and the values can be "online|offline"
I rather like having a single target whose value changes rather than
two entries one of which I must remove when the state changes. In this
case, I can keep a single entry but toss the online=0 stuff you
dislike.
--
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253 T/L: 678-9253
ryanh@xxxxxxxxxx
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home