[PATCH 00 of 16 RFC] add libxl support for blktap3 and introduce the blktap3 build system

[PATCH 00 of 16 RFC] add libxl support for blktap3 and introduce the blktap3 build system

[Thanos Makatos]

This series introduces support for blktap3 in libxl and adds the basis of the
blktap3 build system. There is no essential blktap3 functionality yet: all
blktap3 functions are stubs and they will be implemented in later patches. This
means that trying to use tap as the disk backend for a guest will make libxl
complain since all of blktap3 functions return a failure error code. Most
header files introduced in this series require some basic documentation.
Finally, licenses need to be verified.

[PATCH 01 of 16 RFC] blktap3: Introduce the blktap3 device type

[Thanos Makatos]

This device is used when tap is specifed as the disk backend, instead of vbd.

diff -r 3cd5bf62ded7 -r fef81de66bf7 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Wed Oct 24 17:22:39 2012 +0100
+++ b/tools/libxl/libxl.c	Wed Oct 24 17:24:01 2012 +0100
@@ -1742,7 +1742,7 @@ int libxl__device_from_disk(libxl__gc *g
             device->backend_kind = LIBXL__DEVICE_KIND_VBD;
             break;
         case LIBXL_DISK_BACKEND_TAP:
-            device->backend_kind = LIBXL__DEVICE_KIND_VBD;
+            device->backend_kind = LIBXL__DEVICE_KIND_XENIO;
             break;
         case LIBXL_DISK_BACKEND_QDISK:
             device->backend_kind = LIBXL__DEVICE_KIND_QDISK;
diff -r 3cd5bf62ded7 -r fef81de66bf7 tools/libxl/libxl_types_internal.idl
--- a/tools/libxl/libxl_types_internal.idl	Wed Oct 24 17:22:39 2012 +0100
+++ b/tools/libxl/libxl_types_internal.idl	Wed Oct 24 17:24:01 2012 +0100
@@ -19,6 +19,7 @@ libxl__device_kind = Enumeration("device
     (5, "VFB"),
     (6, "VKBD"),
     (7, "CONSOLE"),
+    (8, "XENIO"),
     ])
 
 libxl__console_backend = Enumeration("console_backend", [

Re: [PATCH 01 of 16 RFC] blktap3: Introduce the blktap3 device type

[Ian Campbell]

On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> This device is used when tap is specifed as the disk backend, instead of vbd.
> 
> diff -r 3cd5bf62ded7 -r fef81de66bf7 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Wed Oct 24 17:22:39 2012 +0100
> +++ b/tools/libxl/libxl.c	Wed Oct 24 17:24:01 2012 +0100
> @@ -1742,7 +1742,7 @@ int libxl__device_from_disk(libxl__gc *g
>              device->backend_kind = LIBXL__DEVICE_KIND_VBD;
>              break;
>          case LIBXL_DISK_BACKEND_TAP:
> -            device->backend_kind = LIBXL__DEVICE_KIND_VBD;
> +            device->backend_kind = LIBXL__DEVICE_KIND_XENIO;

Presumably as well as this change a bunch of related infrastructure
changes are needed? I'd suggest putting all those first and making this
switch at the end, so that xl remains functional at each point in the
series.

>              break;
>          case LIBXL_DISK_BACKEND_QDISK:
>              device->backend_kind = LIBXL__DEVICE_KIND_QDISK;
> diff -r 3cd5bf62ded7 -r fef81de66bf7 tools/libxl/libxl_types_internal.idl
> --- a/tools/libxl/libxl_types_internal.idl	Wed Oct 24 17:22:39 2012 +0100
> +++ b/tools/libxl/libxl_types_internal.idl	Wed Oct 24 17:24:01 2012 +0100
> @@ -19,6 +19,7 @@ libxl__device_kind = Enumeration("device
>      (5, "VFB"),
>      (6, "VKBD"),
>      (7, "CONSOLE"),
> +    (8, "XENIO"),

I know this is what it is called internally but for libxl purposes can
we use BLKTAP or something descriptive like that?

>      ])
>  
>  libxl__console_backend = Enumeration("console_backend", [
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 01 of 16 RFC] blktap3: Introduce the blktap3 device type

[Thanos Makatos]

> -----Original Message-----
> From: Ian Campbell
> Sent: 26 October 2012 12:17
> To: Thanos Makatos
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH 01 of 16 RFC] blktap3: Introduce the
> blktap3 device type
> 
> On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> > This device is used when tap is specifed as the disk backend, instead
> of vbd.
> >
> > diff -r 3cd5bf62ded7 -r fef81de66bf7 tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c	Wed Oct 24 17:22:39 2012 +0100
> > +++ b/tools/libxl/libxl.c	Wed Oct 24 17:24:01 2012 +0100
> > @@ -1742,7 +1742,7 @@ int libxl__device_from_disk(libxl__gc *g
> >              device->backend_kind = LIBXL__DEVICE_KIND_VBD;
> >              break;
> >          case LIBXL_DISK_BACKEND_TAP:
> > -            device->backend_kind = LIBXL__DEVICE_KIND_VBD;
> > +            device->backend_kind = LIBXL__DEVICE_KIND_XENIO;
> 
> Presumably as well as this change a bunch of related infrastructure
> changes are needed? I'd suggest putting all those first and making this
> switch at the end, so that xl remains functional at each point in the
> series.

Ok these changes will be part of the final "switch to blktap3" patch.

> 
> >              break;
> >          case LIBXL_DISK_BACKEND_QDISK:
> >              device->backend_kind = LIBXL__DEVICE_KIND_QDISK; diff -r
> > 3cd5bf62ded7 -r fef81de66bf7 tools/libxl/libxl_types_internal.idl
> > --- a/tools/libxl/libxl_types_internal.idl	Wed Oct 24 17:22:39
> 2012 +0100
> > +++ b/tools/libxl/libxl_types_internal.idl	Wed Oct 24 17:24:01
> 2012 +0100
> > @@ -19,6 +19,7 @@ libxl__device_kind = Enumeration("device
> >      (5, "VFB"),
> >      (6, "VKBD"),
> >      (7, "CONSOLE"),
> > +    (8, "XENIO"),
> 
> I know this is what it is called internally but for libxl purposes can
> we use BLKTAP or something descriptive like that?

Sure.

> 
> >      ])
> >
> >  libxl__console_backend = Enumeration("console_backend", [
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 

[PATCH 02 of 16 RFC] blktap3: Introduce blktap3 support for libxl

[Thanos Makatos]

These functions will be implemented at a later patch.

diff -r fef81de66bf7 -r 513739de43b5 tools/libxl/libxl_blktap3.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:24:37 2012 +0100
@@ -0,0 +1,15 @@
+#include "libxl_osdeps.h"
+#include "libxl_internal.h"
+
+int libxl__blktap_enabled(libxl__gc *gc) {
+    return 1;
+}
+
+char *libxl__blktap_devpath(libxl__gc *gc, const char *disk,
+    libxl_disk_format format) {
+    return NULL;
+}
+
+int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *be_path) {
+    return -ENOSYS;
+}

[PATCH 03 of 16 RFC] blktap3: support for blktap3 in libxl Makefile

[Thanos Makatos]

Point the libxl Makefile to the blktap3 include and library directories instead
of the blktap2 ones, don't link with the blktap2 control library, and compile
libxl with the unimplemented functions introduced in the previous patch.

diff -r 513739de43b5 -r 80e0bc67dcda tools/Rules.mk
--- a/tools/Rules.mk	Wed Oct 24 17:24:37 2012 +0100
+++ b/tools/Rules.mk	Wed Oct 24 17:24:53 2012 +0100
@@ -15,6 +15,7 @@ XEN_XENLIGHT       = $(XEN_ROOT)/tools/l
 XEN_XENSTORE       = $(XEN_ROOT)/tools/xenstore
 XEN_LIBXENSTAT     = $(XEN_ROOT)/tools/xenstat/libxenstat/src
 XEN_BLKTAP2        = $(XEN_ROOT)/tools/blktap2
+XEN_BLKTAP3        = $(XEN_ROOT)/tools/blktap3
 XEN_LIBVCHAN       = $(XEN_ROOT)/tools/libvchan
 
 CFLAGS_xeninclude = -I$(XEN_INCLUDE)
@@ -46,9 +47,9 @@ LIBXL_BLKTAP ?= n
 endif
 
 ifeq ($(LIBXL_BLKTAP),y)
-CFLAGS_libblktapctl = -I$(XEN_BLKTAP2)/control -I$(XEN_BLKTAP2)/include $(CFLAGS_xeninclude)
-LDLIBS_libblktapctl = -L$(XEN_BLKTAP2)/control -lblktapctl
-SHLIB_libblktapctl  = -Wl,-rpath-link=$(XEN_BLKTAP2)/control
+CFLAGS_libblktapctl = -I$(XEN_BLKTAP3)/control -I$(XEN_BLKTAP3)/include $(CFLAGS_xeninclude)
+LDLIBS_libblktapctl = -L$(XEN_BLKTAP3)/control -lblktapctl
+SHLIB_libblktapctl  = -Wl,-rpath-link=$(XEN_BLKTAP3)/control
 else
 CFLAGS_libblktapctl =
 LDLIBS_libblktapctl =
diff -r 513739de43b5 -r 80e0bc67dcda tools/libxl/Makefile
--- a/tools/libxl/Makefile	Wed Oct 24 17:24:37 2012 +0100
+++ b/tools/libxl/Makefile	Wed Oct 24 17:24:53 2012 +0100
@@ -20,7 +20,7 @@ LIBUUID_LIBS += -luuid
 endif
 
 LIBXL_LIBS =
-LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
+LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
 
 CFLAGS_LIBXL += $(CFLAGS_libxenctrl)
 CFLAGS_LIBXL += $(CFLAGS_libxenguest)
@@ -36,9 +36,9 @@ LIBXLU_LIBS =
 
 LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o flexarray.o
 ifeq ($(LIBXL_BLKTAP),y)
-LIBXL_OBJS-y += libxl_blktap2.o
+LIBXL_OBJS-y += libxl_blktap3.o
 else
-LIBXL_OBJS-y += libxl_noblktap2.o
+LIBXL_OBJS-y += libxl_noblktap3.o
 endif
 LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o
 LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpuid.o libxl_noarch.o
@@ -77,7 +77,8 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_c
 			libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
 LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
 
-$(LIBXL_OBJS): CFLAGS += $(CFLAGS_LIBXL) -include $(XEN_ROOT)/tools/config.h
+$(LIBXL_OBJS): CFLAGS += $(CFLAGS_LIBXL) -include $(XEN_ROOT)/tools/config.h \
+	$(CFLAGS_libblktapctl)
 
 AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h _paths.h \
 	_libxl_save_msgs_callout.h _libxl_save_msgs_helper.h

Re: [PATCH 03 of 16 RFC] blktap3: support for blktap3 in libxl Makefile

[Ian Campbell]

> diff -r 513739de43b5 -r 80e0bc67dcda tools/libxl/Makefile
> --- a/tools/libxl/Makefile	Wed Oct 24 17:24:37 2012 +0100
> +++ b/tools/libxl/Makefile	Wed Oct 24 17:24:53 2012 +0100
> @@ -20,7 +20,7 @@ LIBUUID_LIBS += -luuid
>  endif
>  
>  LIBXL_LIBS =
> -LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
> +LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)

What happened to LDLIBS_libblktapctl here?

>  CFLAGS_LIBXL += $(CFLAGS_libxenctrl)
>  CFLAGS_LIBXL += $(CFLAGS_libxenguest)
> @@ -77,7 +77,8 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_c
>  			libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
>  LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
>  
> -$(LIBXL_OBJS): CFLAGS += $(CFLAGS_LIBXL) -include $(XEN_ROOT)/tools/config.h
> +$(LIBXL_OBJS): CFLAGS += $(CFLAGS_LIBXL) -include $(XEN_ROOT)/tools/config.h \
> +	$(CFLAGS_libblktapctl)

This Makefile already contains
CFLAGS_LIBXL += $(CFLAGS_libblktapctl) 
so I think this change is redundant (or else incomplete).

Ian.

Re: [PATCH 03 of 16 RFC] blktap3: support for blktap3 in libxl Makefile

[Thanos Makatos]

> -----Original Message-----
> From: Ian Campbell
> Sent: 26 October 2012 12:19
> To: Thanos Makatos
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH 03 of 16 RFC] blktap3: support for
> blktap3 in libxl Makefile
> 
> > diff -r 513739de43b5 -r 80e0bc67dcda tools/libxl/Makefile
> > --- a/tools/libxl/Makefile	Wed Oct 24 17:24:37 2012 +0100
> > +++ b/tools/libxl/Makefile	Wed Oct 24 17:24:53 2012 +0100
> > @@ -20,7 +20,7 @@ LIBUUID_LIBS += -luuid  endif
> >
> >  LIBXL_LIBS =
> > -LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest)
> > $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(PTYFUNCS_LIBS)
> > $(LIBUUID_LIBS)
> > +LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest)
> > +$(LDLIBS_libxenstore) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
> 
> What happened to LDLIBS_libblktapctl here?

It's temporarily removed since libxl_blktap3.c provides a dummy implementation and re-enabled at a later patch in this series; anyway this doesn't make sense anymore.

> 
> >  CFLAGS_LIBXL += $(CFLAGS_libxenctrl)
> >  CFLAGS_LIBXL += $(CFLAGS_libxenguest) @@ -77,7 +77,8 @@ LIBXL_OBJS =
> > flexarray.o libxl.o libxl_c
> >  			libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-
> y)  LIBXL_OBJS
> > += _libxl_types.o libxl_flask.o _libxl_types_internal.o
> >
> > -$(LIBXL_OBJS): CFLAGS += $(CFLAGS_LIBXL) -include
> > $(XEN_ROOT)/tools/config.h
> > +$(LIBXL_OBJS): CFLAGS += $(CFLAGS_LIBXL) -include
> $(XEN_ROOT)/tools/config.h \
> > +	$(CFLAGS_libblktapctl)
> 
> This Makefile already contains
> CFLAGS_LIBXL += $(CFLAGS_libblktapctl)
> so I think this change is redundant (or else incomplete).

Correct, I missed it.
> 
> Ian.

[PATCH 04 of 16 RFC] blktap3: change libxl__blktap_devpath prototype to return an error code

[Thanos Makatos]

Make libxl__blktap_devpath return an error code instead of the device, since there is no device in dom0 any more. Amend the libxl code that uses this functions accordingly.

diff -r 80e0bc67dcda -r bcb5a6182868 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Wed Oct 24 17:24:53 2012 +0100
+++ b/tools/libxl/libxl.c	Wed Oct 24 17:25:02 2012 +0100
@@ -1844,13 +1844,14 @@ static void device_disk_add(libxl__egc *
                 break;
 
             case LIBXL_DISK_BACKEND_TAP:
-                dev = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
-                if (!dev) {
-                    LOG(ERROR, "failed to get blktap devpath for %p\n",
-                        disk->pdev_path);
+                rc = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
+                if (rc) {
+                    LOG(ERROR, "failed to get blktap devpath for %s: %s\n",
+                        disk->pdev_path, strerror(rc));
                     rc = ERROR_FAIL;
                     goto out;
                 }
+                dev = NULL;
                 flexarray_append(back, "tapdisk-params");
                 flexarray_append(back, libxl__sprintf(gc, "%s:%s",
                     libxl__device_disk_string_of_format(disk->format),
@@ -2277,8 +2278,13 @@ void libxl__device_disk_local_initiate_a
                 dev = disk->pdev_path;
                 break;
             case LIBXL_DISK_FORMAT_VHD:
-                dev = libxl__blktap_devpath(gc, disk->pdev_path,
-                                            disk->format);
+                rc = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
+                if (!rc) {
+                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                           "error getting tapdisk: %s", strerror(rc));
+                    rc = ERROR_FAIL;
+                    goto out;
+                }
                 break;
             case LIBXL_DISK_FORMAT_QCOW:
             case LIBXL_DISK_FORMAT_QCOW2:
diff -r 80e0bc67dcda -r bcb5a6182868 tools/libxl/libxl_blktap3.c
--- a/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:24:53 2012 +0100
+++ b/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:25:02 2012 +0100
@@ -5,9 +5,9 @@ int libxl__blktap_enabled(libxl__gc *gc)
     return 1;
 }
 
-char *libxl__blktap_devpath(libxl__gc *gc, const char *disk,
+int libxl__blktap_devpath(libxl__gc *gc, const char *disk,
     libxl_disk_format format) {
-    return NULL;
+    return -ENOSYS;
 }
 
 int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *be_path) {
diff -r 80e0bc67dcda -r bcb5a6182868 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Wed Oct 24 17:24:53 2012 +0100
+++ b/tools/libxl/libxl_internal.h	Wed Oct 24 17:25:02 2012 +0100
@@ -1344,10 +1344,9 @@ struct libxl__cpuid_policy {
 /* libxl__blktap_devpath:
  *    Argument: path and disk image as specified in config file.
  *      The type specifies whether this is aio, qcow, qcow2, etc.
- *    returns device path xenstore wants to have. returns NULL
- *      if no device corresponds to the disk.
+ *    returns 0 on success, an error code otherwise 
  */
-_hidden char *libxl__blktap_devpath(libxl__gc *gc,
+_hidden int libxl__blktap_devpath(libxl__gc *gc,
                                     const char *disk,
                                     libxl_disk_format format);

Re: [PATCH 04 of 16 RFC] blktap3: change libxl__blktap_devpath prototype to return an error code

[Ian Campbell]

On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> Make libxl__blktap_devpath return an error code instead of the device,
>  since there is no device in dom0 any more.

Does this function eventually go away then? Or will it eventually return
some sort of metadata which can be used to connect to the created tap
device?

>  Amend the libxl code that uses this functions accordingly.
> 
> diff -r 80e0bc67dcda -r bcb5a6182868 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Wed Oct 24 17:24:53 2012 +0100
> +++ b/tools/libxl/libxl.c	Wed Oct 24 17:25:02 2012 +0100
> @@ -1844,13 +1844,14 @@ static void device_disk_add(libxl__egc *
>                  break;
>  
>              case LIBXL_DISK_BACKEND_TAP:
> -                dev = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
> -                if (!dev) {
> -                    LOG(ERROR, "failed to get blktap devpath for %p\n",
> -                        disk->pdev_path);
> +                rc = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
> +                if (rc) {
> +                    LOG(ERROR, "failed to get blktap devpath for %s: %s\n",
> +                        disk->pdev_path, strerror(rc));
>                      rc = ERROR_FAIL;
>                      goto out;
>                  }
> +                dev = NULL;
>                  flexarray_append(back, "tapdisk-params");
>                  flexarray_append(back, libxl__sprintf(gc, "%s:%s",
>                      libxl__device_disk_string_of_format(disk->format),
> @@ -2277,8 +2278,13 @@ void libxl__device_disk_local_initiate_a
>                  dev = disk->pdev_path;
>                  break;
>              case LIBXL_DISK_FORMAT_VHD:
> -                dev = libxl__blktap_devpath(gc, disk->pdev_path,
> -                                            disk->format);
> +                rc = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
> +                if (!rc) {
> +                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +                           "error getting tapdisk: %s", strerror(rc));
> +                    rc = ERROR_FAIL;
> +                    goto out;
> +                }
>                  break;
>              case LIBXL_DISK_FORMAT_QCOW:
>              case LIBXL_DISK_FORMAT_QCOW2:
> diff -r 80e0bc67dcda -r bcb5a6182868 tools/libxl/libxl_blktap3.c
> --- a/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:24:53 2012 +0100
> +++ b/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:25:02 2012 +0100
> @@ -5,9 +5,9 @@ int libxl__blktap_enabled(libxl__gc *gc)
>      return 1;
>  }
>  
> -char *libxl__blktap_devpath(libxl__gc *gc, const char *disk,
> +int libxl__blktap_devpath(libxl__gc *gc, const char *disk,
>      libxl_disk_format format) {
> -    return NULL;
> +    return -ENOSYS;
>  }
>  
>  int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *be_path) {
> diff -r 80e0bc67dcda -r bcb5a6182868 tools/libxl/libxl_internal.h
> --- a/tools/libxl/libxl_internal.h	Wed Oct 24 17:24:53 2012 +0100
> +++ b/tools/libxl/libxl_internal.h	Wed Oct 24 17:25:02 2012 +0100
> @@ -1344,10 +1344,9 @@ struct libxl__cpuid_policy {
>  /* libxl__blktap_devpath:
>   *    Argument: path and disk image as specified in config file.
>   *      The type specifies whether this is aio, qcow, qcow2, etc.
> - *    returns device path xenstore wants to have. returns NULL
> - *      if no device corresponds to the disk.
> + *    returns 0 on success, an error code otherwise 
>   */
> -_hidden char *libxl__blktap_devpath(libxl__gc *gc,
> +_hidden int libxl__blktap_devpath(libxl__gc *gc,
>                                      const char *disk,
>                                      libxl_disk_format format);
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 04 of 16 RFC] blktap3: change libxl__blktap_devpath prototype to return an error code

[Thanos Makatos]

> -----Original Message-----
> From: Ian Campbell
> Sent: 26 October 2012 12:22
> To: Thanos Makatos
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH 04 of 16 RFC] blktap3: change
> libxl__blktap_devpath prototype to return an error code
> 
> On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> > Make libxl__blktap_devpath return an error code instead of the
> device,
> > since there is no device in dom0 any more.
> 
> Does this function eventually go away then? Or will it eventually
> return some sort of metadata which can be used to connect to the
> created tap device?

As in blktap2, it creates the tapdisk process, but doesn't return any metadata, so it's still required. I guess changing the name of the function would clarify it.

> 
> >  Amend the libxl code that uses this functions accordingly.
> >
> > diff -r 80e0bc67dcda -r bcb5a6182868 tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c	Wed Oct 24 17:24:53 2012 +0100
> > +++ b/tools/libxl/libxl.c	Wed Oct 24 17:25:02 2012 +0100
> > @@ -1844,13 +1844,14 @@ static void device_disk_add(libxl__egc *
> >                  break;
> >
> >              case LIBXL_DISK_BACKEND_TAP:
> > -                dev = libxl__blktap_devpath(gc, disk->pdev_path,
> disk->format);
> > -                if (!dev) {
> > -                    LOG(ERROR, "failed to get blktap devpath for
> %p\n",
> > -                        disk->pdev_path);
> > +                rc = libxl__blktap_devpath(gc, disk->pdev_path,
> disk->format);
> > +                if (rc) {
> > +                    LOG(ERROR, "failed to get blktap devpath for %s:
> %s\n",
> > +                        disk->pdev_path, strerror(rc));
> >                      rc = ERROR_FAIL;
> >                      goto out;
> >                  }
> > +                dev = NULL;
> >                  flexarray_append(back, "tapdisk-params");
> >                  flexarray_append(back, libxl__sprintf(gc, "%s:%s",
> >
> > libxl__device_disk_string_of_format(disk->format),
> > @@ -2277,8 +2278,13 @@ void libxl__device_disk_local_initiate_a
> >                  dev = disk->pdev_path;
> >                  break;
> >              case LIBXL_DISK_FORMAT_VHD:
> > -                dev = libxl__blktap_devpath(gc, disk->pdev_path,
> > -                                            disk->format);
> > +                rc = libxl__blktap_devpath(gc, disk->pdev_path,
> disk->format);
> > +                if (!rc) {
> > +                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> > +                           "error getting tapdisk: %s",
> strerror(rc));
> > +                    rc = ERROR_FAIL;
> > +                    goto out;
> > +                }
> >                  break;
> >              case LIBXL_DISK_FORMAT_QCOW:
> >              case LIBXL_DISK_FORMAT_QCOW2:
> > diff -r 80e0bc67dcda -r bcb5a6182868 tools/libxl/libxl_blktap3.c
> > --- a/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:24:53 2012 +0100
> > +++ b/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:25:02 2012 +0100
> > @@ -5,9 +5,9 @@ int libxl__blktap_enabled(libxl__gc *gc)
> >      return 1;
> >  }
> >
> > -char *libxl__blktap_devpath(libxl__gc *gc, const char *disk,
> > +int libxl__blktap_devpath(libxl__gc *gc, const char *disk,
> >      libxl_disk_format format) {
> > -    return NULL;
> > +    return -ENOSYS;
> >  }
> >
> >  int libxl__device_destroy_tapdisk(libxl__gc *gc, const char
> *be_path)
> > { diff -r 80e0bc67dcda -r bcb5a6182868 tools/libxl/libxl_internal.h
> > --- a/tools/libxl/libxl_internal.h	Wed Oct 24 17:24:53 2012
> +0100
> > +++ b/tools/libxl/libxl_internal.h	Wed Oct 24 17:25:02 2012
> +0100
> > @@ -1344,10 +1344,9 @@ struct libxl__cpuid_policy {
> >  /* libxl__blktap_devpath:
> >   *    Argument: path and disk image as specified in config file.
> >   *      The type specifies whether this is aio, qcow, qcow2, etc.
> > - *    returns device path xenstore wants to have. returns NULL
> > - *      if no device corresponds to the disk.
> > + *    returns 0 on success, an error code otherwise
> >   */
> > -_hidden char *libxl__blktap_devpath(libxl__gc *gc,
> > +_hidden int libxl__blktap_devpath(libxl__gc *gc,
> >                                      const char *disk,
> >                                      libxl_disk_format format);
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 

Re: [PATCH 04 of 16 RFC] blktap3: change libxl__blktap_devpath prototype to return an error code

[Ian Campbell]

On Mon, 2012-10-29 at 17:41 +0000, Thanos Makatos wrote:
> > -----Original Message-----
> > From: Ian Campbell
> > Sent: 26 October 2012 12:22
> > To: Thanos Makatos
> > Cc: xen-devel@lists.xensource.com
> > Subject: Re: [Xen-devel] [PATCH 04 of 16 RFC] blktap3: change
> > libxl__blktap_devpath prototype to return an error code
> > 
> > On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> > > Make libxl__blktap_devpath return an error code instead of the
> > device,
> > > since there is no device in dom0 any more.
> > 
> > Does this function eventually go away then? Or will it eventually
> > return some sort of metadata which can be used to connect to the
> > created tap device?
> 
> As in blktap2, it creates the tapdisk process, but doesn't return any
> metadata, so it's still required. I guess changing the name of the
> function would clarify it.

Yes, I think that would be helpful.

Ian.

[PATCH 05 of 16 RFC] blktap3: Don't check if blktap is available

[Thanos Makatos]

Don't check if blktap is enabled since it will always be enabled (no blktap/blkback requirement anymore).

diff -r bcb5a6182868 -r 57896068356d tools/libxl/libxl_blktap3.c
--- a/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:25:02 2012 +0100
+++ b/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:25:12 2012 +0100
@@ -1,10 +1,6 @@
 #include "libxl_osdeps.h"
 #include "libxl_internal.h"
 
-int libxl__blktap_enabled(libxl__gc *gc) {
-    return 1;
-}
-
 int libxl__blktap_devpath(libxl__gc *gc, const char *disk,
     libxl_disk_format format) {
     return -ENOSYS;
diff -r bcb5a6182868 -r 57896068356d tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Wed Oct 24 17:25:02 2012 +0100
+++ b/tools/libxl/libxl_device.c	Wed Oct 24 17:25:12 2012 +0100
@@ -177,13 +177,6 @@ static int disk_try_backend(disk_try_bac
 
     case LIBXL_DISK_BACKEND_TAP:
         if (a->disk->script) goto bad_script;
-
-        if (!libxl__blktap_enabled(a->gc)) {
-            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend tap"
-                       " unsuitable because blktap not available",
-                       a->disk->vdev);
-            return 0;
-        }
         if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW ||
               a->disk->format == LIBXL_DISK_FORMAT_VHD)) {
             goto bad_format;

Re: [PATCH 05 of 16 RFC] blktap3: Don't check if blktap is available

[Ian Campbell]

On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> Don't check if blktap is enabled since it will always be enabled (no
> blktap/blkback requirement anymore).

I think this needs to stay until the *BSD dom0's implement the necessary
kernel drivers (gnttab/gntalloc etc) such that blktap3 can run on them.
Currently we use libxl_noblktap2.c on those platforms and I think until
then we need to retain a libxl_noblktap3.c for use there.

Perhaps even on Linux we would want a sanity check that the drivers are
present and loaded etc?

> 
> diff -r bcb5a6182868 -r 57896068356d tools/libxl/libxl_blktap3.c
> --- a/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:25:02 2012 +0100
> +++ b/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:25:12 2012 +0100
> @@ -1,10 +1,6 @@
>  #include "libxl_osdeps.h"
>  #include "libxl_internal.h"
>  
> -int libxl__blktap_enabled(libxl__gc *gc) {
> -    return 1;
> -}
> -
>  int libxl__blktap_devpath(libxl__gc *gc, const char *disk,
>      libxl_disk_format format) {
>      return -ENOSYS;
> diff -r bcb5a6182868 -r 57896068356d tools/libxl/libxl_device.c
> --- a/tools/libxl/libxl_device.c	Wed Oct 24 17:25:02 2012 +0100
> +++ b/tools/libxl/libxl_device.c	Wed Oct 24 17:25:12 2012 +0100
> @@ -177,13 +177,6 @@ static int disk_try_backend(disk_try_bac
>  
>      case LIBXL_DISK_BACKEND_TAP:
>          if (a->disk->script) goto bad_script;
> -
> -        if (!libxl__blktap_enabled(a->gc)) {
> -            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend tap"
> -                       " unsuitable because blktap not available",
> -                       a->disk->vdev);
> -            return 0;
> -        }
>          if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW ||
>                a->disk->format == LIBXL_DISK_FORMAT_VHD)) {
>              goto bad_format;
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 05 of 16 RFC] blktap3: Don't check if blktap is available

[Thanos Makatos]

> -----Original Message-----
> From: Ian Campbell
> Sent: 26 October 2012 12:24
> To: Thanos Makatos
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH 05 of 16 RFC] blktap3: Don't check if
> blktap is available
> 
> On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> > Don't check if blktap is enabled since it will always be enabled (no
> > blktap/blkback requirement anymore).
> 
> I think this needs to stay until the *BSD dom0's implement the
> necessary kernel drivers (gnttab/gntalloc etc) such that blktap3 can
> run on them.
> Currently we use libxl_noblktap2.c on those platforms and I think until
> then we need to retain a libxl_noblktap3.c for use there.

Ok.

> 
> Perhaps even on Linux we would want a sanity check that the drivers are
> present and loaded etc?

We could ensure that the tapdisk binaries are present, apart from that I don't see anything else we could check.

> 
> >
> > diff -r bcb5a6182868 -r 57896068356d tools/libxl/libxl_blktap3.c
> > --- a/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:25:02 2012 +0100
> > +++ b/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:25:12 2012 +0100
> > @@ -1,10 +1,6 @@
> >  #include "libxl_osdeps.h"
> >  #include "libxl_internal.h"
> >
> > -int libxl__blktap_enabled(libxl__gc *gc) {
> > -    return 1;
> > -}
> > -
> >  int libxl__blktap_devpath(libxl__gc *gc, const char *disk,
> >      libxl_disk_format format) {
> >      return -ENOSYS;
> > diff -r bcb5a6182868 -r 57896068356d tools/libxl/libxl_device.c
> > --- a/tools/libxl/libxl_device.c	Wed Oct 24 17:25:02 2012 +0100
> > +++ b/tools/libxl/libxl_device.c	Wed Oct 24 17:25:12 2012 +0100
> > @@ -177,13 +177,6 @@ static int disk_try_backend(disk_try_bac
> >
> >      case LIBXL_DISK_BACKEND_TAP:
> >          if (a->disk->script) goto bad_script;
> > -
> > -        if (!libxl__blktap_enabled(a->gc)) {
> > -            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend
> tap"
> > -                       " unsuitable because blktap not available",
> > -                       a->disk->vdev);
> > -            return 0;
> > -        }
> >          if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW ||
> >                a->disk->format == LIBXL_DISK_FORMAT_VHD)) {
> >              goto bad_format;
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 

Re: [PATCH 05 of 16 RFC] blktap3: Don't check if blktap is available

[Ian Campbell]

> > 
> > Perhaps even on Linux we would want a sanity check that the drivers are
> > present and loaded etc?
> 
> We could ensure that the tapdisk binaries are present, apart from that
> I don't see anything else we could check.

Could we check for /dev/xen/{evtchn,gntdev,...} or whatever else it
needs?

Ian.

Re: [PATCH 05 of 16 RFC] blktap3: Don't check if blktap is available

[Roger Pau Monné]

On 30/10/12 08:41, Ian Campbell wrote:
>>>
>>> Perhaps even on Linux we would want a sanity check that the drivers are
>>> present and loaded etc?
>>
>> We could ensure that the tapdisk binaries are present, apart from that
>> I don't see anything else we could check.
> 
> Could we check for /dev/xen/{evtchn,gntdev,...} or whatever else it
> needs?

We could also check with xc_gnttab_open, which will return an error on
NetBSD.

Re: [PATCH 05 of 16 RFC] blktap3: Don't check if blktap is available

[Thanos Makatos]

> -----Original Message-----
> From: Ian Campbell
> Sent: 30 October 2012 07:42
> To: Thanos Makatos
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH 05 of 16 RFC] blktap3: Don't check if
> blktap is available
> 
> > >
> > > Perhaps even on Linux we would want a sanity check that the drivers
> > > are present and loaded etc?
> >
> > We could ensure that the tapdisk binaries are present, apart from
> that
> > I don't see anything else we could check.
> 
> Could we check for /dev/xen/{evtchn,gntdev,...} or whatever else it
> needs?

Sure.

> 
> Ian.

[PATCH 06 of 16 RFC] blktap3: Remove physical device creation for libxl

[Thanos Makatos]

blktap3 doesn't need a device in dom0.

diff -r 57896068356d -r 5a0b0e799ae8 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Wed Oct 24 17:25:12 2012 +0100
+++ b/tools/libxl/libxl.c	Wed Oct 24 17:25:26 2012 +0100
@@ -1818,7 +1818,6 @@ static void device_disk_add(libxl__egc *
             case LIBXL_DISK_BACKEND_PHY:
                 dev = disk->pdev_path;
 
-        do_backend_phy:
                 flexarray_append(back, "params");
                 flexarray_append(back, dev);
 
@@ -1856,13 +1855,7 @@ static void device_disk_add(libxl__egc *
                 flexarray_append(back, libxl__sprintf(gc, "%s:%s",
                     libxl__device_disk_string_of_format(disk->format),
                     disk->pdev_path));
-
-                /* tap backends with scripts are rejected by
-                 * libxl__device_disk_set_backend */
-                assert(!disk->script);
-
-                /* now create a phy device to export the device to the guest */
-                goto do_backend_phy;
+                break;
             case LIBXL_DISK_BACKEND_QDISK:
                 flexarray_append(back, "params");
                 flexarray_append(back, libxl__sprintf(gc, "%s:%s",

[PATCH 07 of 16 RFC] blktap3: Introduce blktap3 top-level Makefile

[Thanos Makatos]

diff -r 5a0b0e799ae8 -r 17eea5a9a20c tools/blktap3/Makefile
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/Makefile	Wed Oct 24 17:25:32 2012 +0100
@@ -0,0 +1,12 @@
+XEN_ROOT := $(CURDIR)/../../
+include $(XEN_ROOT)/tools/Rules.mk
+
+all: build
+
+build: ;
+
+clean: ;
+
+install: ;
+
+.PHONY: all build clean install

Re: [PATCH 07 of 16 RFC] blktap3: Introduce blktap3 top-level Makefile

[Ian Campbell]

On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> diff -r 5a0b0e799ae8 -r 17eea5a9a20c tools/blktap3/Makefile
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/blktap3/Makefile	Wed Oct 24 17:25:32 2012 +0100
> @@ -0,0 +1,12 @@
> +XEN_ROOT := $(CURDIR)/../../
> +include $(XEN_ROOT)/tools/Rules.mk
> +
> +all: build
> +
> +build: ;

What does the ";" in this syntax do?

> +
> +clean: ;
> +
> +install: ;
> +
> +.PHONY: all build clean install
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 07 of 16 RFC] blktap3: Introduce blktap3 top-level Makefile

[Philipp Hahn]

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

Hello Ian,

On Friday 26 October 2012 13:25:01 Ian Campbell wrote:
> > +build: ;
>
> What does the ";" in this syntax do?

A Make trick to do (really) nothing:
<http://www.gnu.org/software/make/manual/html_node/Empty-Recipes.html#Empty-Recipes>

Sincerely
Philipp
-- 
Philipp Hahn           Open Source Software Engineer      hahn@univention.de
Univention GmbH        be open.                       fon: +49 421 22 232- 0
Mary-Somerville-Str.1  D-28359 Bremen                 fax: +49 421 22 232-99
                                                   http://www.univention.de/

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

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

Re: [PATCH 07 of 16 RFC] blktap3: Introduce blktap3 top-level Makefile

[Ian Campbell]

On Fri, 2012-10-26 at 21:04 +0100, Philipp Hahn wrote:
> Hello Ian,
> 
> On Friday 26 October 2012 13:25:01 Ian Campbell wrote:
> > > +build: ;
> >
> > What does the ";" in this syntax do?
> 
> A Make trick to do (really) nothing:
> <http://www.gnu.org/software/make/manual/html_node/Empty-Recipes.html#Empty-Recipes>

Ah OK, thanks for the link.

It does say that this doesn't make much sense for phony targets, which
all of these were.

Ian.


> Sincerely
> Philipp

[PATCH 08 of 16 RFC] blktap3: Introduce message types and structures

[Thanos Makatos]

The main difference over the the XEN version is the addition of the stats
structure (used for retrieving stats from a running tapdisk), and the addition
of the tapdisk_message_blkif struct, which is required for the user-space
counterpart of blktap/blkback to communicate with tapdisk.

diff -r 17eea5a9a20c -r 23e2537c7257 tools/blktap3/include/tapdisk-message.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/include/tapdisk-message.h	Wed Oct 24 17:25:37 2012 +0100
@@ -0,0 +1,267 @@
+/* FIXME this is the github blktap2 license (which is the same the blktap2
+ * license)
+ *
+ * Copyright (c) 2008, XenSource Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in the
+ *       documentation and/or other materials provided with the distribution.
+ *     * Neither the name of XenSource Inc. nor the names of its contributors
+ *       may be used to endorse or promote products derived from this software
+ *       without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER
+ * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+ * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef __TAPDISK_MESSAGE_H__
+#define __TAPDISK_MESSAGE_H__
+
+#include <inttypes.h>
+#include <sys/types.h>
+
+#define TAPDISK_MESSAGE_MAX_PATH_LENGTH  256
+#define TAPDISK_MESSAGE_STRING_LENGTH    256
+
+#define TAPDISK_MESSAGE_MAX_MINORS \
+    ((TAPDISK_MESSAGE_MAX_PATH_LENGTH / sizeof(int)) - 1)
+
+#define TAPDISK_MESSAGE_FLAG_SHARED      0x001
+#define TAPDISK_MESSAGE_FLAG_RDONLY      0x002
+#define TAPDISK_MESSAGE_FLAG_ADD_CACHE   0x004
+#define TAPDISK_MESSAGE_FLAG_VHD_INDEX   0x008
+#define TAPDISK_MESSAGE_FLAG_LOG_DIRTY   0x010
+#define TAPDISK_MESSAGE_FLAG_ADD_LCACHE  0x020
+#define TAPDISK_MESSAGE_FLAG_REUSE_PRT   0x040
+#define TAPDISK_MESSAGE_FLAG_SECONDARY   0x080
+#define TAPDISK_MESSAGE_FLAG_STANDBY     0x100
+
+typedef struct tapdisk_message tapdisk_message_t;
+typedef uint32_t tapdisk_message_flag_t;
+typedef struct tapdisk_message_image tapdisk_message_image_t;
+typedef struct tapdisk_message_params tapdisk_message_params_t;
+typedef struct tapdisk_message_string tapdisk_message_string_t;
+typedef struct tapdisk_message_response tapdisk_message_response_t;
+typedef struct tapdisk_message_minors tapdisk_message_minors_t;
+typedef struct tapdisk_message_list tapdisk_message_list_t;
+typedef struct tapdisk_message_stat tapdisk_message_stat_t;
+typedef struct tapdisk_message_blkif tapdisk_message_blkif_t;
+
+struct tapdisk_message_params {
+    tapdisk_message_flag_t flags;
+
+    uint32_t devnum;
+    uint32_t domid;
+    char path[TAPDISK_MESSAGE_MAX_PATH_LENGTH];
+    uint32_t prt_devnum;
+    char secondary[TAPDISK_MESSAGE_MAX_PATH_LENGTH];
+};
+
+struct tapdisk_message_image {
+    uint64_t sectors;
+    uint32_t sector_size;
+    uint32_t info;
+};
+
+struct tapdisk_message_string {
+    char text[TAPDISK_MESSAGE_STRING_LENGTH];
+};
+
+struct tapdisk_message_response {
+    int error;
+    char message[TAPDISK_MESSAGE_STRING_LENGTH];
+};
+
+struct tapdisk_message_minors {
+    int count;
+    int list[TAPDISK_MESSAGE_MAX_MINORS];
+};
+
+struct tapdisk_message_list {
+    int count;
+    int minor;
+    int state;
+    char path[TAPDISK_MESSAGE_MAX_PATH_LENGTH];
+};
+
+struct tapdisk_message_stat {
+    uint16_t type;
+    uint16_t cookie;
+    size_t length;
+};
+
+struct tapdisk_message_blkif {
+    uint32_t domid;
+    uint32_t devid;
+    uint32_t gref[8];
+    uint32_t order;
+    uint32_t proto;
+    char pool[TAPDISK_MESSAGE_STRING_LENGTH];
+    uint32_t port;
+};
+
+struct tapdisk_message {
+    uint16_t type;
+    uint16_t cookie;
+
+    union {
+        pid_t tapdisk_pid;
+        tapdisk_message_image_t image;
+        tapdisk_message_params_t params;
+        tapdisk_message_string_t string;
+        tapdisk_message_minors_t minors;
+        tapdisk_message_response_t response;
+        tapdisk_message_list_t list;
+        tapdisk_message_stat_t info;
+        tapdisk_message_blkif_t blkif;
+    } u;
+};
+
+enum tapdisk_message_id {
+    TAPDISK_MESSAGE_ERROR = 1,
+    TAPDISK_MESSAGE_RUNTIME_ERROR,
+    TAPDISK_MESSAGE_PID,
+    TAPDISK_MESSAGE_PID_RSP,
+    TAPDISK_MESSAGE_ATTACH,
+    TAPDISK_MESSAGE_ATTACH_RSP,
+    TAPDISK_MESSAGE_OPEN,
+    TAPDISK_MESSAGE_OPEN_RSP,
+    TAPDISK_MESSAGE_PAUSE,
+    TAPDISK_MESSAGE_PAUSE_RSP,
+    TAPDISK_MESSAGE_RESUME,
+    TAPDISK_MESSAGE_RESUME_RSP,
+    TAPDISK_MESSAGE_CLOSE,
+    TAPDISK_MESSAGE_CLOSE_RSP,
+    TAPDISK_MESSAGE_DETACH,
+    TAPDISK_MESSAGE_DETACH_RSP,
+    TAPDISK_MESSAGE_LIST_MINORS,
+    TAPDISK_MESSAGE_LIST_MINORS_RSP,
+    TAPDISK_MESSAGE_LIST,
+    TAPDISK_MESSAGE_LIST_RSP,
+    TAPDISK_MESSAGE_STATS,
+    TAPDISK_MESSAGE_STATS_RSP,
+    TAPDISK_MESSAGE_FORCE_SHUTDOWN,
+    TAPDISK_MESSAGE_DISK_INFO,
+    TAPDISK_MESSAGE_DISK_INFO_RSP,
+    TAPDISK_MESSAGE_XENBLKIF_CONNECT,
+    TAPDISK_MESSAGE_XENBLKIF_CONNECT_RSP,
+    TAPDISK_MESSAGE_XENBLKIF_DISCONNECT,
+    TAPDISK_MESSAGE_XENBLKIF_DISCONNECT_RSP,
+    TAPDISK_MESSAGE_EXIT,
+};
+
+#define TAPDISK_MESSAGE_MAX TAPDISK_MESSAGE_EXIT
+
+/**
+ * Retrieves the human-readable representation of the type of the message.
+ */
+static inline char *tapdisk_message_name(enum tapdisk_message_id id)
+{
+    switch (id) {
+    case TAPDISK_MESSAGE_ERROR:
+        return "error";
+
+    case TAPDISK_MESSAGE_PID:
+        return "pid";
+
+    case TAPDISK_MESSAGE_PID_RSP:
+        return "pid response";
+
+    case TAPDISK_MESSAGE_OPEN:
+        return "open";
+
+    case TAPDISK_MESSAGE_OPEN_RSP:
+        return "open response";
+
+    case TAPDISK_MESSAGE_PAUSE:
+        return "pause";
+
+    case TAPDISK_MESSAGE_PAUSE_RSP:
+        return "pause response";
+
+    case TAPDISK_MESSAGE_RESUME:
+        return "resume";
+
+    case TAPDISK_MESSAGE_RESUME_RSP:
+        return "resume response";
+
+    case TAPDISK_MESSAGE_CLOSE:
+        return "close";
+
+    case TAPDISK_MESSAGE_FORCE_SHUTDOWN:
+        return "force shutdown";
+
+    case TAPDISK_MESSAGE_CLOSE_RSP:
+        return "close response";
+
+    case TAPDISK_MESSAGE_ATTACH:
+        return "attach";
+
+    case TAPDISK_MESSAGE_ATTACH_RSP:
+        return "attach response";
+
+    case TAPDISK_MESSAGE_DETACH:
+        return "detach";
+
+    case TAPDISK_MESSAGE_DETACH_RSP:
+        return "detach response";
+
+    case TAPDISK_MESSAGE_LIST_MINORS:
+        return "list minors";
+
+    case TAPDISK_MESSAGE_LIST_MINORS_RSP:
+        return "list minors response";
+
+    case TAPDISK_MESSAGE_LIST:
+        return "list";
+
+    case TAPDISK_MESSAGE_LIST_RSP:
+        return "list response";
+
+    case TAPDISK_MESSAGE_STATS:
+        return "stats";
+
+    case TAPDISK_MESSAGE_STATS_RSP:
+        return "stats response";
+
+    case TAPDISK_MESSAGE_DISK_INFO:
+        return "disk info";
+
+    case TAPDISK_MESSAGE_DISK_INFO_RSP:
+        return "disk info response";
+
+    case TAPDISK_MESSAGE_XENBLKIF_CONNECT:
+        return "blkif connect";
+
+    case TAPDISK_MESSAGE_XENBLKIF_CONNECT_RSP:
+        return "blkif connect response";
+
+    case TAPDISK_MESSAGE_XENBLKIF_DISCONNECT:
+        return "blkif disconnect";
+
+    case TAPDISK_MESSAGE_XENBLKIF_DISCONNECT_RSP:
+        return "blkif disconnect response";
+
+    case TAPDISK_MESSAGE_EXIT:
+        return "exit";
+
+    default:
+        return "unknown";
+    }
+}
+
+#endif /* __TAPDISK_MESSAGE_H__ */

Re: [PATCH 08 of 16 RFC] blktap3: Introduce message types and structures

[Ian Campbell]

On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> The main difference over the the XEN version

what do you mean by "the XEN version"? Is there an existing version of
this header somewhere?

>  is the addition of the stats
> structure (used for retrieving stats from a running tapdisk), and the addition
> of the tapdisk_message_blkif struct, which is required for the user-space
> counterpart of blktap/blkback to communicate with tapdisk.

Am I correct that the interface defined here is one which is private
between the tapdisk processes and the supplied control tools, abstracted
away via libblktapctl?

> +/**
> + * Retrieves the human-readable representation of the type of the message.
> + */
> +static inline char *tapdisk_message_name(enum tapdisk_message_id id)
> +{
> +    switch (id) {

Would a static lookup table be nicer?

Ian.

Re: [PATCH 08 of 16 RFC] blktap3: Introduce message types and structures

[Thanos Makatos]

> -----Original Message-----
> From: Ian Campbell
> Sent: 26 October 2012 12:28
> To: Thanos Makatos
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH 08 of 16 RFC] blktap3: Introduce
> message types and structures
> 
> On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> > The main difference over the the XEN version
> 
> what do you mean by "the XEN version"? Is there an existing version of
> this header somewhere?

I mean the blktap2 living in XEN, as opposed to the forked blktap2 living in github.

> 
> >  is the addition of the stats
> > structure (used for retrieving stats from a running tapdisk), and the
> > addition of the tapdisk_message_blkif struct, which is required for
> > the user-space counterpart of blktap/blkback to communicate with
> tapdisk.
> 
> Am I correct that the interface defined here is one which is private
> between the tapdisk processes and the supplied control tools,
> abstracted away via libblktapctl?

Yes, so this header should be private.

> 
> > +/**
> > + * Retrieves the human-readable representation of the type of the
> message.
> > + */
> > +static inline char *tapdisk_message_name(enum tapdisk_message_id id)
> > +{
> > +    switch (id) {
> 
> Would a static lookup table be nicer?

Definitely.

> 
> Ian.

[PATCH 09 of 16 RFC] blktap3: Introduce the tapdisk control function prototypes

[Thanos Makatos]

This file is based on the github blktap2, which in turn is based on the XEN
blktap2. Most of the changes here are prototype function changes introduced by
the the github blktap2. In blktap3, Linux lists are replaced with BSD tail
queues.

diff -r 23e2537c7257 -r efb6b2844256 tools/blktap3/include/tap-ctl.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/include/tap-ctl.h	Wed Oct 24 17:26:04 2012 +0100
@@ -0,0 +1,184 @@
+/*
+ * FIXME this is the github blktap2 license
+ *
+ * Copyright (c) 2008, XenSource Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in the
+ *       documentation and/or other materials provided with the distribution.
+ *     * Neither the name of XenSource Inc. nor the names of its contributors
+ *       may be used to endorse or promote products derived from this software
+ *       without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER
+ * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+ * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef __TAP_CTL_H__
+#define __TAP_CTL_H__
+
+#include <syslog.h>
+#include <errno.h>
+#include <sys/time.h>
+#include <tapdisk-message.h>
+#include "blktap.h"
+
+/**
+ * Contains information about a tapdisk process.
+ */
+typedef struct tap_list {
+
+    /**
+     * The process ID.
+     */
+    pid_t pid;
+
+    /**
+     * TODO? 
+     */
+    int minor;
+
+    /**
+     * State of the VBD, specified in drivers/tapdisk-vbd.h.
+     */
+    int state;
+
+    /**
+     * TODO type?
+     */
+    char *type;
+
+    /**
+     * /path/to/file
+     */
+    char *path;
+
+    /**
+     * for linked lists
+     */
+    TAILQ_ENTRY(tap_list) entry;
+} tap_list_t;
+
+TAILQ_HEAD(tqh_tap_list, tap_list);
+
+/**
+ * Iterate over a list of struct tap_list.
+ */
+#define tap_list_for_each_entry(_pos, _head) \
+    TAILQ_FOREACH(_pos, _head, entry)
+
+/**
+ * Iterate over a list of struct tap_list allowing deletions without having to
+ * restart the iteration.
+ */
+#define tap_list_for_each_entry_safe(_pos, _n, _head) \
+    TAILQ_FOREACH_SAFE(_pos, _head, entry, _n)
+
+/**
+ * Connects to a tapdisk.
+ *
+ * @param /path/to/file of the control socket (e.g. /var/run/blktap-control/ctl/<pid>
+ * @param socket output parameter that receives the connection
+ * @returns 0 on success, an error code otherwise
+ */
+int tap_ctl_connect(const char *path, int *socket);
+
+/**
+ * Connects to a tapdisk.
+ *
+ * @param id the process ID of the tapdisk to connect to
+ * @param socket output parameter that receives the connection
+ * @returns 0 on success, an error code otherwise
+ */
+int tap_ctl_connect_id(const int id, int *socket);
+
+/**
+ * Reads from the tapdisk connection to the buffer.
+ *
+ * @param fd the file descriptor of the socket to read from
+ * @param buf buffer that receives the output
+ * @param sz size, in bytes, of the buffer
+ * @param timeout (optional) specifies the maximum time to wait for reading
+ * @returns 0 on success, an error code otherwise
+ */
+int tap_ctl_read_raw(const int fd, void *buf, const size_t sz,
+        struct timeval *timeout);
+
+int tap_ctl_read_message(int fd, tapdisk_message_t * message,
+                         struct timeval *timeout);
+int tap_ctl_write_message(int fd, tapdisk_message_t * message,
+                          struct timeval *timeout);
+int tap_ctl_send_and_receive(int fd, tapdisk_message_t * message,
+                             struct timeval *timeout);
+int tap_ctl_connect_send_and_receive(int id, tapdisk_message_t * message,
+                                     struct timeval *timeout);
+char *tap_ctl_socket_name(int id);
+int tap_ctl_list_pid(pid_t pid, struct tqh_tap_list *list);
+
+
+/**
+ * Retrieves a list of all tapdisks.
+ *
+ * @param list output parameter that receives the list of tapdisks
+ * @returns 0 on success, an error code otherwise
+ */
+int tap_ctl_list(struct tqh_tap_list *list);
+
+/**
+ * Deallocates a list of struct tap_list.
+ *
+ * @param list the tapdisk information structure to deallocate.
+ */
+void tap_ctl_list_free(struct tqh_tap_list *list);
+
+/**
+ * Creates a tapdisk process.
+ *
+ * TODO document parameters
+ * @param params
+ * @param flags
+ * @param prt_minor
+ * @param secondary
+ * @returns 0 on success, en error code otherwise
+ */
+int tap_ctl_create(const char *params, int flags, int prt_minor,
+                   char *secondary);
+int tap_ctl_destroy(const int id, const int minor, int force,
+                    struct timeval *timeout);
+
+/*
+ * TODO The following functions are not currently used by anything else other
+ * than the tapdisk itself. Move to a private header?
+ */
+int tap_ctl_spawn(void);
+pid_t tap_ctl_get_pid(const int id);
+
+int tap_ctl_attach(const int id, const int minor);
+int tap_ctl_detach(const int id, const int minor);
+
+int tap_ctl_open(const int id, const int minor, const char *params,
+                 int flags, const int prt_minor, const char *secondary);
+int tap_ctl_close(const int id, const int minor, const int force,
+                  struct timeval *timeout);
+
+int tap_ctl_pause(const int id, const int minor, struct timeval *timeout);
+int tap_ctl_unpause(const int id, const int minor, const char *params);
+
+ssize_t tap_ctl_stats(pid_t pid, int minor, char *buf, size_t size);
+int tap_ctl_stats_fwrite(pid_t pid, int minor, FILE * out);
+
+#endif /* __TAP_CTL_H__ */

Re: [PATCH 09 of 16 RFC] blktap3: Introduce the tapdisk control function prototypes

[Ian Campbell]

On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:

> +    /**
> +     * for linked lists
> +     */
> +    TAILQ_ENTRY(tap_list) entry;

This file doesn't seem to include the necessary header to define this?
(Perhaps it is indirectly included?)

Anyway, since I think this header is intended to be used by consumers of
blktap (rather than internally) I think you need to namespace the
macros. See tools/include/xen-external/bsd-sys-queue-h-seddery and its
usages in tools/libxl/Makefile and extras/mini-os/Makefile to create
local namespaced versions for libxl and mini-os.

Re: [PATCH 09 of 16 RFC] blktap3: Introduce the tapdisk control function prototypes

[Thanos Makatos]

> -----Original Message-----
> From: Ian Campbell
> Sent: 26 October 2012 12:33
> To: Thanos Makatos
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH 09 of 16 RFC] blktap3: Introduce the
> tapdisk control function prototypes
> 
> On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> 
> > +    /**
> > +     * for linked lists
> > +     */
> > +    TAILQ_ENTRY(tap_list) entry;
> 
> This file doesn't seem to include the necessary header to define this?
> (Perhaps it is indirectly included?)

It's provided by blktap.h. My plan is to put commonly used header files in a single header file instead of repeating the inclusion in each and every source file.

> 
> Anyway, since I think this header is intended to be used by consumers
> of blktap (rather than internally) I think you need to namespace the
> macros. See tools/include/xen-external/bsd-sys-queue-h-seddery and its
> usages in tools/libxl/Makefile and extras/mini-os/Makefile to create
> local namespaced versions for libxl and mini-os.
> 
Just out of curiosity, what are the benefits of namespacing the macros? Google doesn't give anything relevant.

Re: [PATCH 09 of 16 RFC] blktap3: Introduce the tapdisk control function prototypes

[Ian Campbell]

> > Anyway, since I think this header is intended to be used by consumers
> > of blktap (rather than internally) I think you need to namespace the
> > macros. See tools/include/xen-external/bsd-sys-queue-h-seddery and its
> > usages in tools/libxl/Makefile and extras/mini-os/Makefile to create
> > local namespaced versions for libxl and mini-os.
> > 
> Just out of curiosity, what are the benefits of namespacing the
> macros? Google doesn't give anything relevant.

It's just good policy for libraries to avoid clashing with names defined
by the application which uses them, or with other libraries which that
library might be using simultaneously.

IOW an application might have it's own TAILQ_ENTRY macro which has
nothing to do with the one you would like to use. By naming it
BLKTAP_TAILQ_ENTRY in blktap (and in general avoiding generic sounding
names) you minimise the chances of this sort of thing.

Of course if a header/type is purely internal to blktap then this
doesn't matter.

Ian.

[PATCH 10 of 16 RFC] blktap3: Introduce the blktap header file

[Thanos Makatos]

In this file various file-system paths are specified, similar to the XEN
blktap2. These defines need to be documented. Unused defines (i.e. IOCTLs) have
been removed.

diff -r efb6b2844256 -r a17c7217bef2 tools/blktap3/include/blktap.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/include/blktap.h	Wed Oct 24 17:26:14 2012 +0100
@@ -0,0 +1,85 @@
+/*
+ * FIXME this is the github blktap2 license.
+ *
+ * Copyright (c) 2008, XenSource Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in the
+ *       documentation and/or other materials provided with the distribution.
+ *     * Neither the name of XenSource Inc. nor the names of its contributors
+ *       may be used to endorse or promote products derived from this software
+ *       without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER
+ * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+ * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef __BLKTAP_3_H__
+#define __BLKTAP_3_H__
+
+#include <xen-external/bsd-sys-queue.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <stdio.h>
+
+/*
+ * TODO Document what each define is.
+ */
+#define BLKTAP3_SYSFS_DIR                   "/sys/class/blktap3"
+#define BLKTAP3_CONTROL_NAME                "blktap-control"
+#define BLKTAP3_CONTROL_DIR                 "/var/run/"BLKTAP3_CONTROL_NAME
+#define BLKTAP3_CONTROL_SOCKET              "ctl"
+#define BLKTAP3_DIRECTORY                   "/dev/xen/blktap-3"
+#define BLKTAP3_RING_DEVICE                 BLKTAP3_DIRECTORY"/blktap"
+#define BLKTAP3_IO_DEVICEBLKTAP3_DIRECTORY  "/tapdev"
+#define BLKTAP3_ENOSPC_SIGNAL_FILE          "/var/run/tapdisk3-enospc"
+
+/*
+ * TODO Can these be supplied from somewhere else?
+ */
+#define likely(_cond)   __builtin_expect(!!(_cond), 1)
+#define unlikely(_cond) __builtin_expect(!!(_cond), 0)
+
+/*
+ * FIXME taken from list.h
+ */
+#define containerof(_ptr, _type, _memb) \
+    ((_type*)((void*)(_ptr) - offsetof(_type, _memb)))
+
+/*
+ * FIXME shouldn't these be supplied from somewhere?
+ */
+#define __printf(a, b) __attribute__((format(printf, a, b)))
+#define __scanf(_f, _a) __attribute__((format (scanf, _f, _a)))
+
+#define TAILQ_MOVE_HEAD(node, src, dst, entry)  \
+    TAILQ_REMOVE(src, node, entry);             \
+    TAILQ_INSERT_HEAD(dst, node, entry);
+
+#define TAILQ_MOVE_TAIL(node, src, dst, entry)  \
+    TAILQ_REMOVE(src, node, entry);             \
+    TAILQ_INSERT_TAIL(dst, node, entry);
+
+/*
+ * TODO This is defined in xen/lib.h, use that instead of redefining it
+ * here.
+ */
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#endif /* ARRAY_SIZE */
+
+#endif /* __BLKTAP_3_H__ */

Re: [PATCH 10 of 16 RFC] blktap3: Introduce the blktap header file

[Ian Campbell]

On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> In this file various file-system paths are specified, similar to the XEN
> blktap2. These defines need to be documented. Unused defines (i.e. IOCTLs) have
> been removed.
> 
> diff -r efb6b2844256 -r a17c7217bef2 tools/blktap3/include/blktap.h
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/blktap3/include/blktap.h	Wed Oct 24 17:26:14 2012 +0100
> @@ -0,0 +1,85 @@
> +/*
> + * FIXME this is the github blktap2 license.
> + *
> + * Copyright (c) 2008, XenSource Inc.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are met:
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in the
> + *       documentation and/or other materials provided with the distribution.
> + *     * Neither the name of XenSource Inc. nor the names of its contributors
> + *       may be used to endorse or promote products derived from this software
> + *       without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER
> + * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
> + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef __BLKTAP_3_H__
> +#define __BLKTAP_3_H__
> +
> +#include <xen-external/bsd-sys-queue.h>
> +#include <errno.h>
> +#include <sys/types.h>
> +#include <stdio.h>
> +
> +/*
> + * TODO Document what each define is.
> + */
> +#define BLKTAP3_SYSFS_DIR                   "/sys/class/blktap3"

I thought there was no kernel side to blktap3?

> +#define BLKTAP3_CONTROL_NAME                "blktap-control"
> +#define BLKTAP3_CONTROL_DIR                 "/var/run/"BLKTAP3_CONTROL_NAME
> +#define BLKTAP3_CONTROL_SOCKET              "ctl"

> +#define BLKTAP3_DIRECTORY                   "/dev/xen/blktap-3"
> +#define BLKTAP3_RING_DEVICE                 BLKTAP3_DIRECTORY"/blktap"

Likewise these two.

> +#define BLKTAP3_IO_DEVICEBLKTAP3_DIRECTORY  "/tapdev"
> +#define BLKTAP3_ENOSPC_SIGNAL_FILE          "/var/run/tapdisk3-enospc"
> +
> +/*
> + * TODO Can these be supplied from somewhere else?
> + */
> +#define likely(_cond)   __builtin_expect(!!(_cond), 1)
> +#define unlikely(_cond) __builtin_expect(!!(_cond), 0)
> +
> +/*
> + * FIXME taken from list.h
> + */
> +#define containerof(_ptr, _type, _memb) \
> +    ((_type*)((void*)(_ptr) - offsetof(_type, _memb)))
> +
> +/*
> + * FIXME shouldn't these be supplied from somewhere?
> + */
> +#define __printf(a, b) __attribute__((format(printf, a, b)))
> +#define __scanf(_f, _a) __attribute__((format (scanf, _f, _a)))

These are all abstractions of gcc-isms (presumably to ease ports to
other compilers). We generally don't bother with this sort of thing in
the tools, although you can if you like. You might want to put into a
compiler.h and add suitable #if like those in
xen/include/xen/compiler.h.

> +
> +#define TAILQ_MOVE_HEAD(node, src, dst, entry)  \
> +    TAILQ_REMOVE(src, node, entry);             \
> +    TAILQ_INSERT_HEAD(dst, node, entry);
> +
> +#define TAILQ_MOVE_TAIL(node, src, dst, entry)  \
> +    TAILQ_REMOVE(src, node, entry);             \
> +    TAILQ_INSERT_TAIL(dst, node, entry);
> +
> +/*
> + * TODO This is defined in xen/lib.h, use that instead of redefining it
> + * here.

xen/lib.h is a hypervisor internal header so isn't available to you
here. I think it is fine to define your own (although it should be in a
private header, not part of the libblktap API)

> + */
> +#ifndef ARRAY_SIZE
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +#endif /* ARRAY_SIZE */
> +
> +#endif /* __BLKTAP_3_H__ */
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 10 of 16 RFC] blktap3: Introduce the blktap header file

[Thanos Makatos]

> -----Original Message-----
> From: Ian Campbell
> Sent: 26 October 2012 12:37
> To: Thanos Makatos
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH 10 of 16 RFC] blktap3: Introduce the
> blktap header file
> 
> > +/*
> > + * TODO Document what each define is.
> > + */
> > +#define BLKTAP3_SYSFS_DIR                   "/sys/class/blktap3"
> 
> I thought there was no kernel side to blktap3?
> 
> > +#define BLKTAP3_CONTROL_NAME                "blktap-control"
> > +#define BLKTAP3_CONTROL_DIR
> "/var/run/"BLKTAP3_CONTROL_NAME
> > +#define BLKTAP3_CONTROL_SOCKET              "ctl"
> 
> > +#define BLKTAP3_DIRECTORY                   "/dev/xen/blktap-3"
> > +#define BLKTAP3_RING_DEVICE
> BLKTAP3_DIRECTORY"/blktap"
> 
> Likewise these two.

Correct, I removed them.

> 
> > +#define BLKTAP3_IO_DEVICEBLKTAP3_DIRECTORY  "/tapdev"
> > +#define BLKTAP3_ENOSPC_SIGNAL_FILE          "/var/run/tapdisk3-
> enospc"
> > +
> > +/*
> > + * TODO Can these be supplied from somewhere else?
> > + */
> > +#define likely(_cond)   __builtin_expect(!!(_cond), 1)
> > +#define unlikely(_cond) __builtin_expect(!!(_cond), 0)
> > +
> > +/*
> > + * FIXME taken from list.h
> > + */
> > +#define containerof(_ptr, _type, _memb) \
> > +    ((_type*)((void*)(_ptr) - offsetof(_type, _memb)))
> > +
> > +/*
> > + * FIXME shouldn't these be supplied from somewhere?
> > + */
> > +#define __printf(a, b) __attribute__((format(printf, a, b))) #define
> > +__scanf(_f, _a) __attribute__((format (scanf, _f, _a)))
> 
> These are all abstractions of gcc-isms (presumably to ease ports to
> other compilers). We generally don't bother with this sort of thing in
> the tools, although you can if you like. You might want to put into a
> compiler.h and add suitable #if like those in
> xen/include/xen/compiler.h.

I'll put them in a compiler.h

> 
> > +
> > +#define TAILQ_MOVE_HEAD(node, src, dst, entry)  \
> > +    TAILQ_REMOVE(src, node, entry);             \
> > +    TAILQ_INSERT_HEAD(dst, node, entry);
> > +
> > +#define TAILQ_MOVE_TAIL(node, src, dst, entry)  \
> > +    TAILQ_REMOVE(src, node, entry);             \
> > +    TAILQ_INSERT_TAIL(dst, node, entry);
> > +
> > +/*
> > + * TODO This is defined in xen/lib.h, use that instead of redefining
> > +it
> > + * here.
> 
> xen/lib.h is a hypervisor internal header so isn't available to you
> here. I think it is fine to define your own (although it should be in a
> private header, not part of the libblktap API)

Right I'll put them in a private header.

> 
> > + */
> > +#ifndef ARRAY_SIZE
> > +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) #endif /*
> > +ARRAY_SIZE */
> > +
> > +#endif /* __BLKTAP_3_H__ */
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 

[PATCH 11 of 16 RFC] blktap3: Provide empty implementation for the tap_ctl_list and tap_ctl_list_free functions

[Thanos Makatos]

They will be used in a later patch.

diff -r a17c7217bef2 -r 7cbf20d7e4d6 tools/blktap3/control/tap-ctl-list.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/control/tap-ctl-list.c	Wed Oct 24 17:26:26 2012 +0100
@@ -0,0 +1,44 @@
+/*
+ * FIXME this is the github blktap2 license
+ * 
+ * Copyright (c) 2008, XenSource Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in the
+ *       documentation and/or other materials provided with the distribution.
+ *     * Neither the name of XenSource Inc. nor the names of its contributors
+ *       may be used to endorse or promote products derived from this software
+ *       without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER
+ * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+ * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include "tap-ctl.h"
+
+void tap_ctl_list_free(__attribute__((__unused__)) struct tqh_tap_list *list)
+{
+    return;
+}
+
+int tap_ctl_list(__attribute__((__unused__)) struct tqh_tap_list *list)
+{
+    return -ENOSYS;
+}

Re: [PATCH 11 of 16 RFC] blktap3: Provide empty implementation for the tap_ctl_list and tap_ctl_list_free functions

[Ian Campbell]

On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif

I think the presence of config.h will be statically determined, wont it?

> +
> +#include "tap-ctl.h"
> +
> +void tap_ctl_list_free(__attribute__((__unused__)) struct tqh_tap_list *list)
> +{
> +    return;
> +}
> +
> +int tap_ctl_list(__attribute__((__unused__)) struct tqh_tap_list *list)
> +{
> +    return -ENOSYS;
> +}
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 11 of 16 RFC] blktap3: Provide empty implementation for the tap_ctl_list and tap_ctl_list_free functions

[Thanos Makatos]

> -----Original Message-----
> From: Ian Campbell
> Sent: 26 October 2012 12:37
> To: Thanos Makatos
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH 11 of 16 RFC] blktap3: Provide empty
> implementation for the tap_ctl_list and tap_ctl_list_free functions
> 
> On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> > +#ifdef HAVE_CONFIG_H
> > +#include "config.h"
> > +#endif
> 
> I think the presence of config.h will be statically determined, wont
> it?

This piece of code is present in (almost) all blktap2 (the github one) files, I seems related to autoconf/automake. I'll remove it.

> 
> > +
> > +#include "tap-ctl.h"
> > +
> > +void tap_ctl_list_free(__attribute__((__unused__)) struct
> > +tqh_tap_list *list) {
> > +    return;
> > +}
> > +
> > +int tap_ctl_list(__attribute__((__unused__)) struct tqh_tap_list
> > +*list) {
> > +    return -ENOSYS;
> > +}
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 

[PATCH 12 of 16 RFC] blktap3: Provide empty implementation for tap_ctl_destroy

[Thanos Makatos]

It will be used in a later patch.

diff -r 7cbf20d7e4d6 -r 26faf531ed07 tools/blktap3/control/tap-ctl-destroy.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/control/tap-ctl-destroy.c	Wed Oct 24 17:26:57 2012 +0100
@@ -0,0 +1,42 @@
+/*
+ * FIXME this is the github blktap2 license
+ * 
+ * Copyright (c) 2008, XenSource Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in the
+ *       documentation and/or other materials provided with the distribution.
+ *     * Neither the name of XenSource Inc. nor the names of its contributors
+ *       may be used to endorse or promote products derived from this software
+ *       without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER
+ * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+ * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include "tap-ctl.h"
+
+int tap_ctl_destroy(__attribute__((__unused__)) const int id,
+        __attribute__((__unused__)) const int minor,
+        __attribute__((__unused__)) int force,
+        __attribute__((__unused__)) struct timeval *timeout)
+{
+    return -ENOSYS;
+}

[PATCH 13 of 16 RFC] blktap3: Provide empty implementation for tap_ctl_create

[Thanos Makatos]

It will be used in a later patch.

diff -r 26faf531ed07 -r 092fce05f530 tools/blktap3/control/tap-ctl-create.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/control/tap-ctl-create.c	Wed Oct 24 17:27:10 2012 +0100
@@ -0,0 +1,43 @@
+/*
+ * FIXME this is the github blktap2 license
+ * 
+ * Copyright (c) 2008, XenSource Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in the
+ *       documentation and/or other materials provided with the distribution.
+ *     * Neither the name of XenSource Inc. nor the names of its contributors
+ *       may be used to endorse or promote products derived from this software
+ *       without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER
+ * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+ * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include "tap-ctl.h"
+
+int tap_ctl_create(__attribute__((__unused__)) const char *params,
+        __attribute__((__unused__)) int flags,
+        __attribute__((__unused__)) int parent_minor,
+        __attribute__((__unused__)) char *secondary)
+{
+    return -ENOSYS;
+}

[PATCH 14 of 16 RFC] blktap3: Provide Makefile for compiling the blktap3 control library

[Thanos Makatos]

This library will be used by libxl to create/destroy tapdisks.

diff -r 092fce05f530 -r 9f5d1f1f9791 tools/blktap3/control/Makefile
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/control/Makefile	Wed Oct 24 17:27:38 2012 +0100
@@ -0,0 +1,57 @@
+XEN_ROOT := $(CURDIR)/../../../
+include $(XEN_ROOT)/tools/Rules.mk
+
+LIBDIR ?= /usr/lib
+
+MAJOR = 1.0
+MINOR = 0
+LIBNAME = libblktapctl
+LIBSONAME = $(LIBNAME).so.$(MAJOR)
+
+override CFLAGS += \
+	-I ../include \
+	$(CFLAGS_xeninclude)
+
+CTL_OBJS = \
+		   tap-ctl-destroy.o \
+		   tap-ctl-create.o \
+		   tap-ctl-list.o
+
+CTL_PICS  = $(patsubst %.o,%.opic,$(CTL_OBJS))
+
+OBJS = $(CTL_OBJS) tap-ctl.o
+PICS = $(CTL_PICS)
+
+LIB_STATIC = $(LIBNAME).a
+LIB_SHARED = $(LIBSONAME).$(MINOR)
+
+all: build
+
+build: $(LIB_STATIC) $(LIB_SHARED)
+
+$(LIBNAME).so: $(LIBSONAME)
+	ln -sf $< $@
+
+$(LIBSONAME): $(LIB_SHARED)
+	ln -sf $< $@
+
+$(LIB_STATIC): $(CTL_OBJS)
+	$(AR) r $@ $^
+
+$(LIB_SHARED): $(CTL_PICS)
+	$(CC) $(LDFLAGS) -fPIC  -Wl,$(SONAME_LDFLAG) -Wl,$(LIBSONAME) \
+		$(SHLIB_LDFLAGS) -rdynamic $^ -o $@
+
+# TODO install in /usr/sbin?
+install: $(LIB_STATIC) $(LIB_SHARED)
+	$(INSTALL_DIR) -p $(DESTDIR)$(SBINDIR)
+	$(INSTALL_PROG) $(LIB_SHARED) $(DESTDIR)$(LIBDIR)
+	ln -sf $(LIBSONAME) $(DESTDIR)$(LIBDIR)/$(LIBNAME).so
+	ln -sf $(LIB_SHARED) $(DESTDIR)$(LIBDIR)/$(LIBSONAME)
+
+clean:
+	rm -f $(OBJS) $(PICS) $(LIB_STATIC) $(LIB_SHARED)
+	rm -f $(LIBNAME).so $(LIBSONAME)
+	rm -f *~
+
+.PHONY: all build clean install

Re: [PATCH 14 of 16 RFC] blktap3: Provide Makefile for compiling the blktap3 control library

[Ian Campbell]

On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> This library will be used by libxl to create/destroy tapdisks.
> 
> diff -r 092fce05f530 -r 9f5d1f1f9791 tools/blktap3/control/Makefile
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/blktap3/control/Makefile	Wed Oct 24 17:27:38 2012 +0100
> @@ -0,0 +1,57 @@
> +XEN_ROOT := $(CURDIR)/../../../
> +include $(XEN_ROOT)/tools/Rules.mk
> +
> +LIBDIR ?= /usr/lib

This comes from configure via config/Tools.mk now.

> +
> +MAJOR = 1.0
> +MINOR = 0
> +LIBNAME = libblktapctl
> +LIBSONAME = $(LIBNAME).so.$(MAJOR)
> +
> +override CFLAGS += \
> +	-I ../include \
> +	$(CFLAGS_xeninclude)
> +
> +CTL_OBJS = \
> +		   tap-ctl-destroy.o \
> +		   tap-ctl-create.o \
> +		   tap-ctl-list.o
> +
> +CTL_PICS  = $(patsubst %.o,%.opic,$(CTL_OBJS))
> +
> +OBJS = $(CTL_OBJS) tap-ctl.o
> +PICS = $(CTL_PICS)
> +
> +LIB_STATIC = $(LIBNAME).a
> +LIB_SHARED = $(LIBSONAME).$(MINOR)
> +
> +all: build
> +
> +build: $(LIB_STATIC) $(LIB_SHARED)
> +
> +$(LIBNAME).so: $(LIBSONAME)
> +	ln -sf $< $@
> +
> +$(LIBSONAME): $(LIB_SHARED)
> +	ln -sf $< $@
> +
> +$(LIB_STATIC): $(CTL_OBJS)
> +	$(AR) r $@ $^
> +
> +$(LIB_SHARED): $(CTL_PICS)
> +	$(CC) $(LDFLAGS) -fPIC  -Wl,$(SONAME_LDFLAG) -Wl,$(LIBSONAME) \
> +		$(SHLIB_LDFLAGS) -rdynamic $^ -o $@
> +
> +# TODO install in /usr/sbin?
> +install: $(LIB_STATIC) $(LIB_SHARED)
> +	$(INSTALL_DIR) -p $(DESTDIR)$(SBINDIR)

Was this intended to be LIBDIR? At least until the TODO is resolved.

> +	$(INSTALL_PROG) $(LIB_SHARED) $(DESTDIR)$(LIBDIR)
> +	ln -sf $(LIBSONAME) $(DESTDIR)$(LIBDIR)/$(LIBNAME).so
> +	ln -sf $(LIB_SHARED) $(DESTDIR)$(LIBDIR)/$(LIBSONAME)
> +
> +clean:
> +	rm -f $(OBJS) $(PICS) $(LIB_STATIC) $(LIB_SHARED)
> +	rm -f $(LIBNAME).so $(LIBSONAME)
> +	rm -f *~
> +
> +.PHONY: all build clean install
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 14 of 16 RFC] blktap3: Provide Makefile for compiling the blktap3 control library

[Thanos Makatos]

> -----Original Message-----
> From: Ian Campbell
> Sent: 26 October 2012 12:41
> To: Thanos Makatos
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH 14 of 16 RFC] blktap3: Provide Makefile
> for compiling the blktap3 control library
> 
> On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> > This library will be used by libxl to create/destroy tapdisks.
> >
> > diff -r 092fce05f530 -r 9f5d1f1f9791 tools/blktap3/control/Makefile
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/tools/blktap3/control/Makefile	Wed Oct 24 17:27:38 2012
> +0100
> > @@ -0,0 +1,57 @@
> > +XEN_ROOT := $(CURDIR)/../../../
> > +include $(XEN_ROOT)/tools/Rules.mk
> > +
> > +LIBDIR ?= /usr/lib
> 
> This comes from configure via config/Tools.mk now.
> 
> > +
> > +MAJOR = 1.0
> > +MINOR = 0
> > +LIBNAME = libblktapctl
> > +LIBSONAME = $(LIBNAME).so.$(MAJOR)
> > +
> > +override CFLAGS += \
> > +	-I ../include \
> > +	$(CFLAGS_xeninclude)
> > +
> > +CTL_OBJS = \
> > +		   tap-ctl-destroy.o \
> > +		   tap-ctl-create.o \
> > +		   tap-ctl-list.o
> > +
> > +CTL_PICS  = $(patsubst %.o,%.opic,$(CTL_OBJS))
> > +
> > +OBJS = $(CTL_OBJS) tap-ctl.o
> > +PICS = $(CTL_PICS)
> > +
> > +LIB_STATIC = $(LIBNAME).a
> > +LIB_SHARED = $(LIBSONAME).$(MINOR)
> > +
> > +all: build
> > +
> > +build: $(LIB_STATIC) $(LIB_SHARED)
> > +
> > +$(LIBNAME).so: $(LIBSONAME)
> > +	ln -sf $< $@
> > +
> > +$(LIBSONAME): $(LIB_SHARED)
> > +	ln -sf $< $@
> > +
> > +$(LIB_STATIC): $(CTL_OBJS)
> > +	$(AR) r $@ $^
> > +
> > +$(LIB_SHARED): $(CTL_PICS)
> > +	$(CC) $(LDFLAGS) -fPIC  -Wl,$(SONAME_LDFLAG) -Wl,$(LIBSONAME) \
> > +		$(SHLIB_LDFLAGS) -rdynamic $^ -o $@
> > +
> > +# TODO install in /usr/sbin?
> > +install: $(LIB_STATIC) $(LIB_SHARED)
> > +	$(INSTALL_DIR) -p $(DESTDIR)$(SBINDIR)
> 
> Was this intended to be LIBDIR? At least until the TODO is resolved.

I'm confused, Which one?

> 
> > +	$(INSTALL_PROG) $(LIB_SHARED) $(DESTDIR)$(LIBDIR)
> > +	ln -sf $(LIBSONAME) $(DESTDIR)$(LIBDIR)/$(LIBNAME).so
> > +	ln -sf $(LIB_SHARED) $(DESTDIR)$(LIBDIR)/$(LIBSONAME)
> > +
> > +clean:
> > +	rm -f $(OBJS) $(PICS) $(LIB_STATIC) $(LIB_SHARED)
> > +	rm -f $(LIBNAME).so $(LIBSONAME)
> > +	rm -f *~
> > +
> > +.PHONY: all build clean install
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 

Re: [PATCH 14 of 16 RFC] blktap3: Provide Makefile for compiling the blktap3 control library

[Ian Campbell]

On Tue, 2012-10-30 at 11:11 +0000, Thanos Makatos wrote:
> > > +
> > > +# TODO install in /usr/sbin?
> > > +install: $(LIB_STATIC) $(LIB_SHARED)
> > > +	$(INSTALL_DIR) -p $(DESTDIR)$(SBINDIR)
> > 
> > Was this intended to be LIBDIR? At least until the TODO is resolved.
> 
> I'm confused, Which one?

The use of SBINDIR here, was it supposed to be a LIBDIR? Because the
following lines all install to LIBDIR not SBINDIR.

> > > +	$(INSTALL_PROG) $(LIB_SHARED) $(DESTDIR)$(LIBDIR)
> > > +	ln -sf $(LIBSONAME) $(DESTDIR)$(LIBDIR)/$(LIBNAME).so
> > > +	ln -sf $(LIB_SHARED) $(DESTDIR)$(LIBDIR)/$(LIBSONAME)

[PATCH 15 of 16 RFC] blktap3: Include the blktap3/control Makefile in the blktap3 compilation

[Thanos Makatos]

diff -r 9f5d1f1f9791 -r b12c1bb767d3 tools/blktap3/Makefile
--- a/tools/blktap3/Makefile	Wed Oct 24 17:27:38 2012 +0100
+++ b/tools/blktap3/Makefile	Wed Oct 24 17:28:12 2012 +0100
@@ -1,12 +1,25 @@
-XEN_ROOT := $(CURDIR)/../../
+XEN_ROOT = $(CURDIR)/../..
 include $(XEN_ROOT)/tools/Rules.mk
 
-all: build
+override CFLAGS = \
+	-Wall \
+	-Wextra \
+	-Werror
+export CFLAGS
 
-build: ;
+SUBDIRS-y = \
+	control
 
-clean: ;
+override CPPCHECK_DIR ?= .
 
-install: ;
+tags:
+	ctags -R --language-force=C --c-kinds=+px
 
-.PHONY: all build clean install
+clean:
+	rm -rf *.a *.so *.o *.rpm $(LIB) *~ $(DEPS) TAGS tags
+
+check:
+	cppcheck --enable=all -q $(CPPCHECK_DIR)
+
+.PHONY: all clean install tags check
+all clean install: %: subdirs-%

[PATCH 16 of 16 RFC] blktap3: Implement libxl__blktap_devpath and libxl__device_destroy_tapdisk

[Thanos Makatos]

Provide implementation for the libxl__blktap_devpath and
libxl__device_destroy_tapdisk functions: the former spawns the tapdisk process,
the latter destroys it. Both of these functions use the blktap_find function,
a function that lists all running tapdisks and looks for one serving a specific
file. Finally, link libxl with the blktap3 control library.

diff -r b12c1bb767d3 -r 0f87cc018fb6 tools/libxl/Makefile
--- a/tools/libxl/Makefile	Wed Oct 24 17:28:12 2012 +0100
+++ b/tools/libxl/Makefile	Wed Oct 24 17:42:44 2012 +0100
@@ -20,7 +20,7 @@ LIBUUID_LIBS += -luuid
 endif
 
 LIBXL_LIBS =
-LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
+LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS) $(LDLIBS_libblktapctl)
 
 CFLAGS_LIBXL += $(CFLAGS_libxenctrl)
 CFLAGS_LIBXL += $(CFLAGS_libxenguest)
@@ -29,7 +29,9 @@ CFLAGS_LIBXL += $(CFLAGS_libblktapctl)
 CFLAGS_LIBXL += -Wshadow
 
 CFLAGS += $(PTHREAD_CFLAGS)
-LDFLAGS += $(PTHREAD_LDFLAGS)
+override LDFLAGS += \
+	$(PTHREAD_LDFLAGS) \
+	-L $(XEN_BLKTAP3)/control
 LIBXL_LIBS += $(PTHREAD_LIBS)
 
 LIBXLU_LIBS =
@@ -172,6 +174,7 @@ libxenlight.so.$(MAJOR): libxenlight.so.
 	ln -sf $< $@
 
 libxenlight.so.$(MAJOR).$(MINOR): $(LIBXL_OBJS)
+	make -C $(XEN_BLKTAP3)
 	$(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxenlight.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $^ $(LIBXL_LIBS) $(APPEND_LDFLAGS)
 
 libxenlight.a: $(LIBXL_OBJS)
diff -r b12c1bb767d3 -r 0f87cc018fb6 tools/libxl/libxl_blktap3.c
--- a/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:28:12 2012 +0100
+++ b/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:42:44 2012 +0100
@@ -1,11 +1,176 @@
+/*
+ * FIXME license
+ */
+
 #include "libxl_osdeps.h"
 #include "libxl_internal.h"
+#include "tap-ctl.h"
 
-int libxl__blktap_devpath(libxl__gc *gc, const char *disk,
-    libxl_disk_format format) {
-    return -ENOSYS;
+/**
+ * Retrieves the tapdisk serving the specified file.
+ *
+ * @param type
+ * @param path
+ * @param tap output parameter that receives the tapdisk information
+ *
+ * @return 0 on success, an error code otherwise
+ *
+ * TODO If neither type nor path are specified, the first tapdisk will be
+ * returned. Not sure if this behaviour is desirable...
+ */
+static int blktap_find(libxl__gc *gc, const char *type, const char *path,
+        struct tap_list *tap)
+{
+    struct tqh_tap_list list;
+    struct tap_list *entry, *next_t;
+    int ret = -ENOENT, err;
+
+    /*
+     * Get all the tapdisks.
+     */
+    TAILQ_INIT(&list);
+    err = tap_ctl_list(&list);
+    if (err) {
+        LOG(DEBUG, "can't list tapdisks: %s\n", strerror(err));
+        return err;
+    }
+
+    if (TAILQ_EMPTY(&list))
+        return ret;
+
+    tap_list_for_each_entry_safe(entry, next_t, &list) {
+
+        /*
+         * Look for the tapdisk that matches the type and path. If the user
+         * supplied a NULL type/path, it's treated as a matching one.
+         */
+
+        if (type && (!entry->type || strcmp(entry->type, type)))
+            continue;
+
+        if (path && (!entry->path || strcmp(entry->path, path)))
+            continue;
+
+        *tap = *entry;
+
+        /*
+         * Set them to NULL so that when the user calls tap_ctl_list_free it
+         * won't free them. The strings weren't copied by the assignment above!
+         */
+        tap->type = tap->path = NULL;
+
+        ret = 0;
+        break;
+    }
+
+    tap_ctl_list_free(&list);
+
+    return ret;
 }
 
-int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *be_path) {
-    return -ENOSYS;
+/**
+ * Creates a tapdisk for the specified path.
+ *
+ * TODO document parameters
+ *
+ * @param gc
+ * @param disk
+ * @param format
+ *
+ * @returns 0 on success, an error code otherwise
+ */
+int libxl__blktap_devpath(libxl__gc *gc, const char *disk,
+        libxl_disk_format format)
+{
+    const char *type = NULL;
+    char *params = NULL;
+    struct tap_list tap;
+    int err = 0;
+
+    type = libxl__device_disk_string_of_format(format);
+
+    /*
+     * Ensure that no other tapdisk is serving this path.
+     * XXX Does libxl protect us against race conditions? What if somebody
+     * manually attaches a tapdisk to this path?
+     */
+    if (!(err = blktap_find(gc, type, disk, &tap))) {
+        LOG(DEBUG, "tapdisk %d already serving %s\n", tap.pid, disk);
+        return 0;
+    }
+
+    LOG(DEBUG, "tapdisk not found\n");
+
+    /*
+     * TODO Should we worry about return codes other than ENOENT?
+     */
+
+    params = libxl__sprintf(gc, "%s:%s", type, disk);
+    if (!(err = tap_ctl_create(params, 0, -1, NULL))) {
+        LOG(DEBUG, "created tapdisk\n");        
+        return 0;
+    }
+
+    LOG(ERROR, "error creating tapdisk: %s\n", strerror(err));
+
+    return err;
 }
+
+/**
+ * Destroys the tapdisk serving the specified path.
+ *
+ * TODO document parameters
+ *
+ * @param gc
+ * @param be_path
+ *
+ * @returns 0 on success, an error code otherwise
+ */
+int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *be_path)
+{
+    char *disk;
+    int err;
+    struct tap_list tap;
+
+    LOG(DEBUG, "destroying tapdisk %s\n", be_path);
+
+    /*
+     * TODO is the path is in the following format?
+     * <backend type>:/path/to/file
+     */
+    disk = strchr(be_path, ':');
+    if (!disk) {
+        LOG(ERROR, "Unable to parse params %s", be_path);
+        return ERROR_INVAL;
+    }
+
+    /*
+     * Replace the ':' with '\0' effectively creating two strings in the same
+     * buffer: the first is the type and the second is the path.
+     */ 
+    *disk++ = '\0';
+
+    err = blktap_find(gc, be_path, disk, &tap);
+    if (err < 0) {
+        /* returns -errno */
+        LOGEV(ERROR, -err, "Unable to find type %s disk %s", be_path, disk);
+        return ERROR_FAIL;
+    }
+
+    err = tap_ctl_destroy(tap.pid, tap.minor, 0, NULL);
+    if (err < 0) {
+        LOGEV(ERROR, -err, "Failed to destroy tap device id %d minor %d",
+              tap.pid, tap.minor);
+        return ERROR_FAIL;
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */

Re: [PATCH 16 of 16 RFC] blktap3: Implement libxl__blktap_devpath and libxl__device_destroy_tapdisk

[Ian Campbell]

On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> Provide implementation for the libxl__blktap_devpath and
> libxl__device_destroy_tapdisk functions: the former spawns the tapdisk process,
> the latter destroys it. Both of these functions use the blktap_find function,
> a function that lists all running tapdisks and looks for one serving a specific
> file. Finally, link libxl with the blktap3 control library.
> 
> diff -r b12c1bb767d3 -r 0f87cc018fb6 tools/libxl/Makefile
> --- a/tools/libxl/Makefile	Wed Oct 24 17:28:12 2012 +0100
> +++ b/tools/libxl/Makefile	Wed Oct 24 17:42:44 2012 +0100
> @@ -20,7 +20,7 @@ LIBUUID_LIBS += -luuid
>  endif
>  
>  LIBXL_LIBS =
> -LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
> +LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS) $(LDLIBS_libblktapctl)

Is this adding back the thing I commented on earlier?

>  CFLAGS_LIBXL += $(CFLAGS_libxenctrl)
>  CFLAGS_LIBXL += $(CFLAGS_libxenguest)
> @@ -29,7 +29,9 @@ CFLAGS_LIBXL += $(CFLAGS_libblktapctl)
>  CFLAGS_LIBXL += -Wshadow
>  
>  CFLAGS += $(PTHREAD_CFLAGS)
> -LDFLAGS += $(PTHREAD_LDFLAGS)
> +override LDFLAGS += \
> +	$(PTHREAD_LDFLAGS) \
> +	-L $(XEN_BLKTAP3)/control

Please (defined and) use LDLIBS_libblktapctl. Also this change to use
override looks very dubious to me -- what does it do?

>  LIBXL_LIBS += $(PTHREAD_LIBS)
>  
>  LIBXLU_LIBS =
> @@ -172,6 +174,7 @@ libxenlight.so.$(MAJOR): libxenlight.so.
>  	ln -sf $< $@
>  
>  libxenlight.so.$(MAJOR).$(MINOR): $(LIBXL_OBJS)
> +	make -C $(XEN_BLKTAP3)

This shouldn't be needed if the tools/Makefile has things ordered
correctly. It is reasonable for tools/libxl to assume that its
prerequisites are already built. People who build with make -C
tools/libxl need to take care of this themself.

>  	$(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxenlight.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $^ $(LIBXL_LIBS) $(APPEND_LDFLAGS)
>  
>  libxenlight.a: $(LIBXL_OBJS)
> diff -r b12c1bb767d3 -r 0f87cc018fb6 tools/libxl/libxl_blktap3.c
> --- a/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:28:12 2012 +0100
> +++ b/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:42:44 2012 +0100
>  
> -int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *be_path) {
> -    return -ENOSYS;
> +/**
> + * Creates a tapdisk for the specified path.
> + *
> + * TODO document parameters
> + *
> + * @param gc
> + * @param disk
> + * @param format
> + *
> + * @returns 0 on success, an error code otherwise
> + */
> +int libxl__blktap_devpath(libxl__gc *gc, const char *disk,
> +        libxl_disk_format format)
> +{
> +    const char *type = NULL;
> +    char *params = NULL;
> +    struct tap_list tap;
> +    int err = 0;
> +
> +    type = libxl__device_disk_string_of_format(format);
> +
> +    /*
> +     * Ensure that no other tapdisk is serving this path.
> +     * XXX Does libxl protect us against race conditions?

Not AFAIK. Does tapdisk not open the file O_EXCL when necessary?

>  What if somebody
> +     * manually attaches a tapdisk to this path?
> +     */
> +    if (!(err = blktap_find(gc, type, disk, &tap))) {
> +        LOG(DEBUG, "tapdisk %d already serving %s\n", tap.pid, disk);
> +        return 0;
> +    }
> +
> +    LOG(DEBUG, "tapdisk not found\n");
> +
> +    /*
> +     * TODO Should we worry about return codes other than ENOENT?
> +     */
> +
> +    params = libxl__sprintf(gc, "%s:%s", type, disk);
> +    if (!(err = tap_ctl_create(params, 0, -1, NULL))) {
> +        LOG(DEBUG, "created tapdisk\n");        
> +        return 0;
> +    }
> +
> +    LOG(ERROR, "error creating tapdisk: %s\n", strerror(err));

You can use LOGE or LOGEV here to get the errno type thing printed
automatically in a consistent way.

Ian.

Re: [PATCH 16 of 16 RFC] blktap3: Implement libxl__blktap_devpath and libxl__device_destroy_tapdisk

[Thanos Makatos]

> -----Original Message-----
> From: Ian Campbell
> Sent: 26 October 2012 12:46
> To: Thanos Makatos
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH 16 of 16 RFC] blktap3: Implement
> libxl__blktap_devpath and libxl__device_destroy_tapdisk
> 
> On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> > Provide implementation for the libxl__blktap_devpath and
> > libxl__device_destroy_tapdisk functions: the former spawns the
> tapdisk
> > process, the latter destroys it. Both of these functions use the
> > blktap_find function, a function that lists all running tapdisks and
> > looks for one serving a specific file. Finally, link libxl with the
> blktap3 control library.
> >
> > diff -r b12c1bb767d3 -r 0f87cc018fb6 tools/libxl/Makefile
> > --- a/tools/libxl/Makefile	Wed Oct 24 17:28:12 2012 +0100
> > +++ b/tools/libxl/Makefile	Wed Oct 24 17:42:44 2012 +0100
> > @@ -20,7 +20,7 @@ LIBUUID_LIBS += -luuid  endif
> >
> >  LIBXL_LIBS =
> > -LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest)
> > $(LDLIBS_libxenstore) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
> > +LIBXL_LIBS = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest)
> > +$(LDLIBS_libxenstore) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
> > +$(LDLIBS_libblktapctl)
> 
> Is this adding back the thing I commented on earlier?

Yes.

> 
> >  CFLAGS_LIBXL += $(CFLAGS_libxenctrl)
> >  CFLAGS_LIBXL += $(CFLAGS_libxenguest) @@ -29,7 +29,9 @@ CFLAGS_LIBXL
> > += $(CFLAGS_libblktapctl)  CFLAGS_LIBXL += -Wshadow
> >
> >  CFLAGS += $(PTHREAD_CFLAGS)
> > -LDFLAGS += $(PTHREAD_LDFLAGS)
> > +override LDFLAGS += \
> > +	$(PTHREAD_LDFLAGS) \
> > +	-L $(XEN_BLKTAP3)/control
> 
> Please (defined and) use LDLIBS_libblktapctl. Also this change to use
> override looks very dubious to me -- what does it do?
> 
> >  LIBXL_LIBS += $(PTHREAD_LIBS)
> >
> >  LIBXLU_LIBS =
> > @@ -172,6 +174,7 @@ libxenlight.so.$(MAJOR): libxenlight.so.
> >  	ln -sf $< $@
> >
> >  libxenlight.so.$(MAJOR).$(MINOR): $(LIBXL_OBJS)
> > +	make -C $(XEN_BLKTAP3)
> 
> This shouldn't be needed if the tools/Makefile has things ordered
> correctly. It is reasonable for tools/libxl to assume that its
> prerequisites are already built. People who build with make -C
> tools/libxl need to take care of this themself.

I've added this so I can build libxl from within tools/libxl without worrying about building tools/blktap3, it simplifies development, doesn't it?

> 
> >  	$(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxenlight.so.$(MAJOR)
> > $(SHLIB_LDFLAGS) -o $@ $^ $(LIBXL_LIBS) $(APPEND_LDFLAGS)
> >
> >  libxenlight.a: $(LIBXL_OBJS)
> > diff -r b12c1bb767d3 -r 0f87cc018fb6 tools/libxl/libxl_blktap3.c
> > --- a/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:28:12 2012 +0100
> > +++ b/tools/libxl/libxl_blktap3.c	Wed Oct 24 17:42:44 2012 +0100
> >
> > -int libxl__device_destroy_tapdisk(libxl__gc *gc, const char
> *be_path) {
> > -    return -ENOSYS;
> > +/**
> > + * Creates a tapdisk for the specified path.
> > + *
> > + * TODO document parameters
> > + *
> > + * @param gc
> > + * @param disk
> > + * @param format
> > + *
> > + * @returns 0 on success, an error code otherwise  */ int
> > +libxl__blktap_devpath(libxl__gc *gc, const char *disk,
> > +        libxl_disk_format format)
> > +{
> > +    const char *type = NULL;
> > +    char *params = NULL;
> > +    struct tap_list tap;
> > +    int err = 0;
> > +
> > +    type = libxl__device_disk_string_of_format(format);
> > +
> > +    /*
> > +     * Ensure that no other tapdisk is serving this path.
> > +     * XXX Does libxl protect us against race conditions?
> 
> Not AFAIK. Does tapdisk not open the file O_EXCL when necessary?

I don't think so, I'll have a look. This could be a problem if two guest VMs are created at the same time and they both use the same VDI, obviously a corner case. Should we protect against that or we're getting paranoid?

> 
> >  What if somebody
> > +     * manually attaches a tapdisk to this path?
> > +     */
> > +    if (!(err = blktap_find(gc, type, disk, &tap))) {
> > +        LOG(DEBUG, "tapdisk %d already serving %s\n", tap.pid,
> disk);
> > +        return 0;
> > +    }
> > +
> > +    LOG(DEBUG, "tapdisk not found\n");
> > +
> > +    /*
> > +     * TODO Should we worry about return codes other than ENOENT?
> > +     */
> > +
> > +    params = libxl__sprintf(gc, "%s:%s", type, disk);
> > +    if (!(err = tap_ctl_create(params, 0, -1, NULL))) {
> > +        LOG(DEBUG, "created tapdisk\n");
> > +        return 0;
> > +    }
> > +
> > +    LOG(ERROR, "error creating tapdisk: %s\n", strerror(err));
> 
> You can use LOGE or LOGEV here to get the errno type thing printed
> automatically in a consistent way.

Ok.

> 
> Ian.

Re: [PATCH 16 of 16 RFC] blktap3: Implement libxl__blktap_devpath and libxl__device_destroy_tapdisk

[Ian Campbell]

> > >  LIBXL_LIBS += $(PTHREAD_LIBS)
> > >
> > >  LIBXLU_LIBS =
> > > @@ -172,6 +174,7 @@ libxenlight.so.$(MAJOR): libxenlight.so.
> > >  	ln -sf $< $@
> > >
> > >  libxenlight.so.$(MAJOR).$(MINOR): $(LIBXL_OBJS)
> > > +	make -C $(XEN_BLKTAP3)
> > 
> > This shouldn't be needed if the tools/Makefile has things ordered
> > correctly. It is reasonable for tools/libxl to assume that its
> > prerequisites are already built. People who build with make -C
> > tools/libxl need to take care of this themself.
> 
> I've added this so I can build libxl from within tools/libxl without
> worrying about building tools/blktap3, it simplifies development,
> doesn't it?

If we did this for everything our Makefiles would be an unholy mess.
Either just run "make -C tools/blktap3 && make -C tools/libxl" or carry
this patch locally.

> > > +libxl__blktap_devpath(libxl__gc *gc, const char *disk,
> > > +        libxl_disk_format format)
> > > +{
> > > +    const char *type = NULL;
> > > +    char *params = NULL;
> > > +    struct tap_list tap;
> > > +    int err = 0;
> > > +
> > > +    type = libxl__device_disk_string_of_format(format);
> > > +
> > > +    /*
> > > +     * Ensure that no other tapdisk is serving this path.
> > > +     * XXX Does libxl protect us against race conditions?
> > 
> > Not AFAIK. Does tapdisk not open the file O_EXCL when necessary?
> 
> I don't think so, I'll have a look. This could be a problem if two
> guest VMs are created at the same time and they both use the same VDI,
> obviously a corner case. Should we protect against that or we're
> getting paranoid?

TBH I'm not sure what the semantics of sharing these things are supposed
to be.

Ian

Re: [PATCH 16 of 16 RFC] blktap3: Implement libxl__blktap_devpath and libxl__device_destroy_tapdisk

[Thanos Makatos]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: 30 October 2012 11:26
> To: Thanos Makatos
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH 16 of 16 RFC] blktap3: Implement
> libxl__blktap_devpath and libxl__device_destroy_tapdisk
> 
> > > >  LIBXL_LIBS += $(PTHREAD_LIBS)
> > > >
> > > >  LIBXLU_LIBS =
> > > > @@ -172,6 +174,7 @@ libxenlight.so.$(MAJOR): libxenlight.so.
> > > >  	ln -sf $< $@
> > > >
> > > >  libxenlight.so.$(MAJOR).$(MINOR): $(LIBXL_OBJS)
> > > > +	make -C $(XEN_BLKTAP3)
> > >
> > > This shouldn't be needed if the tools/Makefile has things ordered
> > > correctly. It is reasonable for tools/libxl to assume that its
> > > prerequisites are already built. People who build with make -C
> > > tools/libxl need to take care of this themself.
> >
> > I've added this so I can build libxl from within tools/libxl without
> > worrying about building tools/blktap3, it simplifies development,
> > doesn't it?
> 
> If we did this for everything our Makefiles would be an unholy mess.
> Either just run "make -C tools/blktap3 && make -C tools/libxl" or carry
> this patch locally.

Ok.

> 
> > > > +libxl__blktap_devpath(libxl__gc *gc, const char *disk,
> > > > +        libxl_disk_format format) {
> > > > +    const char *type = NULL;
> > > > +    char *params = NULL;
> > > > +    struct tap_list tap;
> > > > +    int err = 0;
> > > > +
> > > > +    type = libxl__device_disk_string_of_format(format);
> > > > +
> > > > +    /*
> > > > +     * Ensure that no other tapdisk is serving this path.
> > > > +     * XXX Does libxl protect us against race conditions?
> > >
> > > Not AFAIK. Does tapdisk not open the file O_EXCL when necessary?
> >
> > I don't think so, I'll have a look. This could be a problem if two
> > guest VMs are created at the same time and they both use the same
> VDI,
> > obviously a corner case. Should we protect against that or we're
> > getting paranoid?
> 
> TBH I'm not sure what the semantics of sharing these things are
> supposed to be.
> 
> Ian

Re: [PATCH 00 of 16 RFC] add libxl support for blktap3 and introduce the blktap3 build system

[Ian Campbell]

On Wed, 2012-10-24 at 18:02 +0100, Thanos Makatos wrote:
> This series introduces support for blktap3 in libxl and adds the basis of the
> blktap3 build system. There is no essential blktap3 functionality yet: all
> blktap3 functions are stubs and they will be implemented in later patches. This
> means that trying to use tap as the disk backend for a guest will make libxl
> complain since all of blktap3 functions return a failure error code. Most
> header files introduced in this series require some basic documentation.
> Finally, licenses need to be verified.

As it stands this series will break the build and/or xl functionality at
various points. I recommend ordering them as:
      * Stage 1: populate tools/blktap3, make it build with make -C
        tools/blktap3 (build breakage within tools/blktap3 doesn't
        matter during this stage)
      * Stage 2: connect tools/blktap3 to the top-level build system,
        but don't use it anywhere yet. At this point tools/blktap3 must
        compile successfully.
      * Third tranche: Patch libxl/xl etc to make them use blktap3
        instead of blktap2.

I'm also not convinced this pattern of adding dummy functions up front
and then switching them in with real implementations later is necessary
here. Sometimes that can be a useful technique for avoiding bisection
breakage but it doesn't seem like it should be needed if you follow
something like the staging proposal above.

I'm going to take a pass through the patches but I won't comment on
anything arising from either of the above.

Ian.

Re: [PATCH 00 of 16 RFC] add libxl support for blktap3 and introduce the blktap3 build system

[Thanos Makatos]

> -----Original Message-----
> From: Ian Campbell
> Sent: 26 October 2012 12:15
> To: Thanos Makatos
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH 00 of 16 RFC] add libxl support for
> blktap3 and introduce the blktap3 build system
> 
> As it stands this series will break the build and/or xl functionality
> at various points. I recommend ordering them as:
>       * Stage 1: populate tools/blktap3, make it build with make -C
>         tools/blktap3 (build breakage within tools/blktap3 doesn't
>         matter during this stage)
>       * Stage 2: connect tools/blktap3 to the top-level build system,
>         but don't use it anywhere yet. At this point tools/blktap3 must
>         compile successfully.
>       * Third tranche: Patch libxl/xl etc to make them use blktap3
>         instead of blktap2.

OK this sounds plausible.