Xenstore remove problem - causes bug #473

Xenstore remove problem - causes bug #473

[Murillo Bernardes]

Hey folks,

Xenstore remove is removing more than it is supposed to. Like:

xenstore-rm /local/domain/0/backend/vif/1

removes /local/domain/0/backend/vif/1*

That causes bug #473

I tried to find the problem in xenstore_client.c, but it seems to be ok. 
Probably the problem in xenstored, but I could not find where exactly.

Any clue?

-- 
Murillo Fernandes Bernardes
IBM Linux Technology Center

Re: Xenstore remove problem - causes bug #473

[Ewan Mellor]

On Tue, Jan 10, 2006 at 11:01:23AM -0200, Murillo Bernardes wrote:

> Hey folks,
> 
> Xenstore remove is removing more than it is supposed to. Like:
> 
> xenstore-rm /local/domain/0/backend/vif/1
> 
> removes /local/domain/0/backend/vif/1*
> 
> That causes bug #473

I get

~ # xenstore-rm /tools/ewan
~ # xenstore-write /tools/ewan/12/1 v
~ # xenstore-write /tools/ewan/1/1 v
~ # xenstore-write /tools/ewan/13/1 v
~ # xenstore-rm /tools/ewan/1
~ # xenstore-list /tools/ewan
12
13

Which is as is expected, and I get the same when I use the -t flag to
xenstore-rm.  What is your test case exactly?

If you export XENSTORED_TRACE=1 and restart xenstored then you will get a
/var/log/xenstored-trace.log which may be of some use.  Beware that restarting
Xenstored breaks your connections to your backend drivers, so a full reboot is
required if you want to create domains afterwards.

Cheers,

Ewan.

Re: Xenstore remove problem - causes bug #473

[Keir Fraser]

On 10 Jan 2006, at 13:01, Murillo Bernardes wrote:

> Xenstore remove is removing more than it is supposed to. Like:
>
> xenstore-rm /local/domain/0/backend/vif/1
>
> removes /local/domain/0/backend/vif/1*
>
> That causes bug #473
>
> I tried to find the problem in xenstore_client.c, but it seems to be 
> ok.
> Probably the problem in xenstored, but I could not find where exactly.
>
> Any clue?

I'm not sure it's that simple. Try:

xenstore-write /foo/1/bar 0
xenstore-write /foo/11/bar 0
xenstore-rm /foo/1

I find that /foo/11 is not deleted.

Perhaps something is watching vif/1 and, when the watch fires, 
erroneously causing all vif/1* to get deleted?

The xenstored code of interest, by the way, is in 
xenstored_core.c:do_rm()

  -- Keir

Re: Xenstore remove problem - causes bug #473

[Murillo Bernardes]

On Tuesday 10 January 2006 12:19, Keir Fraser wrote:
> I'm not sure it's that simple. Try:
>
> xenstore-write /foo/1/bar 0
> xenstore-write /foo/11/bar 0
> xenstore-rm /foo/1
>
> I find that /foo/11 is not deleted.
>
You're right

> Perhaps something is watching vif/1 and, when the watch fires,
> erroneously causing all vif/1* to get deleted?
>
makes sense. I'm trying to figure out...

-- 
Murillo Fernandes Bernardes
IBM Linux Technology Center

Re: Xenstore remove problem - causes bug #473

[Murillo Bernardes]

On Tuesday 10 January 2006 12:19, Keir Fraser wrote:
> Perhaps something is watching vif/1 and, when the watch fires,
> erroneously causing all vif/1* to get deleted?
>

I found it!

When a change occur on one backend device we call cleanup_devices() for each 
device in bus. 

The problem is: the test to see if that device was changed is being done with 
strncmp(), using the length of the changed node as maximum length to be 
compared. 

I think the idea is that any sub-node should be matched also, like: if 
"backend/1/status" changed we have to call backend_changed to "backend/1" 
device, and that is ok. But with that code If we delete device "backend/1" 
all devices matching "backend/1*" are being deleted also.

Suggestions on the best/prettier way to handle that?

-- 
Murillo Fernandes Bernardes
IBM Linux Technology Center

Re: Xenstore remove problem - causes bug #473

[Keir Fraser]

On 13 Jan 2006, at 12:28, Murillo Bernardes wrote:

> I think the idea is that any sub-node should be matched also, like: if
> "backend/1/status" changed we have to call backend_changed to 
> "backend/1"
> device, and that is ok. But with that code If we delete device 
> "backend/1"
> all devices matching "backend/1*" are being deleted also.

That can't be the logic. If '/backend/1/status' changed then we would 
not strncmp match on '/backend/1'.

The opposite way round would match though. Perhaps that was the intent.

Anyway, I can implement a fix that maintains the current semantics 
w.r.t. matching on subdirectories.

  -- Keir