Mihir Nanavati
2010-Dec-08 20:47 UTC
[Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
Adds an open xenstore connection function which tries to use the xenbus interface (xs_domain_open) when the socket interface (xs_daemon_opn) fails. Signed-off-by: Mihir Nanavati <mihirn@cs.ubc.ca> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-09 09:17 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
On Wed, 2010-12-08 at 20:47 +0000, Mihir Nanavati wrote:> Adds an open xenstore connection function which tries to use the > xenbus interface (xs_domain_open) when the socket interface > (xs_daemon_opn) fails.I like the concept of this patch. I can''t think of any reason why the callers of libxenstore should need to care about how which communication channel gets used so there''s no reason for the library to defer that choice to the caller. So perhaps we should go one step further push this behaviour down into libxenstore itself? i.e. add "xs_open(int ro)" and make xs_{daemon,domain}_open{,_readonly} compatibility aliases to that function.> Signed-off-by: Mihir Nanavati <mihirn@cs.ubc.ca>_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Mihir Nanavati
2010-Dec-09 09:59 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
Sure, that sounds doable. I''m not sure what the behaviour of the readonly should be though - I don''t see any way of opening the xenbus client as readonly, or at least nothing quite as simple as a xs_domain_open_readonly. How should a xs_open(1) behave in the case that xs_daemon_open_readonly fails? A null or a xs_handle using xs_domain_open? Is there a way to constrain the xenbus handle using some other mechanism? ~M On Thu, Dec 9, 2010 at 1:17 AM, Ian Campbell <Ian.Campbell@citrix.com>wrote:> On Wed, 2010-12-08 at 20:47 +0000, Mihir Nanavati wrote: > > Adds an open xenstore connection function which tries to use the > > xenbus interface (xs_domain_open) when the socket interface > > (xs_daemon_opn) fails. > > I like the concept of this patch. > > I can''t think of any reason why the callers of libxenstore should need > to care about how which communication channel gets used so there''s no > reason for the library to defer that choice to the caller. So perhaps we > should go one step further push this behaviour down into libxenstore > itself? i.e. add "xs_open(int ro)" and make > xs_{daemon,domain}_open{,_readonly} compatibility aliases to that > function. > > > Signed-off-by: Mihir Nanavati <mihirn@cs.ubc.ca> > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Vincent Hanquez
2010-Dec-09 10:06 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
On 09/12/10 09:17, Ian Campbell wrote:> On Wed, 2010-12-08 at 20:47 +0000, Mihir Nanavati wrote: >> Adds an open xenstore connection function which tries to use the >> xenbus interface (xs_domain_open) when the socket interface >> (xs_daemon_opn) fails. > > I like the concept of this patch. > > I can''t think of any reason why the callers of libxenstore should need > to care about how which communication channel gets used so there''s no > reason for the library to defer that choice to the caller. So perhaps we > should go one step further push this behaviour down into libxenstore > itself? i.e. add "xs_open(int ro)" and make > xs_{daemon,domain}_open{,_readonly} compatibility aliases to that > function.Unfortunately, there is practical difference between the socket and the page interface. Which is the reason why every xs connection in xapi are sockets; IIRC polling for events on the page interface didn''t really work; I think there was at least 1 other practical differences, but it seems to have eluded me. -- Vincent _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-09 10:21 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
On Thu, 2010-12-09 at 10:06 +0000, Vincent Hanquez wrote:> On 09/12/10 09:17, Ian Campbell wrote: > > On Wed, 2010-12-08 at 20:47 +0000, Mihir Nanavati wrote: > >> Adds an open xenstore connection function which tries to use the > >> xenbus interface (xs_domain_open) when the socket interface > >> (xs_daemon_opn) fails. > > > > I like the concept of this patch. > > > > I can''t think of any reason why the callers of libxenstore should need > > to care about how which communication channel gets used so there''s no > > reason for the library to defer that choice to the caller. So perhaps we > > should go one step further push this behaviour down into libxenstore > > itself? i.e. add "xs_open(int ro)" and make > > xs_{daemon,domain}_open{,_readonly} compatibility aliases to that > > function. > > Unfortunately, there is practical difference between the socket and the > page interface. Which is the reason why every xs connection in xapi are > sockets; IIRC polling for events on the page interface didn''t really > work;Good point. The proposed function does try the socket connection first though so it ought to work wherever it used too, at the expense of not getting a useful failure message in cases where it wouldn''t have. The pvops kernels /proc/xen/xenbus driver has a poll method with plausible looking runes in it so it''s possible that the problem is historical anyway. If that poll functionality turns out to be buggy then we should just fix it anyway.> I think there was at least 1 other practical differences, but it > seems to have eluded me.Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-09 10:37 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
On Thu, 2010-12-09 at 09:59 +0000, Mihir Nanavati wrote:> Sure, that sounds doable. > > I''m not sure what the behaviour of the readonly should be though - I > don''t see any way of opening the xenbus client as readonly, or at > least nothing quite as simple as a xs_domain_open_readonly.What are readonly connections actually used for? Are they actually enforcing some part of the security model or just preventing casual accidents? The only user I can see is libxenstat which immediately beforehand opened the privcmd interface so it''s already privileged enough to get a rw connection if it wants. The python bindings provide the functionality but I can''t see it being used anywhere. Perhaps just open /proc/xen/xenbus with O_RDONLY and if the kernel driver doesn''t enforce that then it''s a bug in the driver, but not a critical one since we don''t actually rely on it for security?> How should a xs_open(1) behave in the case that > xs_daemon_open_readonly fails? A null or a xs_handle using > xs_domain_open?I don''t think there is any harm in trying to open the rw handle -- if that succeeds then the calling process is privileged enough in the first place.> Is there a way to constrain the xenbus handle using some other > mechanism? > > ~M > > On Thu, Dec 9, 2010 at 1:17 AM, Ian Campbell <Ian.Campbell@citrix.com> > wrote: > On Wed, 2010-12-08 at 20:47 +0000, Mihir Nanavati wrote: > > Adds an open xenstore connection function which tries to use > the > > xenbus interface (xs_domain_open) when the socket interface > > (xs_daemon_opn) fails. > > > I like the concept of this patch. > > I can''t think of any reason why the callers of libxenstore > should need > to care about how which communication channel gets used so > there''s no > reason for the library to defer that choice to the caller. So > perhaps we > should go one step further push this behaviour down into > libxenstore > itself? i.e. add "xs_open(int ro)" and make > xs_{daemon,domain}_open{,_readonly} compatibility aliases to > that > function. > > > > Signed-off-by: Mihir Nanavati <mihirn@cs.ubc.ca> > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Vincent Hanquez
2010-Dec-09 10:53 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
On 09/12/10 10:21, Ian Campbell wrote:>> I think there was at least 1 other practical differences, but it >> seems to have eluded me.Thinking more about it, it might be related to timeout, and/or it might be: If xenstored has decided to not answer you, your thread is dead in the water, because you end up stuck in kernel land forever (wait not interruptible aka D). (It might have been related to historical reasons, however it could still be happening in the unlikely event of xenstored dying) -- Vincent _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-09 11:39 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
On Thu, 2010-12-09 at 10:53 +0000, Vincent Hanquez wrote:> On 09/12/10 10:21, Ian Campbell wrote: > >> I think there was at least 1 other practical differences, but it > >> seems to have eluded me. > > Thinking more about it, it might be related to timeout, and/or it might > be: If xenstored has decided to not answer you, your thread is dead in > the water, because you end up stuck in kernel land forever (wait not > interruptible aka D). > > (It might have been related to historical reasons, however it could > still be happening in the unlikely event of xenstored dying)Sounds plausible. The current driver seems to use wait_event_interruptible and attempt to do something sane looking with O_NONBLOCK on read but on write looks like it may end up waiting for a reply forever if xenstored has gone away, there''s even a "FIXME avoid synchronous wait for response" in the code. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Mihir Nanavati
2010-Dec-10 06:52 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
I''ve made the changes in libxenstore as you recommended -
xs_daemon|domain_open{_readonly} all alias to xs_open.
Also, in the case of opening read-only, if it fails using sockets, it opens
/proc/xen/xenbus with O_RDONLY.
~M
On Thu, Dec 9, 2010 at 3:39 AM, Ian Campbell
<Ian.Campbell@eu.citrix.com>wrote:
> On Thu, 2010-12-09 at 10:53 +0000, Vincent Hanquez wrote:
> > On 09/12/10 10:21, Ian Campbell wrote:
> > >> I think there was at least 1 other practical differences,
but it
> > >> seems to have eluded me.
> >
> > Thinking more about it, it might be related to timeout, and/or it
might
> > be: If xenstored has decided to not answer you, your thread is dead in
> > the water, because you end up stuck in kernel land forever (wait not
> > interruptible aka D).
> >
> > (It might have been related to historical reasons, however it could
> > still be happening in the unlikely event of xenstored dying)
>
> Sounds plausible.
>
> The current driver seems to use wait_event_interruptible and attempt to
> do something sane looking with O_NONBLOCK on read but on write looks
> like it may end up waiting for a reply forever if xenstored has gone
> away, there''s even a "FIXME avoid synchronous wait for
response" in the
> code.
>
> Ian.
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-10 09:07 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
On Fri, 2010-12-10 at 06:52 +0000, Mihir Nanavati wrote:> I''ve made the changes in libxenstore as you recommended - xs_daemon| > domain_open{_readonly} all alias to xs_open. > > > Also, in the case of opening read-only, if it fails using sockets, it > opens /proc/xen/xenbus with O_RDONLY.Thanks! I think xs_open should be simplified to: if (ro) xsh = get_handle(xs_daemon_socket_ro(), ro); else xsh = get_handle(xs_daemon_socket(), ro); if (!xsh) xsh = get_handle(xs_domain_dev(), ro); For future flexibility should we consider passing a flags argument and defining "XS_OPEN_READONLY 1<<0" instead of having an ro argument? It''s probably worth adding xs_close (with xs_daemon_close as an alias) for API symmetry as well as adding a comment to the header marking the aliases as deprecated. I don''t suppose you feel like running sed over the tree to convert the in tree users, do you ;-) Ian.> > > ~M > > On Thu, Dec 9, 2010 at 3:39 AM, Ian Campbell > <Ian.Campbell@eu.citrix.com> wrote: > > On Thu, 2010-12-09 at 10:53 +0000, Vincent Hanquez wrote: > > On 09/12/10 10:21, Ian Campbell wrote: > > >> I think there was at least 1 other practical > differences, but it > > >> seems to have eluded me. > > > > Thinking more about it, it might be related to timeout, > and/or it might > > be: If xenstored has decided to not answer you, your thread > is dead in > > the water, because you end up stuck in kernel land forever > (wait not > > interruptible aka D). > > > > (It might have been related to historical reasons, however > it could > > still be happening in the unlikely event of xenstored dying) > > > Sounds plausible. > > The current driver seems to use wait_event_interruptible and > attempt to > do something sane looking with O_NONBLOCK on read but on write > looks > like it may end up waiting for a reply forever if xenstored > has gone > away, there''s even a "FIXME avoid synchronous wait for > response" in the > code. > > Ian. > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Mihir Nanavati
2010-Dec-10 09:38 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
On Fri, Dec 10, 2010 at 1:07 AM, Ian Campbell <Ian.Campbell@eu.citrix.com>wrote:> On Fri, 2010-12-10 at 06:52 +0000, Mihir Nanavati wrote: > > I''ve made the changes in libxenstore as you recommended - xs_daemon| > > domain_open{_readonly} all alias to xs_open. > > > > > > Also, in the case of opening read-only, if it fails using sockets, it > > opens /proc/xen/xenbus with O_RDONLY. > > Thanks! I think xs_open should be simplified to: > if (ro) > xsh = get_handle(xs_daemon_socket_ro(), ro); > else > xsh = get_handle(xs_daemon_socket(), ro); > > if (!xsh) > xsh = get_handle(xs_domain_dev(), ro); > >Thanks, that looks much cleaner.> For future flexibility should we consider passing a flags argument and > defining "XS_OPEN_READONLY 1<<0" instead of having an ro argument? > >Sure, we could do it, but I''m not too sure what other modes we could have for opening, let alone ones that might be used simultaneously in a bit field ;)> It''s probably worth adding xs_close (with xs_daemon_close as an alias) > for API symmetry as well as adding a comment to the header marking the > aliases as deprecated. >Sure, will do.> > I don''t suppose you feel like running sed over the tree to convert the > in tree users, do you ;-) > >Could do, but I''d rather we get the interface finalized first ;) Is there anything one specially needs to take into consideration when replacing them in the bindings? ~M> Ian. > > > > > > > ~M > > > > On Thu, Dec 9, 2010 at 3:39 AM, Ian Campbell > > <Ian.Campbell@eu.citrix.com> wrote: > > > > On Thu, 2010-12-09 at 10:53 +0000, Vincent Hanquez wrote: > > > On 09/12/10 10:21, Ian Campbell wrote: > > > >> I think there was at least 1 other practical > > differences, but it > > > >> seems to have eluded me. > > > > > > Thinking more about it, it might be related to timeout, > > and/or it might > > > be: If xenstored has decided to not answer you, your thread > > is dead in > > > the water, because you end up stuck in kernel land forever > > (wait not > > > interruptible aka D). > > > > > > (It might have been related to historical reasons, however > > it could > > > still be happening in the unlikely event of xenstored dying) > > > > > > Sounds plausible. > > > > The current driver seems to use wait_event_interruptible and > > attempt to > > do something sane looking with O_NONBLOCK on read but on write > > looks > > like it may end up waiting for a reply forever if xenstored > > has gone > > away, there''s even a "FIXME avoid synchronous wait for > > response" in the > > code. > > > > Ian. > > > > > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-10 09:48 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
On Fri, 2010-12-10 at 09:38 +0000, Mihir Nanavati wrote:> > > On Fri, Dec 10, 2010 at 1:07 AM, Ian Campbell > > For future flexibility should we consider passing a flags > argument and defining "XS_OPEN_READONLY 1<<0" instead of > having an ro argument? > > Sure, we could do it, but I''m not too sure what other modes we could > have for opening, let alone ones that might be used simultaneously in > a bit field ;)There''s no downside to using a flag field now, even if no compelling use cases come to mind right now and it might avoid an API change in the future. One vague thought I had was that I recently added a "nonreentrant" flag to libxc for use in language bindings which like to control threading themselves. Some sort of "no watches" flag might be useful in the future for similar reasons.> I don''t suppose you feel like running sed over the tree to > convert the > in tree users, do you ;-) > > > Could do, but I''d rather we get the interface finalized first ;)Sure.> Is there anything one specially needs to take into consideration when > replacing them in the bindings?I can''t think of any -- try it and if it isn''t obviously broken it''s probably fine ;-) Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Mihir Nanavati
2010-Dec-10 09:55 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
Fair enough - is this something like what you had in mind? ~M On Fri, Dec 10, 2010 at 1:48 AM, Ian Campbell <Ian.Campbell@eu.citrix.com>wrote:> On Fri, 2010-12-10 at 09:38 +0000, Mihir Nanavati wrote: > > > > > > On Fri, Dec 10, 2010 at 1:07 AM, Ian Campbell > > > > For future flexibility should we consider passing a flags > > argument and defining "XS_OPEN_READONLY 1<<0" instead of > > having an ro argument? > > > > Sure, we could do it, but I''m not too sure what other modes we could > > have for opening, let alone ones that might be used simultaneously in > > a bit field ;) > > There''s no downside to using a flag field now, even if no compelling use > cases come to mind right now and it might avoid an API change in the > future. > > One vague thought I had was that I recently added a "nonreentrant" flag > to libxc for use in language bindings which like to control threading > themselves. Some sort of "no watches" flag might be useful in the future > for similar reasons. > > > I don''t suppose you feel like running sed over the tree to > > convert the > > in tree users, do you ;-) > > > > > > Could do, but I''d rather we get the interface finalized first ;) > > Sure. > > > Is there anything one specially needs to take into consideration when > > replacing them in the bindings? > > I can''t think of any -- try it and if it isn''t obviously broken it''s > probably fine ;-) > > Ian. > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-10 10:03 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
On Fri, 2010-12-10 at 09:55 +0000, Mihir Nanavati wrote:> Fair enough - is this something like what you had in mind?Almost. You don''t need two bits to encode the boolean writeable property -- I reckon should just ditch XS_OPEN_READWRITE since its the default and equivalent to the absence of XS_OPEN_READONLY. The common case should be to pass flags == 0 and get a read+write connection. Ian.> > ~M > > On Fri, Dec 10, 2010 at 1:48 AM, Ian Campbell > <Ian.Campbell@eu.citrix.com> wrote: > On Fri, 2010-12-10 at 09:38 +0000, Mihir Nanavati wrote: > > > > > > On Fri, Dec 10, 2010 at 1:07 AM, Ian Campbell > > > > > For future flexibility should we consider passing a > flags > > argument and defining "XS_OPEN_READONLY 1<<0" > instead of > > having an ro argument? > > > > Sure, we could do it, but I''m not too sure what other modes > we could > > have for opening, let alone ones that might be used > simultaneously in > > a bit field ;) > > > There''s no downside to using a flag field now, even if no > compelling use > cases come to mind right now and it might avoid an API change > in the > future. > > One vague thought I had was that I recently added a > "nonreentrant" flag > to libxc for use in language bindings which like to control > threading > themselves. Some sort of "no watches" flag might be useful in > the future > for similar reasons. > > > I don''t suppose you feel like running sed over the > tree to > > convert the > > in tree users, do you ;-) > > > > > > Could do, but I''d rather we get the interface finalized > first ;) > > > Sure. > > > Is there anything one specially needs to take into > consideration when > > replacing them in the bindings? > > > I can''t think of any -- try it and if it isn''t obviously > broken it''s > probably fine ;-) > > Ian. > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Mihir Nanavati
2010-Dec-10 10:34 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
Done. ~M On Fri, Dec 10, 2010 at 2:03 AM, Ian Campbell <Ian.Campbell@eu.citrix.com>wrote:> On Fri, 2010-12-10 at 09:55 +0000, Mihir Nanavati wrote: > > Fair enough - is this something like what you had in mind? > > Almost. You don''t need two bits to encode the boolean writeable property > -- I reckon should just ditch XS_OPEN_READWRITE since its the default > and equivalent to the absence of XS_OPEN_READONLY. The common case > should be to pass flags == 0 and get a read+write connection. > > Ian. > > > > > ~M > > > > On Fri, Dec 10, 2010 at 1:48 AM, Ian Campbell > > <Ian.Campbell@eu.citrix.com> wrote: > > On Fri, 2010-12-10 at 09:38 +0000, Mihir Nanavati wrote: > > > > > > > > > On Fri, Dec 10, 2010 at 1:07 AM, Ian Campbell > > > > > > > > For future flexibility should we consider passing a > > flags > > > argument and defining "XS_OPEN_READONLY 1<<0" > > instead of > > > having an ro argument? > > > > > > Sure, we could do it, but I''m not too sure what other modes > > we could > > > have for opening, let alone ones that might be used > > simultaneously in > > > a bit field ;) > > > > > > There''s no downside to using a flag field now, even if no > > compelling use > > cases come to mind right now and it might avoid an API change > > in the > > future. > > > > One vague thought I had was that I recently added a > > "nonreentrant" flag > > to libxc for use in language bindings which like to control > > threading > > themselves. Some sort of "no watches" flag might be useful in > > the future > > for similar reasons. > > > > > I don''t suppose you feel like running sed over the > > tree to > > > convert the > > > in tree users, do you ;-) > > > > > > > > > Could do, but I''d rather we get the interface finalized > > first ;) > > > > > > Sure. > > > > > Is there anything one specially needs to take into > > consideration when > > > replacing them in the bindings? > > > > > > I can''t think of any -- try it and if it isn''t obviously > > broken it''s > > probably fine ;-) > > > > Ian. > > > > > > > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-10 10:45 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
Thanks but please put the deprecation comment in the header where potential callers are mostly likely to see it. Tiny nitpick: it should be "if (...)" not "if(...)". On Fri, 2010-12-10 at 10:34 +0000, Mihir Nanavati wrote:> Done. > > ~M > > On Fri, Dec 10, 2010 at 2:03 AM, Ian Campbell > <Ian.Campbell@eu.citrix.com> wrote: > On Fri, 2010-12-10 at 09:55 +0000, Mihir Nanavati wrote: > > Fair enough - is this something like what you had in mind? > > > Almost. You don''t need two bits to encode the boolean > writeable property > -- I reckon should just ditch XS_OPEN_READWRITE since its the > default > and equivalent to the absence of XS_OPEN_READONLY. The common > case > should be to pass flags == 0 and get a read+write connection. > > Ian. > > > > > > ~M > > > > On Fri, Dec 10, 2010 at 1:48 AM, Ian Campbell > > <Ian.Campbell@eu.citrix.com> wrote: > > On Fri, 2010-12-10 at 09:38 +0000, Mihir Nanavati > wrote: > > > > > > > > > On Fri, Dec 10, 2010 at 1:07 AM, Ian Campbell > > > > > > > > For future flexibility should we consider > passing a > > flags > > > argument and defining "XS_OPEN_READONLY > 1<<0" > > instead of > > > having an ro argument? > > > > > > Sure, we could do it, but I''m not too sure what > other modes > > we could > > > have for opening, let alone ones that might be > used > > simultaneously in > > > a bit field ;) > > > > > > There''s no downside to using a flag field now, even > if no > > compelling use > > cases come to mind right now and it might avoid an > API change > > in the > > future. > > > > One vague thought I had was that I recently added a > > "nonreentrant" flag > > to libxc for use in language bindings which like to > control > > threading > > themselves. Some sort of "no watches" flag might be > useful in > > the future > > for similar reasons. > > > > > I don''t suppose you feel like running sed > over the > > tree to > > > convert the > > > in tree users, do you ;-) > > > > > > > > > Could do, but I''d rather we get the interface > finalized > > first ;) > > > > > > Sure. > > > > > Is there anything one specially needs to take into > > consideration when > > > replacing them in the bindings? > > > > > > I can''t think of any -- try it and if it isn''t > obviously > > broken it''s > > probably fine ;-) > > > > Ian. > > > > > > > > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Mihir Nanavati
2010-Dec-10 11:10 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
Is this one ok? Thanks! ~M On Fri, Dec 10, 2010 at 2:45 AM, Ian Campbell <Ian.Campbell@eu.citrix.com>wrote:> Thanks but please put the deprecation comment in the header where > potential callers are mostly likely to see it. > > Tiny nitpick: it should be "if (...)" not "if(...)". > > On Fri, 2010-12-10 at 10:34 +0000, Mihir Nanavati wrote: > > Done. > > > > ~M > > > > On Fri, Dec 10, 2010 at 2:03 AM, Ian Campbell > > <Ian.Campbell@eu.citrix.com> wrote: > > On Fri, 2010-12-10 at 09:55 +0000, Mihir Nanavati wrote: > > > Fair enough - is this something like what you had in mind? > > > > > > Almost. You don''t need two bits to encode the boolean > > writeable property > > -- I reckon should just ditch XS_OPEN_READWRITE since its the > > default > > and equivalent to the absence of XS_OPEN_READONLY. The common > > case > > should be to pass flags == 0 and get a read+write connection. > > > > Ian. > > > > > > > > > > ~M > > > > > > On Fri, Dec 10, 2010 at 1:48 AM, Ian Campbell > > > <Ian.Campbell@eu.citrix.com> wrote: > > > On Fri, 2010-12-10 at 09:38 +0000, Mihir Nanavati > > wrote: > > > > > > > > > > > > On Fri, Dec 10, 2010 at 1:07 AM, Ian Campbell > > > > > > > > > > > For future flexibility should we consider > > passing a > > > flags > > > > argument and defining "XS_OPEN_READONLY > > 1<<0" > > > instead of > > > > having an ro argument? > > > > > > > > Sure, we could do it, but I''m not too sure what > > other modes > > > we could > > > > have for opening, let alone ones that might be > > used > > > simultaneously in > > > > a bit field ;) > > > > > > > > > There''s no downside to using a flag field now, even > > if no > > > compelling use > > > cases come to mind right now and it might avoid an > > API change > > > in the > > > future. > > > > > > One vague thought I had was that I recently added a > > > "nonreentrant" flag > > > to libxc for use in language bindings which like to > > control > > > threading > > > themselves. Some sort of "no watches" flag might be > > useful in > > > the future > > > for similar reasons. > > > > > > > I don''t suppose you feel like running sed > > over the > > > tree to > > > > convert the > > > > in tree users, do you ;-) > > > > > > > > > > > > Could do, but I''d rather we get the interface > > finalized > > > first ;) > > > > > > > > > Sure. > > > > > > > Is there anything one specially needs to take into > > > consideration when > > > > replacing them in the bindings? > > > > > > > > > I can''t think of any -- try it and if it isn''t > > obviously > > > broken it''s > > > probably fine ;-) > > > > > > Ian. > > > > > > > > > > > > > > > > > > > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-10 11:24 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
On Fri, 2010-12-10 at 11:10 +0000, Mihir Nanavati wrote:> Is this one ok? Thanks!The way the API information is now presented in xs.h isn''t that orderly or clear on what is deprecated. I think it would be better to add "deprecated please use xs_open()" to each to the comment blocks before the deprecated functions and to put xs_open and xs_close before those functions, with a suitable comment block describing their use.> > ~M > > On Fri, Dec 10, 2010 at 2:45 AM, Ian Campbell > <Ian.Campbell@eu.citrix.com> wrote: > Thanks but please put the deprecation comment in the header > where > potential callers are mostly likely to see it. > > Tiny nitpick: it should be "if (...)" not "if(...)". > > > On Fri, 2010-12-10 at 10:34 +0000, Mihir Nanavati wrote: > > Done. > > > > ~M > > > > On Fri, Dec 10, 2010 at 2:03 AM, Ian Campbell > > <Ian.Campbell@eu.citrix.com> wrote: > > On Fri, 2010-12-10 at 09:55 +0000, Mihir Nanavati > wrote: > > > Fair enough - is this something like what you had > in mind? > > > > > > Almost. You don''t need two bits to encode the > boolean > > writeable property > > -- I reckon should just ditch XS_OPEN_READWRITE > since its the > > default > > and equivalent to the absence of XS_OPEN_READONLY. > The common > > case > > should be to pass flags == 0 and get a read+write > connection. > > > > Ian. > > > > > > > > > > ~M > > > > > > On Fri, Dec 10, 2010 at 1:48 AM, Ian Campbell > > > <Ian.Campbell@eu.citrix.com> wrote: > > > On Fri, 2010-12-10 at 09:38 +0000, Mihir > Nanavati > > wrote: > > > > > > > > > > > > On Fri, Dec 10, 2010 at 1:07 AM, Ian > Campbell > > > > > > > > > > > For future flexibility should we > consider > > passing a > > > flags > > > > argument and defining > "XS_OPEN_READONLY > > 1<<0" > > > instead of > > > > having an ro argument? > > > > > > > > Sure, we could do it, but I''m not too > sure what > > other modes > > > we could > > > > have for opening, let alone ones that > might be > > used > > > simultaneously in > > > > a bit field ;) > > > > > > > > > There''s no downside to using a flag field > now, even > > if no > > > compelling use > > > cases come to mind right now and it might > avoid an > > API change > > > in the > > > future. > > > > > > One vague thought I had was that I > recently added a > > > "nonreentrant" flag > > > to libxc for use in language bindings > which like to > > control > > > threading > > > themselves. Some sort of "no watches" flag > might be > > useful in > > > the future > > > for similar reasons. > > > > > > > I don''t suppose you feel like > running sed > > over the > > > tree to > > > > convert the > > > > in tree users, do you ;-) > > > > > > > > > > > > Could do, but I''d rather we get the > interface > > finalized > > > first ;) > > > > > > > > > Sure. > > > > > > > Is there anything one specially needs to > take into > > > consideration when > > > > replacing them in the bindings? > > > > > > > > > I can''t think of any -- try it and if it > isn''t > > obviously > > > broken it''s > > > probably fine ;-) > > > > > > Ian. > > > > > > > > > > > > > > > > > > > > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Mihir Nanavati
2010-Dec-10 11:39 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
Sorry, that was rather confusing ;) Hope this reads better. ~M On Fri, Dec 10, 2010 at 3:24 AM, Ian Campbell <Ian.Campbell@eu.citrix.com>wrote:> On Fri, 2010-12-10 at 11:10 +0000, Mihir Nanavati wrote: > > Is this one ok? Thanks! > > The way the API information is now presented in xs.h isn''t that orderly > or clear on what is deprecated. I think it would be better to add > "deprecated please use xs_open()" to each to the comment blocks before > the deprecated functions and to put xs_open and xs_close before those > functions, with a suitable comment block describing their use. > > > > > ~M > > > > On Fri, Dec 10, 2010 at 2:45 AM, Ian Campbell > > <Ian.Campbell@eu.citrix.com> wrote: > > Thanks but please put the deprecation comment in the header > > where > > potential callers are mostly likely to see it. > > > > Tiny nitpick: it should be "if (...)" not "if(...)". > > > > > > On Fri, 2010-12-10 at 10:34 +0000, Mihir Nanavati wrote: > > > Done. > > > > > > ~M > > > > > > On Fri, Dec 10, 2010 at 2:03 AM, Ian Campbell > > > <Ian.Campbell@eu.citrix.com> wrote: > > > On Fri, 2010-12-10 at 09:55 +0000, Mihir Nanavati > > wrote: > > > > Fair enough - is this something like what you had > > in mind? > > > > > > > > > Almost. You don''t need two bits to encode the > > boolean > > > writeable property > > > -- I reckon should just ditch XS_OPEN_READWRITE > > since its the > > > default > > > and equivalent to the absence of XS_OPEN_READONLY. > > The common > > > case > > > should be to pass flags == 0 and get a read+write > > connection. > > > > > > Ian. > > > > > > > > > > > > > > ~M > > > > > > > > On Fri, Dec 10, 2010 at 1:48 AM, Ian Campbell > > > > <Ian.Campbell@eu.citrix.com> wrote: > > > > On Fri, 2010-12-10 at 09:38 +0000, Mihir > > Nanavati > > > wrote: > > > > > > > > > > > > > > > On Fri, Dec 10, 2010 at 1:07 AM, Ian > > Campbell > > > > > > > > > > > > > > For future flexibility should we > > consider > > > passing a > > > > flags > > > > > argument and defining > > "XS_OPEN_READONLY > > > 1<<0" > > > > instead of > > > > > having an ro argument? > > > > > > > > > > Sure, we could do it, but I''m not too > > sure what > > > other modes > > > > we could > > > > > have for opening, let alone ones that > > might be > > > used > > > > simultaneously in > > > > > a bit field ;) > > > > > > > > > > > > There''s no downside to using a flag field > > now, even > > > if no > > > > compelling use > > > > cases come to mind right now and it might > > avoid an > > > API change > > > > in the > > > > future. > > > > > > > > One vague thought I had was that I > > recently added a > > > > "nonreentrant" flag > > > > to libxc for use in language bindings > > which like to > > > control > > > > threading > > > > themselves. Some sort of "no watches" flag > > might be > > > useful in > > > > the future > > > > for similar reasons. > > > > > > > > > I don''t suppose you feel like > > running sed > > > over the > > > > tree to > > > > > convert the > > > > > in tree users, do you ;-) > > > > > > > > > > > > > > > Could do, but I''d rather we get the > > interface > > > finalized > > > > first ;) > > > > > > > > > > > > Sure. > > > > > > > > > Is there anything one specially needs to > > take into > > > > consideration when > > > > > replacing them in the bindings? > > > > > > > > > > > > I can''t think of any -- try it and if it > > isn''t > > > obviously > > > > broken it''s > > > > probably fine ;-) > > > > > > > > Ian. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Dec-10 11:52 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
On Fri, 2010-12-10 at 11:39 +0000, Mihir Nanavati wrote:> Sorry, that was rather confusing ;) Hope this reads better.Much, thanks! I think "int mode" should be "unsigned long flags" but don''t feel that strongly so: Acked-by: Ian Campbell <ian.campbell@citrix.com> (If you are submitting as an attachment I think IanJ/Stefano will prefer if you duplicate the changelog in the attachement as well as in the body so they don''t need to reconstruct the commit) Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Dec-10 17:28 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: Use xenbus to
communicate with xenstore if the socket fails"):> The current driver seems to use wait_event_interruptible and attempt to
> do something sane looking with O_NONBLOCK on read but on write looks
> like it may end up waiting for a reply forever if xenstored has gone
> away, there''s even a "FIXME avoid synchronous wait for
response" in the
> code.
However, given the plan to try the socket first, I think it would be
best to go ahead and hide this distinction inside libxenstore even
while that bug remains unfixed.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2010-Dec-10 17:49 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails
Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: Use xenbus to
communicate with xenstore if the socket fails"):> On Fri, 2010-12-10 at 11:39 +0000, Mihir Nanavati wrote:
> > Sorry, that was rather confusing ;) Hope this reads better.
Thanks for the patch.
> Much, thanks!
>
> I think "int mode" should be "unsigned long flags" but
don''t feel that
> strongly so:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
It should be unsigned of some kind, surely, and you''re right about the
name I''ll fix that up.
> (If you are submitting as an attachment I think IanJ/Stefano will prefer
> if you duplicate the changelog in the attachement as well as in the body
> so they don''t need to reconstruct the commit)
In general, I would definitely prefer it if revised patches contained
a fresh copy of the original changelog comment, with a notes about
what changed, if applicable. That saves going back in the thread
trying to find which message has the changelog comment and trying to
decide whether the old comment still applies. I''m not that bothered
about the exact format.
In this case I think the original comment from the top of the thread
will do.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Dec-13 17:19 UTC
Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket failsx
On Fri, 10 Dec 2010, Ian Jackson wrote:> Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails"): > > On Fri, 2010-12-10 at 11:39 +0000, Mihir Nanavati wrote: > > > Sorry, that was rather confusing ;) Hope this reads better. > > Thanks for the patch. > > > Much, thanks! > > > > I think "int mode" should be "unsigned long flags" but don''t feel that > > strongly so: > > Acked-by: Ian Campbell <ian.campbell@citrix.com> > > It should be unsigned of some kind, surely, and you''re right about the > name I''ll fix that up.I applied the patch changing int mode into unsigned long flags, like Ian suggested. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel