[PATCH] Add an ioctl interface for simple xenstore access

[PATCH] Add an ioctl interface for simple xenstore access

[Dan Smith]

[-- Attachment #1.1.1: Type: text/plain, Size: 768 bytes --]

This patch adds an ioctl interface to /proc/xen/xenbus to allow simple
access to xenstore from domU.  This patch introduces only
xenbus_read() support, but write, remove, etc could be easily added.
Also, this interface can be easily moved to /dev/xen/xenbus (or
something similar) later.

The following snippet of a C program provides easy access to xenstore
nodes from inside a domU:

  #include <xen/io/xenbus.h>
  
  int main(int argc, char **argv)
  {
          int fd;
          int ret;
          struct xenbus_ioctl param;
  
          fd = open("/proc/xen/xenbus", O_RDWR);
  
          strcpy(param.path, argv[1]);
          ret = ioctl(fd, 0, &param);
  
          printf("%s\n", param.value);
  }

Comments welcome, of course ;)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: xenbus_ioctl.patch --]
[-- Type: text/x-patch, Size: 2735 bytes --]

# HG changeset patch
# User Dan Smith <danms@us.ibm.com>
# Node ID c01070e1dbed9f19098431fd82120960695f5240
# Parent  96ba0a2bc9de7da1c70a2528481f7448f3a9524d
Add a preliminary ioctl interface to xenbus.
This patch adds an ioctl interface to /proc/xen/xenbus to allow simple
access to xenstore from domU.  This patch introduces only xenbus_read()
support, but write, remove, etc could be easily added.  Also, this
interface can be easily moved to /dev/xen/xenbus (or something similar)
later.

Signed-off-by: Dan Smith <danms@us.ibm.com>

diff -r 96ba0a2bc9de -r c01070e1dbed linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_dev.c
--- a/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_dev.c	Wed Mar 15 13:35:43 2006 +0100
+++ b/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_dev.c	Fri Mar 17 14:27:23 2006 -0800
@@ -208,11 +208,63 @@ static int xenbus_dev_release(struct ino
 	return 0;
 }
 
+static int xenbus_dev_ioctl(struct inode *inode, struct file *filp,
+			    unsigned int cmd, unsigned long arg)
+{
+	void *output;
+	struct xenbus_ioctl *param;
+	unsigned int len;
+	int ret = 0;
+
+	if (!access_ok(VERIFY_WRITE, (void __user *)arg, sizeof(param))) 
+		return -EACCES;
+	
+	param = kmalloc(sizeof(*param), GFP_KERNEL);
+	if (param == NULL)
+		return -ENOMEM;
+
+	if (copy_from_user(param, (void *)arg, sizeof(*param))) {
+		ret = -EACCES;
+		goto out;
+	}
+	
+	param->path[XENBUS_IOCTL_ARG_LEN - 1] = '\0';
+
+	switch (cmd) {
+	case XEN_XENBUS_IOCTL_READ:
+		
+		output = (char *)xenbus_read(XBT_NULL, param->path, "", &len);
+		
+		if (IS_ERR(output)) {
+			ret = -EINVAL;
+			goto out;
+		}
+		
+		strncpy(param->value, output, XENBUS_IOCTL_ARG_LEN);
+		param->value_len = len;
+		break;
+
+	default:
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (copy_to_user((void*)arg, param, sizeof(*param))) {
+		ret = -EACCES;
+		goto out;
+	}
+
+ out:
+	kfree(param);
+	return ret;
+}
+
 static struct file_operations xenbus_dev_file_ops = {
 	.read = xenbus_dev_read,
 	.write = xenbus_dev_write,
 	.open = xenbus_dev_open,
 	.release = xenbus_dev_release,
+	.ioctl = xenbus_dev_ioctl,
 };
 
 static int __init
diff -r 96ba0a2bc9de -r c01070e1dbed xen/include/public/io/xenbus.h
--- a/xen/include/public/io/xenbus.h	Wed Mar 15 13:35:43 2006 +0100
+++ b/xen/include/public/io/xenbus.h	Fri Mar 17 14:27:23 2006 -0800
@@ -29,6 +29,18 @@ typedef enum
 
 } XenbusState;
 
+/*
+ * XenBus ioctl support
+ */
+#define XENBUS_IOCTL_ARG_LEN 256
+struct xenbus_ioctl {
+	char path[XENBUS_IOCTL_ARG_LEN];
+	char value[XENBUS_IOCTL_ARG_LEN];
+	uint32_t value_len;
+};
+
+#define XEN_XENBUS_IOCTL_READ 0
+
 #endif /* _XEN_PUBLIC_IO_XENBUS_H */
 
 /*

[-- Attachment #1.1.3: Type: text/plain, Size: 92 bytes --]

-- 
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms@us.ibm.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 190 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] Add an ioctl interface for simple xenstore access

[Muli Ben-Yehuda]

On Fri, Mar 17, 2006 at 02:33:36PM -0800, Dan Smith wrote:

> This patch adds an ioctl interface to /proc/xen/xenbus to allow simple
> access to xenstore from domU.  This patch introduces only
> xenbus_read() support, but write, remove, etc could be easily added.
> Also, this interface can be easily moved to /dev/xen/xenbus (or
> something similar) later.

Hmm, wouldn't a virtual file system be a much better fit for xenstore
access? the kernel community considers ioctls Evil with a capital E.

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

Re: [PATCH] Add an ioctl interface for simple xenstore access

[Keir Fraser]

On 18 Mar 2006, at 07:14, Muli Ben-Yehuda wrote:

>> This patch adds an ioctl interface to /proc/xen/xenbus to allow simple
>> access to xenstore from domU.  This patch introduces only
>> xenbus_read() support, but write, remove, etc could be easily added.
>> Also, this interface can be easily moved to /dev/xen/xenbus (or
>> something similar) later.
>
> Hmm, wouldn't a virtual file system be a much better fit for xenstore
> access? the kernel community considers ioctls Evil with a capital E.

Also, we already support xenstore access via read/write on that device 
file. And libxenstore knows how to target that read/write interface. 
Why would we add an ioctl to do the same thing?

  -- Keir

Re: [PATCH] Add an ioctl interface for simple xenstore access

[Anthony Liguori]

Muli Ben-Yehuda wrote:
> On Fri, Mar 17, 2006 at 02:33:36PM -0800, Dan Smith wrote:
>
>   
>> This patch adds an ioctl interface to /proc/xen/xenbus to allow simple
>> access to xenstore from domU.  This patch introduces only
>> xenbus_read() support, but write, remove, etc could be easily added.
>> Also, this interface can be easily moved to /dev/xen/xenbus (or
>> something similar) later.
>>     
>
> Hmm, wouldn't a virtual file system be a much better fit for xenstore
> access? the kernel community considers ioctls Evil with a capital E.
>   
I'm not sure how one would express the transaction semantics of xenstore 
via a virtual filesystem.  There is also an awkward mapping between some 
of the attributes of xenstore (for instance, there's no way to determine 
if an attribute returned by a list is a directory or not) that makes a 
filesystem non-trivial.

If you remember, I had written a fuse filesystem front-end for xenstore 
at one point.  It's definitely neat, but I think we would have to make 
some changes to Xenstore for it to really work out well.

Regards,

Anthony Liguori

> Cheers,
> Muli
>   

Re: [PATCH] Add an ioctl interface for simple xenstore access

[Anthony Liguori]

Keir Fraser wrote:
>> Hmm, wouldn't a virtual file system be a much better fit for xenstore
>> access? the kernel community considers ioctls Evil with a capital E.
>
> Also, we already support xenstore access via read/write on that device 
> file. And libxenstore knows how to target that read/write interface. 
> Why would we add an ioctl to do the same thing?
Hi Keir,

We had discussed this in a previous thread and I thought you were okay 
with this approach.  The /proc/xen/xenbus interface requires full 
parsing of the XenBus protocol.  This requires libxenstore in domU.  
Today, libxenstore is shipped with the rest of the tools package on most 
distros.  If one wants to write a Xen-aware application for a domU, that 
brings in a pretty large number of unnecessary dependencies.  Also, for 
32 bit management apps in a 64 bit environment, it makes things very 
nasty (should we build a 32 bit and 64 bit version of libxenstore?  Is 
the protocol 32/64 bit safe?).

Ideally, an app could just use a simple interface to /proc/xen/xenbus to 
access XenStore.  That solves all of the above problems elegantly.  An 
ioctl() interface seemed like the most obvious approach that wouldn't 
break existing apps.  Of course, any suggestion for a better interface 
would be appreciated.

The general problem of xenstore access in domU is a big issue for us 
(and I assume it will be for most people building Xen management 
infrastructure).  It would really help to have a good solution for 3.0.2.

Regards,

Anthony Liguori

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

Re: [PATCH] Add an ioctl interface for simple xenstore access

[Keir Fraser]

On 18 Mar 2006, at 18:02, Anthony Liguori wrote:

> We had discussed this in a previous thread and I thought you were okay 
> with this approach.  The /proc/xen/xenbus interface requires full 
> parsing of the XenBus protocol.  This requires libxenstore in domU.  
> Today, libxenstore is shipped with the rest of the tools package on 
> most distros.  If one wants to write a Xen-aware application for a 
> domU, that brings in a pretty large number of unnecessary 
> dependencies.  Also, for 32 bit management apps in a 64 bit 
> environment, it makes things very nasty (should we build a 32 bit and 
> 64 bit version of libxenstore?  Is the protocol 32/64 bit safe?).
>
> Ideally, an app could just use a simple interface to /proc/xen/xenbus 
> to access XenStore.  That solves all of the above problems elegantly.  
> An ioctl() interface seemed like the most obvious approach that 
> wouldn't break existing apps.  Of course, any suggestion for a better 
> interface would be appreciated.

Either way you're going to have an abstraction layer in your apps that 
hides the grotty details of how you access xenstore via 
/proc/xen/xenbus, whether it's via the read/write interface or via 
ioctls. That given, why not just statically link with libxenstore, or 
even include xs.h, xs.c and xs_lib.c in your applications? They're 
small, so no significant bloat, and LGPL, so no fear of GPL bleed into 
your management apps.

The protocol is 32/64-bit safe.

  -- Keir

Re: [PATCH] Add an ioctl interface for simple xenstore access

[Muli Ben-Yehuda]

On Sat, Mar 18, 2006 at 11:56:27AM -0600, Anthony Liguori wrote:

> I'm not sure how one would express the transaction semantics of xenstore 
> via a virtual filesystem.

file open - transaction begin, file close - transaction commit, if you
need multiple "files" in a single transaction, you make this a special
file. Not very elegant, but as plan9 shows us, nearly anything can be
represented as a file{system}.

> There is also an awkward mapping between some 
> of the attributes of xenstore (for instance, there's no way to determine 
> if an attribute returned by a list is a directory or not)

If it has "sub attributes", represent it as a directory.

> If you remember, I had written a fuse filesystem front-end for xenstore 
> at one point.  It's definitely neat, but I think we would have to make 
> some changes to Xenstore for it to really work out well.

Ok. I freely acknowledge knowing very little about xenstore and how to
best represent its data. I do however know what the review comments
for upstream inclusion of any new ioctls would be like ;-)

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

Re: [PATCH] Add an ioctl interface for simple xenstore access

[Gerd Hoffmann]

> We had discussed this in a previous thread and I thought you were okay
> with this approach.  The /proc/xen/xenbus interface requires full
> parsing of the XenBus protocol.  This requires libxenstore in domU. 
> Today, libxenstore is shipped with the rest of the tools package on most
> distros.  

Changing this and move libxenstore to another package (to avoid pulling
in xend with python if everything you want is xenstore access) isn't a
big problem ...

btw: are xenstore watches supposed to work inside domU?

cheers,

  Gerd

-- 
Gerd 'just married' Hoffmann <kraxel@suse.de>
I'm the hacker formerly known as Gerd Knorr.
http://www.suse.de/~kraxel/just-married.jpeg

Re: [PATCH] Add an ioctl interface for simple xenstore access

[Ewan Mellor]

On Mon, Mar 20, 2006 at 09:38:22AM +0100, Gerd Hoffmann wrote:

> > We had discussed this in a previous thread and I thought you were okay
> > with this approach.  The /proc/xen/xenbus interface requires full
> > parsing of the XenBus protocol.  This requires libxenstore in domU. 
> > Today, libxenstore is shipped with the rest of the tools package on most
> > distros.  
> 
> Changing this and move libxenstore to another package (to avoid pulling
> in xend with python if everything you want is xenstore access) isn't a
> big problem ...
> 
> btw: are xenstore watches supposed to work inside domU?

Watches work in kernel space in domU (the frontends to the split device
drivers use them), but they don't work in userspace there.  The problem is
that there is no support in xenbus for blocking a userspace process waiting
for a watch to fire.

In domain 0, Xend uses a unix domain socket straight to Xenstored: it can
block on that, as it's not going through the Xenbus kernel-level driver.

If someone wanted to add such support, that would be appreciated.

Ewan.

Re: [PATCH] Add an ioctl interface for simple xenstore access

[Keir Fraser]

On 20 Mar 2006, at 08:52, Ewan Mellor wrote:

> Watches work in kernel space in domU (the frontends to the split device
> drivers use them), but they don't work in userspace there.  The 
> problem is
> that there is no support in xenbus for blocking a userspace process 
> waiting
> for a watch to fire.

There's no need for special-case blocking in the kernel. The 
libxenstore reader thread will block on read() and then wake up other 
threads as necessary by writing to a pipe. All that's needed in the 
kernel is some bookkeeping to allow demux and teardown if the device 
file is closed. Some of that can be pushed to xenstored if we extend it 
to support multiple connections over a single shared page, but it's not 
really necessary.

  -- Keir

Re: [PATCH] Add an ioctl interface for simple xenstore access

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 891 bytes --]

> Watches work in kernel space in domU (the frontends to the split device
> drivers use them), but they don't work in userspace there.  The problem is
> that there is no support in xenbus for blocking a userspace process waiting
> for a watch to fire.
> 
> In domain 0, Xend uses a unix domain socket straight to Xenstored: it can
> block on that, as it's not going through the Xenbus kernel-level driver.

Huh?  Sleeping on the unix socket and sleeping on the /proc/xen/xenbus
filehandle should work equally well, shouldn't it?  Well, right now
there is no poll support, so you can't stuff the filehandle into
select()-loop.  But that is trivially fixable, patch below
(compile-tested only though).  Or did I miss the real problem?

cheers,

  Gerd

-- 
Gerd 'just married' Hoffmann <kraxel@suse.de>
I'm the hacker formerly known as Gerd Knorr.
http://www.suse.de/~kraxel/just-married.jpeg

[-- Attachment #2: xenbus-poll --]
[-- Type: text/plain, Size: 846 bytes --]

diff -r f6bd46559b93 drivers/xen/xenbus/xenbus_dev.c
--- a/drivers/xen/xenbus/xenbus_dev.c	Mon Mar  6 17:57:34 2006
+++ b/drivers/xen/xenbus/xenbus_dev.c	Mon Mar 20 11:04:49 2006
@@ -36,6 +36,7 @@
 #include <linux/notifier.h>
 #include <linux/wait.h>
 #include <linux/fs.h>
+#include <linux/poll.h>
 
 #include "xenbus_comms.h"
 
@@ -208,11 +209,22 @@
 	return 0;
 }
 
+static unsigned int xenbus_dev_poll(struct file *file, poll_table *wait)
+{
+	struct xenbus_dev_data *u = file->private_data;
+
+	poll_wait(file, &u->read_waitq, wait);
+	if (u->read_cons != u->read_prod)
+		return POLLIN | POLLRDNORM;
+	return 0;
+}
+
 static struct file_operations xenbus_dev_file_ops = {
 	.read = xenbus_dev_read,
 	.write = xenbus_dev_write,
 	.open = xenbus_dev_open,
 	.release = xenbus_dev_release,
+	.poll = xenbus_dev_poll,
 };
 
 static int __init

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

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