[BUG] xs.watch and xs.unwatch are unreliable

[BUG] xs.watch and xs.unwatch are unreliable

[Sergei Lebedev]

Hi list,

I’ve initially wanted to report another inconsistency in ``xen.lowlevel.xs`` documentation, but this time the issue is more subtle.

Both ``xs.watch`` and ``xs.unwatch`` accept two arguments: a path to watch and a token. According to the documentation, the second argument must be a string, which makes sense, since the token should be sent directly to XenStore. Instead of doing the simple thing (transmitting the token as-is), the implementation makes a new token from *the pointer* to the token object and sends it instead:

    PyObject *token;
    char token_str[MAX_STRLEN(unsigned long) + 1];

    snprintf(token_str, sizeof(token_str), "%li", (unsigned long)token);

This does work for simple cases, e.g. if a token is a string literal 

    >>> from xen.lowlevel.xs import xs
    >>> h = xs()
    >>> h.watch(“@introduceDomain”, “token”)
    >>> h.unwatch(“@introduceDomain”, “token”)

or a small number

    >>> h.watch(“@introduceDomain”, 42)
    >>> h.unwatch(“@introduceDomain”, 42)

But in the general case this is broken

    >>> h.watch("@introduceDomain", 100000000000000000000000000000)
    >>> h.unwatch("@introduceDomain", 100000000000000000000000000000)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    xen.lowlevel.xs.Error: (2, 'No such file or directory’)

Here’s another example with a string token

    >>> token1 = str(100000000000000000000000000000)
    >>> token2 = str(100000000000000000000000000000)
    >>> token1 == token2
    True
    >>> h.watch("@introduceDomain", token1)
    >>> h.unwatch("@introduceDomain", token2)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    xen.lowlevel.xs.Error: (2, 'No such file or directory’)

I’m not sure what would be the best way to handle this as there might be existing code relying on this undocumented behaviour. What do you think?

Regards,
Sergei
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [BUG] xs.watch and xs.unwatch are unreliable

[Wei Liu]

I've CC'ed some people who might have an idea whether they are replying
on this behaviour. I doubt that but let's better be sure...

On Tue, Mar 01, 2016 at 11:17:54PM +0300, Sergei Lebedev wrote:
> Hi list,
> 
> I’ve initially wanted to report another inconsistency in ``xen.lowlevel.xs`` documentation, but this time the issue is more subtle.
> 
> Both ``xs.watch`` and ``xs.unwatch`` accept two arguments: a path to watch and a token. According to the documentation, the second argument must be a string, which makes sense, since the token should be sent directly to XenStore. Instead of doing the simple thing (transmitting the token as-is), the implementation makes a new token from *the pointer* to the token object and sends it instead:
> 
>     PyObject *token;
>     char token_str[MAX_STRLEN(unsigned long) + 1];
> 
>     snprintf(token_str, sizeof(token_str), "%li", (unsigned long)token);
> 
> This does work for simple cases, e.g. if a token is a string literal 
> 
>     >>> from xen.lowlevel.xs import xs
>     >>> h = xs()
>     >>> h.watch(“@introduceDomain”, “token”)
>     >>> h.unwatch(“@introduceDomain”, “token”)
> 
> or a small number
> 
>     >>> h.watch(“@introduceDomain”, 42)
>     >>> h.unwatch(“@introduceDomain”, 42)
> 
> But in the general case this is broken
> 
>     >>> h.watch("@introduceDomain", 100000000000000000000000000000)
>     >>> h.unwatch("@introduceDomain", 100000000000000000000000000000)
>     Traceback (most recent call last):
>       File "<stdin>", line 1, in <module>
>     xen.lowlevel.xs.Error: (2, 'No such file or directory’)
> 
> Here’s another example with a string token
> 
>     >>> token1 = str(100000000000000000000000000000)
>     >>> token2 = str(100000000000000000000000000000)
>     >>> token1 == token2
>     True
>     >>> h.watch("@introduceDomain", token1)
>     >>> h.unwatch("@introduceDomain", token2)
>     Traceback (most recent call last):
>       File "<stdin>", line 1, in <module>
>     xen.lowlevel.xs.Error: (2, 'No such file or directory’)
> 
> I’m not sure what would be the best way to handle this as there might be existing code relying on this undocumented behaviour. What do you think?
> 

In any case this looks like a real bug.

The fix (as you said) is to transmit the token. I don't think your
proposed fix would break existing users though.

Wei.


> Regards,
> Sergei
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [BUG] xs.watch and xs.unwatch are unreliable

[David Vrabel]

On 01/03/16 20:17, Sergei Lebedev wrote:
> Hi list,
> 
> I’ve initially wanted to report another inconsistency in
> ``xen.lowlevel.xs`` documentation, but this time the issue is more
> subtle.

[...]

> Here’s another example with a string token
> 
>     >>> token1 = str(100000000000000000000000000000)
>     >>> token2 = str(100000000000000000000000000000)
>     >>> token1 == token2
>     True
>     >>> h.watch("@introduceDomain", token1)
>     >>> h.unwatch("@introduceDomain", token2)
>     Traceback (most recent call last):
>       File "<stdin>", line 1, in <module>
>     xen.lowlevel.xs.Error: (2, 'No such file or directory’)
> 
> I’m not sure what would be the best way to handle this as there might
> be existing code relying on this undocumented behaviour. What do you
> think?

I think you're stuck with this behaviour.  If you fix it there's a risk
of breaking existing applications by unwatch removing the wrong watch.

Perhaps you could extend the watch API to return a watch object that has
an unwatch() method?

  watch = h.watch("/some/path", "token")
  ...
  watch.unwatch()

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [BUG] xs.watch and xs.unwatch are unreliable

[Wei Liu]

On Thu, Mar 03, 2016 at 04:47:12PM +0000, David Vrabel wrote:
> On 01/03/16 20:17, Sergei Lebedev wrote:
> > Hi list,
> > 
> > I’ve initially wanted to report another inconsistency in
> > ``xen.lowlevel.xs`` documentation, but this time the issue is more
> > subtle.
> 
> [...]
> 
> > Here’s another example with a string token
> > 
> >     >>> token1 = str(100000000000000000000000000000)
> >     >>> token2 = str(100000000000000000000000000000)
> >     >>> token1 == token2
> >     True
> >     >>> h.watch("@introduceDomain", token1)
> >     >>> h.unwatch("@introduceDomain", token2)
> >     Traceback (most recent call last):
> >       File "<stdin>", line 1, in <module>
> >     xen.lowlevel.xs.Error: (2, 'No such file or directory’)
> > 
> > I’m not sure what would be the best way to handle this as there might
> > be existing code relying on this undocumented behaviour. What do you
> > think?
> 
> I think you're stuck with this behaviour.  If you fix it there's a risk
> of breaking existing applications by unwatch removing the wrong watch.
> 

I'm not sure I follow this. Do you have an example why it would remove
the wrong watch?

Wei.

> Perhaps you could extend the watch API to return a watch object that has
> an unwatch() method?
> 
>   watch = h.watch("/some/path", "token")
>   ...
>   watch.unwatch()
> 
> David
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [BUG] xs.watch and xs.unwatch are unreliable

[David Vrabel]

On 03/03/16 17:02, Wei Liu wrote:
> On Thu, Mar 03, 2016 at 04:47:12PM +0000, David Vrabel wrote:
>> On 01/03/16 20:17, Sergei Lebedev wrote:
>>> Hi list,
>>>
>>> I’ve initially wanted to report another inconsistency in
>>> ``xen.lowlevel.xs`` documentation, but this time the issue is more
>>> subtle.
>>
>> [...]
>>
>>> Here’s another example with a string token
>>>
>>>     >>> token1 = str(100000000000000000000000000000)
>>>     >>> token2 = str(100000000000000000000000000000)
>>>     >>> token1 == token2
>>>     True
>>>     >>> h.watch("@introduceDomain", token1)
>>>     >>> h.unwatch("@introduceDomain", token2)
>>>     Traceback (most recent call last):
>>>       File "<stdin>", line 1, in <module>
>>>     xen.lowlevel.xs.Error: (2, 'No such file or directory’)
>>>
>>> I’m not sure what would be the best way to handle this as there might
>>> be existing code relying on this undocumented behaviour. What do you
>>> think?
>>
>> I think you're stuck with this behaviour.  If you fix it there's a risk
>> of breaking existing applications by unwatch removing the wrong watch.
>>
> 
> I'm not sure I follow this. Do you have an example why it would remove
> the wrong watch?

  token1 = str(100000000000000000000000000000)
  token2 = str(100000000000000000000000000000)

  h.watch("path", token1)
  h.watch("path", token2)

Created two unique tokens.

  h.unwatch("path", token1)

Which watch should be removed if token1 and token2 no longer have a
unique token?  Although, I'm not sure of the behaviour of adding two
watches with the same token.

It also occurs to me that if this area is going to be improved, it
should be the kernel that provides the token since it has to be unique
across all users.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [BUG] xs.watch and xs.unwatch are unreliable

[Wei Liu]

On Thu, Mar 03, 2016 at 05:18:31PM +0000, David Vrabel wrote:
> On 03/03/16 17:02, Wei Liu wrote:
> > On Thu, Mar 03, 2016 at 04:47:12PM +0000, David Vrabel wrote:
> >> On 01/03/16 20:17, Sergei Lebedev wrote:
> >>> Hi list,
> >>>
> >>> I’ve initially wanted to report another inconsistency in
> >>> ``xen.lowlevel.xs`` documentation, but this time the issue is more
> >>> subtle.
> >>
> >> [...]
> >>
> >>> Here’s another example with a string token
> >>>
> >>>     >>> token1 = str(100000000000000000000000000000)
> >>>     >>> token2 = str(100000000000000000000000000000)
> >>>     >>> token1 == token2
> >>>     True
> >>>     >>> h.watch("@introduceDomain", token1)
> >>>     >>> h.unwatch("@introduceDomain", token2)
> >>>     Traceback (most recent call last):
> >>>       File "<stdin>", line 1, in <module>
> >>>     xen.lowlevel.xs.Error: (2, 'No such file or directory’)
> >>>
> >>> I’m not sure what would be the best way to handle this as there might
> >>> be existing code relying on this undocumented behaviour. What do you
> >>> think?
> >>
> >> I think you're stuck with this behaviour.  If you fix it there's a risk
> >> of breaking existing applications by unwatch removing the wrong watch.
> >>
> > 
> > I'm not sure I follow this. Do you have an example why it would remove
> > the wrong watch?
> 
>   token1 = str(100000000000000000000000000000)
>   token2 = str(100000000000000000000000000000)
> 
>   h.watch("path", token1)
>   h.watch("path", token2)
> 
> Created two unique tokens.
> 
>   h.unwatch("path", token1)
> 
> Which watch should be removed if token1 and token2 no longer have a
> unique token?  Although, I'm not sure of the behaviour of adding two
> watches with the same token.
> 

Right. I also have no idea what the behaviour should be...

But given the incarnation of the binding, user would expect it to behave
the same as the underlying C API. Python binding is not responsible for
covering up the undefined behaviour.

And a side note is that the bindings of xs_watch and xs_unwatch can only
be used safely in very restricted way. That is, user needs to stash the
exact token object somewhere, which is not impossible, but also very
inconvenient.

Let's either document this behaviour or change it. I don't have
preference here.

> It also occurs to me that if this area is going to be improved, it
> should be the kernel that provides the token since it has to be unique
> across all users.
> 

This would be good.

Wei.

> David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel