Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Xen: Devel

[PATCH v5 6/8] libxl: introduce libxl__alloc_vdev

 

 

Xen devel RSS feed   Index | Next | Previous | View Threaded


stefano.stabellini at eu

May 4, 2012, 4:13 AM

Post #1 of 12 (103 views)
Permalink
[PATCH v5 6/8] libxl: introduce libxl__alloc_vdev

Introduce libxl__alloc_vdev: find a spare virtual block device in the
domain passed as argument.

Changes in v5:
- remove domid paramter to libxl__alloc_vdev (assume
LIBXL_TOOLSTACK_DOMID);
- remove scaling limit from libxl__alloc_vdev.

Changes in v4:
- rename libxl__devid_to_vdev to libxl__devid_to_localdev;
- introduce upper bound for encode_disk_name;
- better error handling;

Signed-off-by: Stefano Stabellini <stefano.stabellini [at] eu>
---
tools/libxl/libxl_internal.c | 31 ++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 4 +++
tools/libxl/libxl_linux.c | 45 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_netbsd.c | 6 +++++
4 files changed, 86 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index bd5ebb3..7a1e017 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -480,6 +480,37 @@ out:
return rc;
}

+/* libxl__alloc_vdev only works on the local domain, that is the domain
+ * where the toolstack is running */
+static char * libxl__alloc_vdev(libxl__gc *gc, const char *blkdev_start,
+ xs_transaction_t t)
+{
+ int devid = 0, disk = 0, part = 0;
+ char *vdev = NULL;
+ char *dompath = libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID);
+
+ libxl__device_disk_dev_number(blkdev_start, &disk, &part);
+ if (part != 0) {
+ LOG(ERROR, "blkdev_start is invalid");
+ return NULL;
+ }
+
+ do {
+ vdev = GCSPRINTF("d%dp0", disk);
+ devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
+ if (libxl__xs_read(gc, t,
+ libxl__sprintf(gc, "%s/device/vbd/%d/backend",
+ dompath, devid)) == NULL) {
+ if (errno == ENOENT)
+ return libxl__devid_to_localdev(gc, devid);
+ else
+ return NULL;
+ }
+ disk++;
+ } while (disk <= (1 << 20));
+ return NULL;
+}
+
char * libxl__device_disk_local_attach(libxl__gc *gc,
const libxl_device_disk *in_disk,
libxl_device_disk **new_disk,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 0074ec4..a451c79 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -77,6 +77,8 @@
#define LIBXL_PV_EXTRA_MEMORY 1024
#define LIBXL_HVM_EXTRA_MEMORY 2048
#define LIBXL_MIN_DOM0_MEM (128*1024)
+/* use 0 as the domid of the toolstack domain for now */
+#define LIBXL_TOOLSTACK_DOMID 0
#define QEMU_SIGNATURE "DeviceModelRecord0002"
#define STUBDOM_CONSOLE_LOGGING 0
#define STUBDOM_CONSOLE_SAVE 1
@@ -763,6 +765,8 @@ _hidden int libxl__ev_devstate_wait(libxl__gc *gc, libxl__ev_devstate *ds,
*/
_hidden int libxl__try_phy_backend(mode_t st_mode);

+_hidden char *libxl__devid_to_localdev(libxl__gc *gc, int devid);
+
/* from libxl_pci */

_hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting);
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 925248b..cb596a9 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -25,3 +25,48 @@ int libxl__try_phy_backend(mode_t st_mode)

return 1;
}
+
+#define EXT_SHIFT 28
+#define EXTENDED (1<<EXT_SHIFT)
+#define VDEV_IS_EXTENDED(dev) ((dev)&(EXTENDED))
+#define BLKIF_MINOR_EXT(dev) ((dev)&(~EXTENDED))
+
+/* same as in Linux */
+static char *encode_disk_name(char *ptr, unsigned int n)
+{
+ if (n >= 26)
+ ptr = encode_disk_name(ptr, n / 26 - 1);
+ *ptr = 'a' + n % 26;
+ return ptr + 1;
+}
+
+char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
+{
+ int minor;
+ int offset;
+ int nr_parts;
+ char *ptr = NULL;
+ char *ret = libxl__zalloc(gc, 32);
+
+ if (!VDEV_IS_EXTENDED(devid)) {
+ minor = devid & 0xff;
+ nr_parts = 16;
+ } else {
+ minor = BLKIF_MINOR_EXT(devid);
+ nr_parts = 256;
+ }
+ offset = minor / nr_parts;
+
+ /* arbitrary upper bound */
+ if (offset > 676)
+ return NULL;
+
+ strcpy(ret, "xvd");
+ ptr = encode_disk_name(ret + 3, offset);
+ if (minor % nr_parts == 0)
+ *ptr = 0;
+ else
+ snprintf(ptr, ret + 32 - ptr,
+ "%d", minor & (nr_parts - 1));
+ return ret;
+}
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 9e0ed6d..dbf5f71 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -24,3 +24,9 @@ int libxl__try_phy_backend(mode_t st_mode)

return 0;
}
+
+char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
+{
+ /* TODO */
+ return NULL;
+}
--
1.7.2.5


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


Ian.Campbell at citrix

May 4, 2012, 7:59 AM

Post #2 of 12 (107 views)
Permalink
Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev [In reply to]

On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> Introduce libxl__alloc_vdev: find a spare virtual block device in the
> domain passed as argument.
>
> Changes in v5:
> - remove domid paramter to libxl__alloc_vdev (assume
> LIBXL_TOOLSTACK_DOMID);
> - remove scaling limit from libxl__alloc_vdev.
>
> Changes in v4:
> - rename libxl__devid_to_vdev to libxl__devid_to_localdev;
> - introduce upper bound for encode_disk_name;
> - better error handling;
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini [at] eu>
> ---
> tools/libxl/libxl_internal.c | 31 ++++++++++++++++++++++++++++
> tools/libxl/libxl_internal.h | 4 +++
> tools/libxl/libxl_linux.c | 45 ++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/libxl_netbsd.c | 6 +++++
> 4 files changed, 86 insertions(+), 0 deletions(-)
>
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index bd5ebb3..7a1e017 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -480,6 +480,37 @@ out:
> return rc;
> }
>
> +/* libxl__alloc_vdev only works on the local domain, that is the domain
> + * where the toolstack is running */
> +static char * libxl__alloc_vdev(libxl__gc *gc, const char *blkdev_start,
> + xs_transaction_t t)
> +{
> + int devid = 0, disk = 0, part = 0;
> + char *vdev = NULL;
> + char *dompath = libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID);
> +
> + libxl__device_disk_dev_number(blkdev_start, &disk, &part);

If you specify the default blkdev_start in xl as d0 instead of xvda
doesn't at least this bit magically become portable to BSD etc too?

Or couldn't it actually be an int and save you parsing at all?

> + if (part != 0) {
> + LOG(ERROR, "blkdev_start is invalid");
> + return NULL;
> + }
> +
> + do {
> + vdev = GCSPRINTF("d%dp0", disk);
> + devid = libxl__device_disk_dev_number(vdev, NULL, NULL);

libxl__device_disk_dev_number does the right thing with "d<N>" (i.e.
without the hardcoded "p0"), I think.

I'd be tempted to inline the GCSPRINTF in the arg and do away with vdev
since you don't use it again.

> + if (libxl__xs_read(gc, t,
> + libxl__sprintf(gc, "%s/device/vbd/%d/backend",
> + dompath, devid)) == NULL) {
> + if (errno == ENOENT)
> + return libxl__devid_to_localdev(gc, devid);
> + else
> + return NULL;
> + }
> + disk++;
> + } while (disk <= (1 << 20));

That's a whole lotta disks ;-)

> + return NULL;
> +}
> +
> char * libxl__device_disk_local_attach(libxl__gc *gc,
> const libxl_device_disk *in_disk,
> libxl_device_disk **new_disk,
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 0074ec4..a451c79 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -77,6 +77,8 @@
> #define LIBXL_PV_EXTRA_MEMORY 1024
> #define LIBXL_HVM_EXTRA_MEMORY 2048
> #define LIBXL_MIN_DOM0_MEM (128*1024)
> +/* use 0 as the domid of the toolstack domain for now */
> +#define LIBXL_TOOLSTACK_DOMID 0
> #define QEMU_SIGNATURE "DeviceModelRecord0002"
> #define STUBDOM_CONSOLE_LOGGING 0
> #define STUBDOM_CONSOLE_SAVE 1
> @@ -763,6 +765,8 @@ _hidden int libxl__ev_devstate_wait(libxl__gc *gc, libxl__ev_devstate *ds,
> */
> _hidden int libxl__try_phy_backend(mode_t st_mode);
>
> +_hidden char *libxl__devid_to_localdev(libxl__gc *gc, int devid);
> +
> /* from libxl_pci */
>
> _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting);
> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index 925248b..cb596a9 100644
> --- a/tools/libxl/libxl_linux.c
> +++ b/tools/libxl/libxl_linux.c
> @@ -25,3 +25,48 @@ int libxl__try_phy_backend(mode_t st_mode)
>
> return 1;
> }
> +
> +#define EXT_SHIFT 28
> +#define EXTENDED (1<<EXT_SHIFT)
> +#define VDEV_IS_EXTENDED(dev) ((dev)&(EXTENDED))
> +#define BLKIF_MINOR_EXT(dev) ((dev)&(~EXTENDED))
> +
> +/* same as in Linux */
> +static char *encode_disk_name(char *ptr, unsigned int n)
> +{
> + if (n >= 26)
> + ptr = encode_disk_name(ptr, n / 26 - 1);
> + *ptr = 'a' + n % 26;
> + return ptr + 1;
> +}
> +
> +char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
> +{
> + int minor;
> + int offset;
> + int nr_parts;
> + char *ptr = NULL;
> + char *ret = libxl__zalloc(gc, 32);
> +
> + if (!VDEV_IS_EXTENDED(devid)) {
> + minor = devid & 0xff;
> + nr_parts = 16;
> + } else {
> + minor = BLKIF_MINOR_EXT(devid);
> + nr_parts = 256;
> + }
> + offset = minor / nr_parts;
> +
> + /* arbitrary upper bound */
> + if (offset > 676)
> + return NULL;
> +
> + strcpy(ret, "xvd");
> + ptr = encode_disk_name(ret + 3, offset);
> + if (minor % nr_parts == 0)
> + *ptr = 0;
> + else
> + snprintf(ptr, ret + 32 - ptr,
> + "%d", minor & (nr_parts - 1));
> + return ret;
> +}
> diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> index 9e0ed6d..dbf5f71 100644
> --- a/tools/libxl/libxl_netbsd.c
> +++ b/tools/libxl/libxl_netbsd.c

Aha, here's where we need a BSD contribution. CC someone?

> @@ -24,3 +24,9 @@ int libxl__try_phy_backend(mode_t st_mode)
>
> return 0;
> }
> +
> +char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
> +{
> + /* TODO */
> + return NULL;
> +}



_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


Ian.Jackson at eu

May 4, 2012, 9:39 AM

Post #3 of 12 (96 views)
Permalink
Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev [In reply to]

Stefano Stabellini writes ("[PATCH v5 6/8] libxl: introduce libxl__alloc_vdev"):
> Introduce libxl__alloc_vdev: find a spare virtual block device in the
> domain passed as argument.
...
> + do {
> + vdev = GCSPRINTF("d%dp0", disk);
> + devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
> + if (libxl__xs_read(gc, t,
> + libxl__sprintf(gc, "%s/device/vbd/%d/backend",
> + dompath, devid)) == NULL) {
> + if (errno == ENOENT)
> + return libxl__devid_to_localdev(gc, devid);
> + else
> + return NULL;
> + }
> + disk++;
> + } while (disk <= (1 << 20));

This should be "<", as disk==(1<<20) is too big.
And the magic number 20 should not be repeated.

But it turns out that you don't need to do this check here at all
because libxl__device_disk_dev_number will check this, correctly.

All you need to do is actually pay attention to its error return.

> + return NULL;

All the NULL error exits should log an error surely.

> +/* same as in Linux */
> +static char *encode_disk_name(char *ptr, unsigned int n)
> +{
> + if (n >= 26)
> + ptr = encode_disk_name(ptr, n / 26 - 1);
> + *ptr = 'a' + n % 26;
> + return ptr + 1;
> +}

This still makes it difficult to see that the buffer cannot be
overrun. I mentioned this in response to a previous posting of this
code and IIRC you were going to do something about it, if only a
comment explaining what the maximum buffer length is and why it's
safe.

> + strcpy(ret, "xvd");
> + ptr = encode_disk_name(ret + 3, offset);
> + if (minor % nr_parts == 0)
> + *ptr = 0;
> + else
> + snprintf(ptr, ret + 32 - ptr,
> + "%d", minor & (nr_parts - 1));

You do not handle snprintf buffer truncation sensibly here. As it
happens I think this overflow cannot happen but this deserves a
comment at the very least, to explain the lack of error handling.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


Ian.Jackson at eu

May 4, 2012, 9:46 AM

Post #4 of 12 (97 views)
Permalink
Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev [In reply to]

Ian Campbell writes ("Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev"):
> On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > domain passed as argument.
...
> > + libxl__device_disk_dev_number(blkdev_start, &disk, &part);
>
> If you specify the default blkdev_start in xl as d0 instead of xvda
> doesn't at least this bit magically become portable to BSD etc too?

This bit is portable already. It's specified in terms of the guest
device name rather than the host device name. This may be confusing
but it saves on having a converter for host device names to disk
numbers.

> Or couldn't it actually be an int and save you parsing at all?

In general it's surely more friendly to allow the user to specify this
with a name, like we do with vdevs elsewhere.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


Ian.Campbell at citrix

May 4, 2012, 10:08 AM

Post #5 of 12 (97 views)
Permalink
Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev [In reply to]

On Fri, 2012-05-04 at 17:46 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev"):
> > On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> > > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > > domain passed as argument.
> ...
> > > + libxl__device_disk_dev_number(blkdev_start, &disk, &part);
> >
> > If you specify the default blkdev_start in xl as d0 instead of xvda
> > doesn't at least this bit magically become portable to BSD etc too?
>
> This bit is portable already. It's specified in terms of the guest
> device name rather than the host device name. This may be confusing
> but it saves on having a converter for host device names to disk
> numbers.

Right. I think I really should have made this comment on the previous
patch, What I was trying to suggest is that using the Linux specific
"xvda" in the example in the config file and as the default (as patch
5/8 does) is not portable, even though the infrastructure itself is
quite capable of dealing with the portable alternatives -- IOW using
"d0" both in the example and in the actual default would make xl be
automatically portable without other patches (libxl still needs a patch
for the BSD libxl__devid_to_localdev but that's it).

> > Or couldn't it actually be an int and save you parsing at all?
>
> In general it's surely more friendly to allow the user to specify this
> with a name, like we do with vdevs elsewhere.

That seems reasonable.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


Ian.Jackson at eu

May 4, 2012, 10:16 AM

Post #6 of 12 (96 views)
Permalink
Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev [In reply to]

Ian Campbell writes ("Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev"):
> Right. I think I really should have made this comment on the previous
> patch, What I was trying to suggest is that using the Linux specific
> "xvda" in the example in the config file

"xvda" is not currently thought to be necessarily Linux-specific. See
docs/misc/vbd-interface.text. Perhaps that needs to be revisited, but
in general it's perfectly possible to pass "xvda" in a domain config
file for a BSD guest, or whatever. And Linux writing "xvdfoo" in the
config file doesn't always make Linux call the device /dev/xvdfoo.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


Ian.Campbell at citrix

May 4, 2012, 10:20 AM

Post #7 of 12 (96 views)
Permalink
Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev [In reply to]

On Fri, 2012-05-04 at 18:16 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev"):
> > Right. I think I really should have made this comment on the previous
> > patch, What I was trying to suggest is that using the Linux specific
> > "xvda" in the example in the config file
>
> "xvda" is not currently thought to be necessarily Linux-specific. See
> docs/misc/vbd-interface.text. Perhaps that needs to be revisited, but
> in general it's perfectly possible to pass "xvda" in a domain config
> file for a BSD guest, or whatever. And Linux writing "xvdfoo" in the
> config file doesn't always make Linux call the device /dev/xvdfoo.

Hrm, I guess. It certainly gives the impression of being Linux specific,
even if technically it isn't, since people associate xvd* with Linux
even if it happens to be valid and work elsewhere. I guess in reality
very few people, including BSD folks, are going to change this (because
it'll just work) so there's no chance of them getting confused.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


stefano.stabellini at eu

May 14, 2012, 6:48 AM

Post #8 of 12 (87 views)
Permalink
Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev [In reply to]

On Fri, 4 May 2012, Ian Campbell wrote:
> On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > domain passed as argument.
> >
> > Changes in v5:
> > - remove domid paramter to libxl__alloc_vdev (assume
> > LIBXL_TOOLSTACK_DOMID);
> > - remove scaling limit from libxl__alloc_vdev.
> >
> > Changes in v4:
> > - rename libxl__devid_to_vdev to libxl__devid_to_localdev;
> > - introduce upper bound for encode_disk_name;
> > - better error handling;
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini [at] eu>
> > ---
> > tools/libxl/libxl_internal.c | 31 ++++++++++++++++++++++++++++
> > tools/libxl/libxl_internal.h | 4 +++
> > tools/libxl/libxl_linux.c | 45 ++++++++++++++++++++++++++++++++++++++++++
> > tools/libxl/libxl_netbsd.c | 6 +++++
> > 4 files changed, 86 insertions(+), 0 deletions(-)
> >
> > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> > index bd5ebb3..7a1e017 100644
> > --- a/tools/libxl/libxl_internal.c
> > +++ b/tools/libxl/libxl_internal.c
> > @@ -480,6 +480,37 @@ out:
> > return rc;
> > }
> >
> > +/* libxl__alloc_vdev only works on the local domain, that is the domain
> > + * where the toolstack is running */
> > +static char * libxl__alloc_vdev(libxl__gc *gc, const char *blkdev_start,
> > + xs_transaction_t t)
> > +{
> > + int devid = 0, disk = 0, part = 0;
> > + char *vdev = NULL;
> > + char *dompath = libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID);
> > +
> > + libxl__device_disk_dev_number(blkdev_start, &disk, &part);
>
> If you specify the default blkdev_start in xl as d0 instead of xvda
> doesn't at least this bit magically become portable to BSD etc too?

It cannot work on BSD for other reason, see below.
In any case blkdev_start can be anything that
libxl__device_disk_dev_number can parse, so d0p0 works too.


> Or couldn't it actually be an int and save you parsing at all?

Yes, it could. But isn't "xvda" much more intuitive?


> > + if (part != 0) {
> > + LOG(ERROR, "blkdev_start is invalid");
> > + return NULL;
> > + }
> > +
> > + do {
> > + vdev = GCSPRINTF("d%dp0", disk);
> > + devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
>
> libxl__device_disk_dev_number does the right thing with "d<N>" (i.e.
> without the hardcoded "p0"), I think.

In my tests it doesn't work properly using just d<N>.


> I'd be tempted to inline the GCSPRINTF in the arg and do away with vdev
> since you don't use it again.

OK

> > diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> > index 9e0ed6d..dbf5f71 100644
> > --- a/tools/libxl/libxl_netbsd.c
> > +++ b/tools/libxl/libxl_netbsd.c
>
> Aha, here's where we need a BSD contribution. CC someone?

There is no working gntdev or QDISK on BSD at the moment.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


Ian.Campbell at citrix

May 14, 2012, 6:56 AM

Post #9 of 12 (89 views)
Permalink
Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev [In reply to]

On Mon, 2012-05-14 at 14:48 +0100, Stefano Stabellini wrote:
> On Fri, 4 May 2012, Ian Campbell wrote:
> > On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> > > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > > domain passed as argument.
> > >
> > > Changes in v5:
> > > - remove domid paramter to libxl__alloc_vdev (assume
> > > LIBXL_TOOLSTACK_DOMID);
> > > - remove scaling limit from libxl__alloc_vdev.
> > >
> > > Changes in v4:
> > > - rename libxl__devid_to_vdev to libxl__devid_to_localdev;
> > > - introduce upper bound for encode_disk_name;
> > > - better error handling;
> > >
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini [at] eu>
> > > ---
> > > tools/libxl/libxl_internal.c | 31 ++++++++++++++++++++++++++++
> > > tools/libxl/libxl_internal.h | 4 +++
> > > tools/libxl/libxl_linux.c | 45 ++++++++++++++++++++++++++++++++++++++++++
> > > tools/libxl/libxl_netbsd.c | 6 +++++
> > > 4 files changed, 86 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> > > index bd5ebb3..7a1e017 100644
> > > --- a/tools/libxl/libxl_internal.c
> > > +++ b/tools/libxl/libxl_internal.c
> > > @@ -480,6 +480,37 @@ out:
> > > return rc;
> > > }
> > >
> > > +/* libxl__alloc_vdev only works on the local domain, that is the domain
> > > + * where the toolstack is running */
> > > +static char * libxl__alloc_vdev(libxl__gc *gc, const char *blkdev_start,
> > > + xs_transaction_t t)
> > > +{
> > > + int devid = 0, disk = 0, part = 0;
> > > + char *vdev = NULL;
> > > + char *dompath = libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID);
> > > +
> > > + libxl__device_disk_dev_number(blkdev_start, &disk, &part);
> >
> > If you specify the default blkdev_start in xl as d0 instead of xvda
> > doesn't at least this bit magically become portable to BSD etc too?
>
> It cannot work on BSD for other reason, see below.
> In any case blkdev_start can be anything that
> libxl__device_disk_dev_number can parse, so d0p0 works too.
>
>
> > Or couldn't it actually be an int and save you parsing at all?
>
> Yes, it could. But isn't "xvda" much more intuitive?

To Linux users, yes. But on BSD the virt devices have a different naming
scheme, so xvda wouldn't necessarily make much sense to them.

However I agree with IanJ's comment that letting the user use a name is
friendlier than just a bare number.

Anyway, lets just leave it as "xvda", it's not a huge deal.

> > > + if (part != 0) {
> > > + LOG(ERROR, "blkdev_start is invalid");
> > > + return NULL;
> > > + }
> > > +
> > > + do {
> > > + vdev = GCSPRINTF("d%dp0", disk);
> > > + devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
> >
> > libxl__device_disk_dev_number does the right thing with "d<N>" (i.e.
> > without the hardcoded "p0"), I think.
>
> In my tests it doesn't work properly using just d<N>.

I misread it, the sscanf in libxl__device_disk_dev_number needs to match
at least twice (so "dN" and "pM"). In theory that could be changed but
lets not do that now.

> > > diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> > > index 9e0ed6d..dbf5f71 100644
> > > --- a/tools/libxl/libxl_netbsd.c
> > > +++ b/tools/libxl/libxl_netbsd.c
> >
> > Aha, here's where we need a BSD contribution. CC someone?
>
> There is no working gntdev or QDISK on BSD at the moment.

Oh, right, that does make it somewhat pointless...

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


stefano.stabellini at eu

May 14, 2012, 7:10 AM

Post #10 of 12 (88 views)
Permalink
Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev [In reply to]

On Fri, 4 May 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[PATCH v5 6/8] libxl: introduce libxl__alloc_vdev"):
> > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > domain passed as argument.
> ...
> > + do {
> > + vdev = GCSPRINTF("d%dp0", disk);
> > + devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
> > + if (libxl__xs_read(gc, t,
> > + libxl__sprintf(gc, "%s/device/vbd/%d/backend",
> > + dompath, devid)) == NULL) {
> > + if (errno == ENOENT)
> > + return libxl__devid_to_localdev(gc, devid);
> > + else
> > + return NULL;
> > + }
> > + disk++;
> > + } while (disk <= (1 << 20));
>
> This should be "<", as disk==(1<<20) is too big.
> And the magic number 20 should not be repeated.
>
> But it turns out that you don't need to do this check here at all
> because libxl__device_disk_dev_number will check this, correctly.
>
> All you need to do is actually pay attention to its error return.

OK

> > + return NULL;
>
> All the NULL error exits should log an error surely.

The error log is in the next patch. Should I add a second log here?
Or maybe I should move the error log from the caller to the callee?


> > +/* same as in Linux */
> > +static char *encode_disk_name(char *ptr, unsigned int n)
> > +{
> > + if (n >= 26)
> > + ptr = encode_disk_name(ptr, n / 26 - 1);
> > + *ptr = 'a' + n % 26;
> > + return ptr + 1;
> > +}
>
> This still makes it difficult to see that the buffer cannot be
> overrun. I mentioned this in response to a previous posting of this
> code and IIRC you were going to do something about it, if only a
> comment explaining what the maximum buffer length is and why it's
> safe.

I am keen on keeping the code as is, because it is the same that is in
Linux (see comment above).
I'll add a comment.

> > + strcpy(ret, "xvd");
> > + ptr = encode_disk_name(ret + 3, offset);
> > + if (minor % nr_parts == 0)
> > + *ptr = 0;
> > + else
> > + snprintf(ptr, ret + 32 - ptr,
> > + "%d", minor & (nr_parts - 1));
>
> You do not handle snprintf buffer truncation sensibly here. As it
> happens I think this overflow cannot happen but this deserves a
> comment at the very least, to explain the lack of error handling.

Same here, I am keen on keeping the code as is, because it is the same
that is in Linux.
I'll add a comment.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


Ian.Jackson at eu

May 14, 2012, 7:22 AM

Post #11 of 12 (89 views)
Permalink
Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev [In reply to]

Stefano Stabellini writes ("Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev"):
> On Fri, 4 May 2012, Ian Jackson wrote:
> > All you need to do is actually pay attention to its error return.
>
> OK
>
> > > + return NULL;
> >
> > All the NULL error exits should log an error surely.
>
> The error log is in the next patch. Should I add a second log here?
> Or maybe I should move the error log from the caller to the callee?

Well, my view is that a function should either always log if it
returns ERROR_FAIL (or NULL if that's its calling pattern), or never
do so. And if it logs this should be in a doc comment.

I inferred that the function was supposed to log by the fact that the
other error paths logged (that is, I didn't read the doc comment or
the rest of the code).

I don't mind where the logging is done (although normally it's best to
do it closer to the source of the error) but I do want to make sure
that we don't have cases where there is an exit path which simply
fails without explaining why.

> > > +/* same as in Linux */
> > > +static char *encode_disk_name(char *ptr, unsigned int n)
> > > +{
> > > + if (n >= 26)
> > > + ptr = encode_disk_name(ptr, n / 26 - 1);
> > > + *ptr = 'a' + n % 26;
> > > + return ptr + 1;
> > > +}
> >
> > This still makes it difficult to see that the buffer cannot be
> > overrun. I mentioned this in response to a previous posting of this
> > code and IIRC you were going to do something about it, if only a
> > comment explaining what the maximum buffer length is and why it's
> > safe.
>
> I am keen on keeping the code as is, because it is the same that is in
> Linux (see comment above).

I read the comment as "this implements the same transformation as the
Linux kernel" not "this is the same code as actually used in the Linux
kernel". If you mean the latter, do say so.

Also, in that case why is the recursive function name, formatting,
etc. not the same as in Linux (as far as they can be) ? Surely if
Linux changes this we will want to change our code too and that will
be easiest if we can c&p without needing to reformat, rename, etc.

> I'll add a comment.

OK. The comments, taken together, should constitute proofs that the
code does not overrun any buffers.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


stefano.stabellini at eu

May 14, 2012, 7:43 AM

Post #12 of 12 (86 views)
Permalink
Re: [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev [In reply to]

On Mon, 14 May 2012, Ian Jackson wrote:
> > > > +/* same as in Linux */
> > > > +static char *encode_disk_name(char *ptr, unsigned int n)
> > > > +{
> > > > + if (n >= 26)
> > > > + ptr = encode_disk_name(ptr, n / 26 - 1);
> > > > + *ptr = 'a' + n % 26;
> > > > + return ptr + 1;
> > > > +}
> > >
> > > This still makes it difficult to see that the buffer cannot be
> > > overrun. I mentioned this in response to a previous posting of this
> > > code and IIRC you were going to do something about it, if only a
> > > comment explaining what the maximum buffer length is and why it's
> > > safe.
> >
> > I am keen on keeping the code as is, because it is the same that is in
> > Linux (see comment above).
>
> I read the comment as "this implements the same transformation as the
> Linux kernel" not "this is the same code as actually used in the Linux
> kernel". If you mean the latter, do say so.

Yes, it is actually the same used in the Linux kernel.


> Also, in that case why is the recursive function name, formatting,
> etc. not the same as in Linux (as far as they can be) ? Surely if
> Linux changes this we will want to change our code too and that will
> be easiest if we can c&p without needing to reformat, rename, etc.

It is exactly the same code, except for the tab indentation that I
replaced with space indentation to follow the coding style.


> > I'll add a comment.
>
> OK. The comments, taken together, should constitute proofs that the
> code does not overrun any buffers.

The two comments refer to the upper bound that we introduced, thanks to
the limit there can be no buffer overruns.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

Xen devel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.