[PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Mihir Nanavati]

[-- Attachment #1.1: Type: text/plain, Size: 198 bytes --]

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>

[-- Attachment #1.2: Type: text/html, Size: 773 bytes --]

[-- Attachment #2: libxl_domain.patch --]
[-- Type: text/x-patch, Size: 2948 bytes --]

diff -r f5d6afa46dd7 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Tue Dec 07 18:10:46 2010 +0000
+++ b/tools/libxl/libxl.c	Wed Dec 08 12:32:04 2010 -0800
@@ -1418,11 +1418,11 @@
         goto out;
 
     /* we mustn't use the parent's handle in the child */
-    xsh = xs_daemon_open();
+    xsh = libxl__xs_open();
 
     xs_write(xsh, XBT_NULL, path, pid, len);
 
-    xs_daemon_close(xsh);
+    libxl__xs_close(xsh);
 out:
     free(path);
     free(pid);
diff -r f5d6afa46dd7 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Tue Dec 07 18:10:46 2010 +0000
+++ b/tools/libxl/libxl_device.c	Wed Dec 08 12:32:04 2010 -0800
@@ -428,7 +428,7 @@
     unsigned int num;
     char **l = NULL;
 
-    xsh = xs_daemon_open();
+    xsh = libxl__xs_open(); 
     path = libxl__sprintf(&gc, "/local/domain/0/device-model/%d/state", domid);
     xs_watch(xsh, path, path);
     tv.tv_sec = LIBXL_DEVICE_MODEL_START_TIMEOUT;
@@ -450,7 +450,7 @@
 
         free(p);
         xs_unwatch(xsh, path, path);
-        xs_daemon_close(xsh);
+        libxl__xs_close(xsh);
         libxl__free_all(&gc);
         return rc;
 again:
@@ -467,7 +467,7 @@
         }
     }
     xs_unwatch(xsh, path, path);
-    xs_daemon_close(xsh);
+    libxl__xs_close(xsh);
     LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Device Model not ready");
     libxl__free_all(&gc);
     return -1;
diff -r f5d6afa46dd7 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Tue Dec 07 18:10:46 2010 +0000
+++ b/tools/libxl/libxl_internal.h	Wed Dec 08 12:32:04 2010 -0800
@@ -140,6 +140,8 @@
 _hidden char *libxl__xs_get_dompath(libxl__gc *gc, uint32_t domid); // logs errs
 _hidden char *libxl__xs_read(libxl__gc *gc, xs_transaction_t t, char *path);
 _hidden char **libxl__xs_directory(libxl__gc *gc, xs_transaction_t t, char *path, unsigned int *nb);
+_hidden struct xs_handle *libxl__xs_open(void);
+_hidden void libxl__xs_close(struct xs_handle *xsh);
 
 /* from xl_dom */
 _hidden int libxl__domain_is_hvm(libxl_ctx *ctx, uint32_t domid);
diff -r f5d6afa46dd7 tools/libxl/libxl_utils.c
--- a/tools/libxl/libxl_utils.c	Tue Dec 07 18:10:46 2010 +0000
+++ b/tools/libxl/libxl_utils.c	Wed Dec 08 12:32:04 2010 -0800
@@ -399,7 +399,7 @@
 
 int libxl_ctx_postfork(libxl_ctx *ctx) {
     if (ctx->xsh) xs_daemon_destroy_postfork(ctx->xsh);
-    ctx->xsh = xs_daemon_open();
+    ctx->xsh = libxl__xs_open();
     if (!ctx->xsh) return ERROR_FAIL;
     return 0;
 }
diff -r f5d6afa46dd7 tools/libxl/libxl_xshelp.c
--- a/tools/libxl/libxl_xshelp.c	Tue Dec 07 18:10:46 2010 +0000
+++ b/tools/libxl/libxl_xshelp.c	Wed Dec 08 12:32:04 2010 -0800
@@ -121,3 +121,19 @@
     libxl__ptr_add(gc, ret);
     return ret;
 }
+
+struct xs_handle *libxl__xs_open(void)
+{
+    struct xs_handle* xsh = NULL;
+    
+    xsh = xs_daemon_open();
+    if ( !xsh )
+        xsh = xs_domain_open();
+
+    return xsh;
+}
+
+void libxl__xs_close(struct xs_handle *xsh)
+{
+    xs_daemon_close(xsh);
+}

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Ian Campbell]

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>

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Mihir Nanavati]

[-- Attachment #1.1: Type: text/plain, Size: 1239 bytes --]

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>
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 1723 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Ian Campbell]

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>
>         
>         
>         
> 

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Vincent Hanquez]

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

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Ian Campbell]

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.

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Vincent Hanquez]

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

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Ian Campbell]

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.

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Mihir Nanavati]

[-- Attachment #1.1: Type: text/plain, Size: 1247 bytes --]

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.
>
>

[-- Attachment #1.2: Type: text/html, Size: 1746 bytes --]

[-- Attachment #2: libxenstore_domain.patch --]
[-- Type: text/x-patch, Size: 1922 bytes --]

diff -r 89116f28083f tools/xenstore/xs.c
--- a/tools/xenstore/xs.c	Wed Dec 08 10:46:31 2010 +0000
+++ b/tools/xenstore/xs.c	Thu Dec 09 22:48:18 2010 -0800
@@ -182,12 +182,15 @@
 	return -1;
 }
 
-static int get_dev(const char *connect_to)
+static int get_dev(const char *connect_to, int ro)
 {
-	return open(connect_to, O_RDWR);
+	if (!ro)
+		return open(connect_to, O_RDWR);
+	else
+		return open(connect_to, O_RDONLY);
 }
 
-static struct xs_handle *get_handle(const char *connect_to)
+static struct xs_handle *get_handle(const char *connect_to, int ro)
 {
 	struct stat buf;
 	struct xs_handle *h = NULL;
@@ -199,7 +202,7 @@
 	if (S_ISSOCK(buf.st_mode))
 		fd = get_socket(connect_to);
 	else
-		fd = get_dev(connect_to);
+		fd = get_dev(connect_to, ro);
 
 	if (fd == -1)
 		return NULL;
@@ -237,17 +240,37 @@
 
 struct xs_handle *xs_daemon_open(void)
 {
-	return get_handle(xs_daemon_socket());
+	return xs_open(0);
 }
 
 struct xs_handle *xs_daemon_open_readonly(void)
 {
-	return get_handle(xs_daemon_socket_ro());
+	return xs_open(1);
 }
 
 struct xs_handle *xs_domain_open(void)
 {
-	return get_handle(xs_domain_dev());
+	return xs_open(0);
+}
+
+struct xs_handle *xs_open(int ro)
+{
+	struct xs_handle *xsh = NULL;
+
+	if(!ro) {
+		xsh = get_handle(xs_daemon_socket(), 0);
+
+		if(!xsh)
+			xsh = get_handle(xs_domain_dev(), 0);
+	}
+	else {
+		xsh = get_handle(xs_daemon_socket_ro(), 1);
+
+		if(!xsh)
+			xsh = get_handle(xs_domain_dev(), 1);
+	}
+
+	return xsh;
 }
 
 static void close_free_msgs(struct xs_handle *h) {
diff -r 89116f28083f tools/xenstore/xs.h
--- a/tools/xenstore/xs.h	Wed Dec 08 10:46:31 2010 +0000
+++ b/tools/xenstore/xs.h	Thu Dec 09 22:48:18 2010 -0800
@@ -39,6 +39,7 @@
  */
 struct xs_handle *xs_daemon_open(void);
 struct xs_handle *xs_domain_open(void);
+struct xs_handle *xs_open(int ro);
 
 /* Connect to the xs daemon (readonly for non-root clients).
  * Returns a handle or NULL.

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Ian Campbell]

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.
>         
> 
> 

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Mihir Nanavati]

[-- Attachment #1.1: Type: text/plain, Size: 2864 bytes --]

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.
> >
> >
> >
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 4435 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Ian Campbell]

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.

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Mihir Nanavati]

[-- Attachment #1.1: Type: text/plain, Size: 1459 bytes --]

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.
>
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 2101 bytes --]

[-- Attachment #2: libxenstore_domain.patch --]
[-- Type: text/x-diff, Size: 2661 bytes --]

diff -r 89116f28083f tools/xenstore/xs.c
--- a/tools/xenstore/xs.c	Wed Dec 08 10:46:31 2010 +0000
+++ b/tools/xenstore/xs.c	Fri Dec 10 01:54:53 2010 -0800
@@ -182,12 +182,17 @@
 	return -1;
 }
 
-static int get_dev(const char *connect_to)
+static int get_dev(const char *connect_to, int mode)
 {
-	return open(connect_to, O_RDWR);
+	if (mode & XS_OPEN_READWRITE)
+		return open(connect_to, O_RDWR);
+	else if(mode & XS_OPEN_READONLY)
+		return open(connect_to, O_RDONLY);
+	else
+		return -1;
 }
 
-static struct xs_handle *get_handle(const char *connect_to)
+static struct xs_handle *get_handle(const char *connect_to, int mode)
 {
 	struct stat buf;
 	struct xs_handle *h = NULL;
@@ -199,7 +204,7 @@
 	if (S_ISSOCK(buf.st_mode))
 		fd = get_socket(connect_to);
 	else
-		fd = get_dev(connect_to);
+		fd = get_dev(connect_to, mode);
 
 	if (fd == -1)
 		return NULL;
@@ -235,19 +240,41 @@
 	return h;
 }
 
+/*
+ * The following three functions have been deprecated and are
+ * maintained as aliases to xs_open for legacy code. Newer code
+ * should use xs_open directly
+ */
 struct xs_handle *xs_daemon_open(void)
 {
-	return get_handle(xs_daemon_socket());
+	return xs_open(XS_OPEN_READWRITE);
 }
 
 struct xs_handle *xs_daemon_open_readonly(void)
 {
-	return get_handle(xs_daemon_socket_ro());
+	return xs_open(XS_OPEN_READONLY);
 }
 
 struct xs_handle *xs_domain_open(void)
 {
-	return get_handle(xs_domain_dev());
+	return xs_open(XS_OPEN_READWRITE);
+}
+
+struct xs_handle *xs_open(int mode)
+{
+	struct xs_handle *xsh = NULL;
+
+	if(mode & XS_OPEN_READWRITE)
+		xsh = get_handle(xs_daemon_socket(), mode);
+	else if(mode & XS_OPEN_READONLY)
+		xsh = get_handle(xs_daemon_socket_ro(), mode);
+	else
+		return NULL;
+
+	if(!xsh)
+		xsh = get_handle(xs_domain_dev(), mode);
+
+	return xsh;
 }
 
 static void close_free_msgs(struct xs_handle *h) {
@@ -303,6 +330,12 @@
         close_fds_free(h);
 }
 
+void xs_close(struct xs_handle* xsh)
+{
+	if(xsh)
+		xs_daemon_close(xsh);
+}
+
 static bool read_all(int fd, void *data, unsigned int len)
 {
 	while (len) {
diff -r 89116f28083f tools/xenstore/xs.h
--- a/tools/xenstore/xs.h	Wed Dec 08 10:46:31 2010 +0000
+++ b/tools/xenstore/xs.h	Fri Dec 10 01:54:53 2010 -0800
@@ -24,6 +24,9 @@
 
 #define XBT_NULL 0
 
+#define XS_OPEN_READWRITE	1<<0
+#define XS_OPEN_READONLY	1<<1
+
 struct xs_handle;
 typedef uint32_t xs_transaction_t;
 
@@ -39,6 +42,8 @@
  */
 struct xs_handle *xs_daemon_open(void);
 struct xs_handle *xs_domain_open(void);
+struct xs_handle *xs_open(int mode);
+void xs_close(struct xs_handle *xsh);
 
 /* Connect to the xs daemon (readonly for non-root clients).
  * Returns a handle or NULL.

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Ian Campbell]

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.
>         
>         
>         
> 

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Mihir Nanavati]

[-- Attachment #1.1: Type: text/plain, Size: 2436 bytes --]

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.
> >
> >
> >
> >
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 3409 bytes --]

[-- Attachment #2: libxenstore_domain.patch --]
[-- Type: text/x-diff, Size: 2494 bytes --]

diff -r 89116f28083f tools/xenstore/xs.c
--- a/tools/xenstore/xs.c	Wed Dec 08 10:46:31 2010 +0000
+++ b/tools/xenstore/xs.c	Fri Dec 10 02:33:42 2010 -0800
@@ -182,12 +182,15 @@
 	return -1;
 }
 
-static int get_dev(const char *connect_to)
+static int get_dev(const char *connect_to, int mode)
 {
-	return open(connect_to, O_RDWR);
+	if(mode & XS_OPEN_READONLY)
+		return open(connect_to, O_RDONLY);
+	else
+		return open(connect_to, O_RDWR);
 }
 
-static struct xs_handle *get_handle(const char *connect_to)
+static struct xs_handle *get_handle(const char *connect_to, int mode)
 {
 	struct stat buf;
 	struct xs_handle *h = NULL;
@@ -199,7 +202,7 @@
 	if (S_ISSOCK(buf.st_mode))
 		fd = get_socket(connect_to);
 	else
-		fd = get_dev(connect_to);
+		fd = get_dev(connect_to, mode);
 
 	if (fd == -1)
 		return NULL;
@@ -235,19 +238,39 @@
 	return h;
 }
 
+/*
+ * The following three functions have been deprecated and are
+ * maintained as aliases to xs_open for legacy code. Newer code
+ * should use xs_open directly
+ */
 struct xs_handle *xs_daemon_open(void)
 {
-	return get_handle(xs_daemon_socket());
+	return xs_open(0);
 }
 
 struct xs_handle *xs_daemon_open_readonly(void)
 {
-	return get_handle(xs_daemon_socket_ro());
+	return xs_open(XS_OPEN_READONLY);
 }
 
 struct xs_handle *xs_domain_open(void)
 {
-	return get_handle(xs_domain_dev());
+	return xs_open(0);
+}
+
+struct xs_handle *xs_open(int mode)
+{
+	struct xs_handle *xsh = NULL;
+
+	if(mode & XS_OPEN_READONLY)
+		xsh = get_handle(xs_daemon_socket_ro(), mode);
+	else
+		xsh = get_handle(xs_daemon_socket(), mode);
+
+	if(!xsh)
+		xsh = get_handle(xs_domain_dev(), mode);
+
+	return xsh;
 }
 
 static void close_free_msgs(struct xs_handle *h) {
@@ -303,6 +326,12 @@
         close_fds_free(h);
 }
 
+void xs_close(struct xs_handle* xsh)
+{
+	if(xsh)
+		xs_daemon_close(xsh);
+}
+
 static bool read_all(int fd, void *data, unsigned int len)
 {
 	while (len) {
diff -r 89116f28083f tools/xenstore/xs.h
--- a/tools/xenstore/xs.h	Wed Dec 08 10:46:31 2010 +0000
+++ b/tools/xenstore/xs.h	Fri Dec 10 02:33:42 2010 -0800
@@ -24,6 +24,8 @@
 
 #define XBT_NULL 0
 
+#define XS_OPEN_READONLY	1<<0
+
 struct xs_handle;
 typedef uint32_t xs_transaction_t;
 
@@ -39,6 +41,8 @@
  */
 struct xs_handle *xs_daemon_open(void);
 struct xs_handle *xs_domain_open(void);
+struct xs_handle *xs_open(int mode);
+void xs_close(struct xs_handle *xsh);
 
 /* Connect to the xs daemon (readonly for non-root clients).
  * Returns a handle or NULL.

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Ian Campbell]

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.
>         >
>         >
>         >
>         >
>         
>         
>         
> 

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Mihir Nanavati]

[-- Attachment #1.1: Type: text/plain, Size: 3724 bytes --]

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.
> >         >
> >         >
> >         >
> >         >
> >
> >
> >
> >
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 5121 bytes --]

[-- Attachment #2: libxenstore_domain.patch --]
[-- Type: text/x-diff, Size: 2587 bytes --]

diff -r 89116f28083f tools/xenstore/xs.c
--- a/tools/xenstore/xs.c	Wed Dec 08 10:46:31 2010 +0000
+++ b/tools/xenstore/xs.c	Fri Dec 10 03:05:06 2010 -0800
@@ -182,12 +182,15 @@
 	return -1;
 }
 
-static int get_dev(const char *connect_to)
+static int get_dev(const char *connect_to, int mode)
 {
-	return open(connect_to, O_RDWR);
+	if (mode & XS_OPEN_READONLY)
+		return open(connect_to, O_RDONLY);
+	else
+		return open(connect_to, O_RDWR);
 }
 
-static struct xs_handle *get_handle(const char *connect_to)
+static struct xs_handle *get_handle(const char *connect_to, int mode)
 {
 	struct stat buf;
 	struct xs_handle *h = NULL;
@@ -199,7 +202,7 @@
 	if (S_ISSOCK(buf.st_mode))
 		fd = get_socket(connect_to);
 	else
-		fd = get_dev(connect_to);
+		fd = get_dev(connect_to, mode);
 
 	if (fd == -1)
 		return NULL;
@@ -237,17 +240,32 @@
 
 struct xs_handle *xs_daemon_open(void)
 {
-	return get_handle(xs_daemon_socket());
+	return xs_open(0);
 }
 
 struct xs_handle *xs_daemon_open_readonly(void)
 {
-	return get_handle(xs_daemon_socket_ro());
+	return xs_open(XS_OPEN_READONLY);
 }
 
 struct xs_handle *xs_domain_open(void)
 {
-	return get_handle(xs_domain_dev());
+	return xs_open(0);
+}
+
+struct xs_handle *xs_open(int mode)
+{
+	struct xs_handle *xsh = NULL;
+
+	if (mode & XS_OPEN_READONLY)
+		xsh = get_handle(xs_daemon_socket_ro(), mode);
+	else
+		xsh = get_handle(xs_daemon_socket(), mode);
+
+	if (!xsh)
+		xsh = get_handle(xs_domain_dev(), mode);
+
+	return xsh;
 }
 
 static void close_free_msgs(struct xs_handle *h) {
@@ -303,6 +321,12 @@
         close_fds_free(h);
 }
 
+void xs_close(struct xs_handle* xsh)
+{
+	if (xsh)
+		xs_daemon_close(xsh);
+}
+
 static bool read_all(int fd, void *data, unsigned int len)
 {
 	while (len) {
diff -r 89116f28083f tools/xenstore/xs.h
--- a/tools/xenstore/xs.h	Wed Dec 08 10:46:31 2010 +0000
+++ b/tools/xenstore/xs.h	Fri Dec 10 03:05:06 2010 -0800
@@ -24,6 +24,8 @@
 
 #define XBT_NULL 0
 
+#define XS_OPEN_READONLY	1<<0
+
 struct xs_handle;
 typedef uint32_t xs_transaction_t;
 
@@ -34,6 +36,12 @@
 
 /* On failure, these routines set errno. */
 
+/*
+ * The following three functions have been deprecated and are
+ * maintained as aliases to xs_open for legacy code. Newer code
+ * should use xs_open directly
+ */
+
 /* Connect to the xs daemon.
  * Returns a handle or NULL.
  */
@@ -45,6 +53,9 @@
  */
 struct xs_handle *xs_daemon_open_readonly(void);
 
+struct xs_handle *xs_open(int mode);
+void xs_close(struct xs_handle *xsh);
+
 /* Close the connection to the xs daemon. */
 void xs_daemon_close(struct xs_handle *);
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Ian Campbell]

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.
>         >         >
>         >         >
>         >         >
>         >         >
>         >
>         >
>         >
>         >
>         
>         
>         
> 

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Mihir Nanavati]

[-- Attachment #1.1: Type: text/plain, Size: 5646 bytes --]

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.
> >         >         >
> >         >         >
> >         >         >
> >         >         >
> >         >
> >         >
> >         >
> >         >
> >
> >
> >
> >
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 7676 bytes --]

[-- Attachment #2: libxenstore_domain.patch --]
[-- Type: text/x-diff, Size: 3171 bytes --]

diff -r 89116f28083f tools/xenstore/xs.c
--- a/tools/xenstore/xs.c	Wed Dec 08 10:46:31 2010 +0000
+++ b/tools/xenstore/xs.c	Fri Dec 10 03:39:56 2010 -0800
@@ -182,12 +182,15 @@
 	return -1;
 }
 
-static int get_dev(const char *connect_to)
+static int get_dev(const char *connect_to, int mode)
 {
-	return open(connect_to, O_RDWR);
+	if (mode & XS_OPEN_READONLY)
+		return open(connect_to, O_RDONLY);
+	else
+		return open(connect_to, O_RDWR);
 }
 
-static struct xs_handle *get_handle(const char *connect_to)
+static struct xs_handle *get_handle(const char *connect_to, int mode)
 {
 	struct stat buf;
 	struct xs_handle *h = NULL;
@@ -199,7 +202,7 @@
 	if (S_ISSOCK(buf.st_mode))
 		fd = get_socket(connect_to);
 	else
-		fd = get_dev(connect_to);
+		fd = get_dev(connect_to, mode);
 
 	if (fd == -1)
 		return NULL;
@@ -237,17 +240,32 @@
 
 struct xs_handle *xs_daemon_open(void)
 {
-	return get_handle(xs_daemon_socket());
+	return xs_open(0);
 }
 
 struct xs_handle *xs_daemon_open_readonly(void)
 {
-	return get_handle(xs_daemon_socket_ro());
+	return xs_open(XS_OPEN_READONLY);
 }
 
 struct xs_handle *xs_domain_open(void)
 {
-	return get_handle(xs_domain_dev());
+	return xs_open(0);
+}
+
+struct xs_handle *xs_open(int mode)
+{
+	struct xs_handle *xsh = NULL;
+
+	if (mode & XS_OPEN_READONLY)
+		xsh = get_handle(xs_daemon_socket_ro(), mode);
+	else
+		xsh = get_handle(xs_daemon_socket(), mode);
+
+	if (!xsh)
+		xsh = get_handle(xs_domain_dev(), mode);
+
+	return xsh;
 }
 
 static void close_free_msgs(struct xs_handle *h) {
@@ -303,6 +321,12 @@
         close_fds_free(h);
 }
 
+void xs_close(struct xs_handle* xsh)
+{
+	if (xsh)
+		xs_daemon_close(xsh);
+}
+
 static bool read_all(int fd, void *data, unsigned int len)
 {
 	while (len) {
diff -r 89116f28083f tools/xenstore/xs.h
--- a/tools/xenstore/xs.h	Wed Dec 08 10:46:31 2010 +0000
+++ b/tools/xenstore/xs.h	Fri Dec 10 03:39:56 2010 -0800
@@ -24,6 +24,8 @@
 
 #define XBT_NULL 0
 
+#define XS_OPEN_READONLY	1<<0
+
 struct xs_handle;
 typedef uint32_t xs_transaction_t;
 
@@ -34,18 +36,34 @@
 
 /* On failure, these routines set errno. */
 
+/* Open a connection to the xs daemon.
+ * Attempts to make a connection over the socket interface, 
+ * and if it fails, then over the  xenbus interface.
+ * Mode 0 specifies read-write access, XS_OPEN_READONLY for
+ * read-only access.
+ * Returns a handle or NULL.
+ */
+struct xs_handle *xs_open(int mode);
+
+/* Close the connection to the xs daemon. */
+void xs_close(struct xs_handle *xsh);
+
 /* Connect to the xs daemon.
  * Returns a handle or NULL.
+ * Deprecated, please use xs_open(0) instead
  */
 struct xs_handle *xs_daemon_open(void);
 struct xs_handle *xs_domain_open(void);
 
 /* Connect to the xs daemon (readonly for non-root clients).
  * Returns a handle or NULL.
+ * Deprecated, please use xs_open(XS_OPEN_READONLY) instead
  */
 struct xs_handle *xs_daemon_open_readonly(void);
 
-/* Close the connection to the xs daemon. */
+/* Close the connection to the xs daemon.
+ * Deprecated, please use xs_close() instead
+ */
 void xs_daemon_close(struct xs_handle *);
 
 /* Throw away the connection to the xs daemon, for use after fork(). */

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Ian Campbell]

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.

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Ian Jackson]

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.

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket failsx

[Stefano Stabellini]

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.

Re: [PATCH] libxl: Use xenbus to communicate with xenstore if the socket fails

[Ian Jackson]

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.