Skip to contentSkip to conversation

add libxl support for blktap3 and introduce the blktap3 build system

RFC PATCH In review Thanos Makatos 4 participants 50 messages Oct 24, 2012 – Oct 30, 2012

lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 add libxl support for blktap3 and introduce the blktap3 build system

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.

#1 lore raw

RFC PATCH 1/1 blktap3: Introduce the blktap3 device type

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

tools/libxl/libxl.c +1 -1
diff -r 3cd5bf62ded7 -r fef81de66bf7 tools/libxl/libxl.c
@@ -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;
tools/libxl/libxl_types_internal.idl +1 -0
diff -r 3cd5bf62ded7 -r fef81de66bf7 tools/libxl/libxl_types_internal.idl
@@ -19,6 +19,7 @@ libxl__device_kind = Enumeration("device
     (5, "VFB"),
     (6, "VKBD"),
     (7, "CONSOLE"),
+    (8, "XENIO"),
     ])
 
 libxl__console_backend = Enumeration("console_backend", [
#2 lore raw

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

tools/libxl/libxl.c int libxl__device_from_disk(libxl__gc *g · line 1745

Quoted 11 lines · 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

tools/libxl/libxl.c +1 -1
@@ -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.

tools/libxl/libxl_types_internal.idl libxl__device_kind = Enumeration("device · line 22

Quoted 11 lines
tools/libxl/libxl.c +0 -0
             break;
         case LIBXL_DISK_BACKEND_QDISK:
             device->backend_kind = LIBXL__DEVICE_KIND_QDISK;

diff -r 3cd5bf62ded7 -r fef81de66bf7 tools/libxl/libxl_types_internal.idl

tools/libxl/libxl_types_internal.idl +1 -0
@@ -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?

Quoted 8 lines
tools/libxl/libxl_types_internal.idl +0 -0
     ])
 
 libxl__console_backend = Enumeration("console_backend", [

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

#3 lore raw

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

Quoted 12 lines

-----Original Message-----
From: Ian Campbell
Sent: 26 October 2012 12:17
To: Thanos Makatos
Cc: [email protected]
Subject: Re: [Xen-devel] [PATCH 01 of 16 RFC] blktap3: Introduce the blktap3 device type

Quoted 1 line · Thanos Makatos

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

of vbd.

Quoted 1 line
Quoted 14 lines

tools/libxl/libxl.c int libxl__device_from_disk(libxl__gc *g · line 1745

Quoted 9 lines

diff -r 3cd5bf62ded7 -r fef81de66bf7 tools/libxl/libxl.c

tools/libxl/libxl.c +1 -1
@@ -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.

Quoted 9 lines
Quoted 6 lines
tools/libxl/libxl.c +0 -0
             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

Quoted 1 line

+++ b/tools/libxl/libxl_types_internal.idl Wed Oct 24 17:24:01

2012 +0100

Quoted 8 lines

tools/libxl/libxl.c libxl__device_kind = Enumeration("device · line 22

Quoted 5 lines
tools/libxl/libxl.c +1 -0
@@ -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.

Quoted 10 lines
Quoted 9 lines
tools/libxl/libxl.c +0 -0
     ])
 
 libxl__console_backend = Enumeration("console_backend", [

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

#4 lore raw

RFC PATCH 1/1 blktap3: Introduce blktap3 support for libxl

In reply to Thanos Makatos

These functions will be implemented at a later patch.

tools/libxl/libxl_blktap3.c new file +15 -0
diff -r fef81de66bf7 -r 513739de43b5 tools/libxl/libxl_blktap3.c
@@ -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;
+}
#5 lore raw

RFC PATCH 1/1 blktap3: support for blktap3 in libxl Makefile

In reply to 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.

tools/Rules.mk +4 -3
diff -r 513739de43b5 -r 80e0bc67dcda tools/Rules.mk
@@ -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 =
tools/libxl/Makefile +5 -4
diff -r 513739de43b5 -r 80e0bc67dcda tools/libxl/Makefile
@@ -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
#6 lore raw

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

tools/libxl/Makefile LIBUUID_LIBS += -luuid · line 23

Quoted 9 lines

diff -r 513739de43b5 -r 80e0bc67dcda tools/libxl/Makefile

tools/libxl/Makefile +1 -1
@@ -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?

tools/libxl/Makefile LIBXL_OBJS = flexarray.o libxl.o libxl_c · line 81

Quoted 9 lines
tools/libxl/Makefile +2 -1
 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.

#7 lore raw

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

Quoted 15 lines

-----Original Message-----
From: Ian Campbell
Sent: 26 October 2012 12:19
To: Thanos Makatos
Cc: [email protected]
Subject: Re: [Xen-devel] [PATCH 03 of 16 RFC] blktap3: support for blktap3 in libxl Makefile

Quoted 5 lines

diff -r 513739de43b5 -r 80e0bc67dcda tools/libxl/Makefile

tools/libxl/Makefile +0 -0
@@ -20,7 +20,7 @@ LIBUUID_LIBS += -luuid  endif
 
Quoted 8 lines

tools/libxl/Makefile LIBUUID_LIBS += -luuid endif · line 25

Quoted 6 lines
tools/libxl/Makefile +2 -1
 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.

Quoted 11 lines
Quoted 5 lines
tools/libxl/Makefile +0 -0
 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

Quoted 5 lines

+= _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

Quoted 6 lines

$(XEN_ROOT)/tools/config.h \

tools/libxl/Makefile LIBXL_OBJS = flexarray.o libxl.o libxl_c · line 81

Quoted 1 line
tools/libxl/Makefile +1 -0
+	$(CFLAGS_libblktapctl)

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

Correct, I missed it.

Quoted 2 lines

Ian.

#8 lore raw

RFC PATCH 1/1 blktap3: change libxl__blktap_devpath prototype to return an error code

In reply to 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.

tools/libxl/libxl.c +12 -6
diff -r 80e0bc67dcda -r bcb5a6182868 tools/libxl/libxl.c
@@ -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:
tools/libxl/libxl_blktap3.c +2 -2
diff -r 80e0bc67dcda -r bcb5a6182868 tools/libxl/libxl_blktap3.c
@@ -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) {
tools/libxl/libxl_internal.h +2 -3
diff -r 80e0bc67dcda -r bcb5a6182868 tools/libxl/libxl_internal.h
@@ -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);
#9 lore raw

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

Quoted 2 lines · Thanos Makatos

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?

Quoted 76 lines

Amend the libxl code that uses this functions accordingly.

diff -r 80e0bc67dcda -r bcb5a6182868 tools/libxl/libxl.c

tools/libxl/libxl.c +12 -6
@@ -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

tools/libxl/libxl_blktap3.c +2 -2
@@ -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

tools/libxl/libxl_internal.h +2 -3
@@ -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
[email protected]
http://lists.xen.org/xen-devel

#10 lore raw

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

Quoted 8 lines

-----Original Message-----
From: Ian Campbell
Sent: 26 October 2012 12:22
To: Thanos Makatos
Cc: [email protected]
Subject: Re: [Xen-devel] [PATCH 04 of 16 RFC] blktap3: change libxl__blktap_devpath prototype to return an error code

Quoted 8 lines
Quoted 1 line · Thanos Makatos

Make libxl__blktap_devpath return an error code instead of the

device,

Quoted 1 line

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.

Quoted 88 lines

tools/libxl/libxl.c static void device_disk_add(libxl__egc * · line 1847

Quoted 11 lines

Amend the libxl code that uses this functions accordingly.

diff -r 80e0bc67dcda -r bcb5a6182868 tools/libxl/libxl.c

tools/libxl/libxl.c +0 -1
@@ -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);

tools/libxl/libxl.c static void device_disk_add(libxl__egc * · line 1849

Quoted 2 lines
tools/libxl/libxl.c +0 -2
-                if (!dev) {
-                    LOG(ERROR, "failed to get blktap devpath for

%p\n",

tools/libxl/libxl.c static void device_disk_add(libxl__egc * · line 1847

Quoted 2 lines
tools/libxl/libxl.c +1 -1
-                        disk->pdev_path);
+                rc = libxl__blktap_devpath(gc, disk->pdev_path,

disk->format);

tools/libxl/libxl.c static void device_disk_add(libxl__egc * · line 1849

Quoted 2 lines
tools/libxl/libxl.c +2 -0
+                if (rc) {
+                    LOG(ERROR, "failed to get blktap devpath for %s:

%s\n",

Quoted 10 lines
tools/libxl/libxl.c +2 -0
+                        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),

tools/libxl/libxl.c void libxl__device_disk_local_initiate_a · line 2281

Quoted 6 lines
tools/libxl/libxl.c +1 -2
@@ -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);

tools/libxl/libxl.c void libxl__device_disk_local_initiate_a

Quoted 3 lines
tools/libxl/libxl.c +3 -0
+                if (!rc) {
+                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                           "error getting tapdisk: %s",

strerror(rc));

Quoted 11 lines
tools/libxl/libxl.c +3 -0
+                    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

tools/libxl/libxl_blktap3.c +0 -0
@@ -5,9 +5,9 @@ int libxl__blktap_enabled(libxl__gc *gc)
     return 1;

tools/libxl/libxl_blktap3.c int libxl__blktap_enabled(libxl__gc *gc) · line 13

Quoted 10 lines
tools/libxl/libxl_blktap3.c +2 -2
 }
 
-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)

Quoted 2 lines

{ diff -r 80e0bc67dcda -r bcb5a6182868 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h Wed Oct 24 17:24:53 2012

+0100

Quoted 1 line

+++ b/tools/libxl/libxl_internal.h Wed Oct 24 17:25:02 2012

+0100

Quoted 18 lines
tools/libxl/libxl_blktap3.c +2 -3
@@ -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
[email protected]
http://lists.xen.org/xen-devel

#11 lore raw

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

Quoted 13 lines · Thanos Makatos
Quoted 13 lines

-----Original Message-----
From: Ian Campbell
Sent: 26 October 2012 12:22
To: Thanos Makatos
Cc: [email protected]
Subject: Re: [Xen-devel] [PATCH 04 of 16 RFC] blktap3: change libxl__blktap_devpath prototype to return an error code

Quoted 1 line · Thanos Makatos

Make libxl__blktap_devpath return an error code instead of the

device,

Quoted 1 line

since there is no device in dom0 any more.

Quoted 7 lines
Quoted 3 lines

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.

#12 lore raw

RFC PATCH 1/1 blktap3: Don't check if blktap is available

In reply to Thanos Makatos

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

tools/libxl/libxl_blktap3.c +0 -4
diff -r bcb5a6182868 -r 57896068356d tools/libxl/libxl_blktap3.c
@@ -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;
tools/libxl/libxl_device.c +0 -7
diff -r bcb5a6182868 -r 57896068356d tools/libxl/libxl_device.c
@@ -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;
#13 lore raw

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

Quoted 2 lines · Thanos Makatos

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?

Quoted 37 lines

diff -r bcb5a6182868 -r 57896068356d tools/libxl/libxl_blktap3.c

tools/libxl/libxl_blktap3.c +0 -4
@@ -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

tools/libxl/libxl_device.c +0 -7
@@ -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
[email protected]
http://lists.xen.org/xen-devel

#14 lore raw

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

Quoted 9 lines

-----Original Message-----
From: Ian Campbell
Sent: 26 October 2012 12:24
To: Thanos Makatos
Cc: [email protected]
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:

Quoted 8 lines
Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 40 lines
Quoted 20 lines

diff -r bcb5a6182868 -r 57896068356d tools/libxl/libxl_blktap3.c

tools/libxl/libxl_blktap3.c +0 -4
@@ -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

tools/libxl/libxl_device.c static int disk_try_backend(disk_try_bac · line 182

Quoted 6 lines
tools/libxl/libxl_device.c +0 -3
@@ -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"

Quoted 12 lines
tools/libxl/libxl_device.c +0 -4
-                       " 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
[email protected]
http://lists.xen.org/xen-devel

#15 lore raw

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

Quoted 6 lines
Quoted 3 lines

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.

#16 lore raw

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

Quoted 9 lines · Ian Campbell
Quoted 6 lines
Quoted 3 lines

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.

#17 lore raw

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

In reply to Ian Campbell
Quoted 14 lines

-----Original Message-----
From: Ian Campbell
Sent: 30 October 2012 07:42
To: Thanos Makatos
Cc: [email protected]
Subject: Re: [Xen-devel] [PATCH 05 of 16 RFC] blktap3: Don't check if blktap is available

Quoted 4 lines
Quoted 3 lines

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

Quoted 6 lines
Quoted 1 line

We could ensure that the tapdisk binaries are present, apart from

that

Quoted 1 line

I don't see anything else we could check.

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

Sure.

Quoted 2 lines

Ian.

#18 lore raw

RFC PATCH 1/1 blktap3: Remove physical device creation for libxl

In reply to Thanos Makatos

blktap3 doesn't need a device in dom0.

tools/libxl/libxl.c +1 -8
diff -r 57896068356d -r 5a0b0e799ae8 tools/libxl/libxl.c
@@ -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",
#19 lore raw

RFC PATCH 1/1 blktap3: Introduce blktap3 top-level Makefile

In reply to Thanos Makatos
tools/blktap3/Makefile new file +12 -0
diff -r 5a0b0e799ae8 -r 17eea5a9a20c tools/blktap3/Makefile
@@ -0,0 +1,12 @@
+XEN_ROOT := $(CURDIR)/../../
+include $(XEN_ROOT)/tools/Rules.mk
+
+all: build
+
+build: ;
+
+clean: ;
+
+install: ;
+
+.PHONY: all build clean install
#20 lore raw

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

tools/blktap3/Makefile line 6

Quoted 10 lines · Thanos Makatos

diff -r 5a0b0e799ae8 -r 17eea5a9a20c tools/blktap3/Makefile

tools/blktap3/Makefile new file +6 -0
@@ -0,0 +1,12 @@
+XEN_ROOT := $(CURDIR)/../../
+include $(XEN_ROOT)/tools/Rules.mk
+
+all: build
+
+build: ;

What does the ";" in this syntax do?

Quoted 11 lines
tools/blktap3/Makefile new file +6 -0
+
+clean: ;
+
+install: ;
+
+.PHONY: all build clean install

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

#21 lore raw

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

Hello Ian,

Quoted 3 lines · On Friday 26 October 2012 13:25:01 Ian Campbell wrote:
Quoted 1 line

+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

Signature · 4 lines
Philipp Hahn           Open Source Software Engineer      [email protected]
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/

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

#22 lore raw

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

Quoted 9 lines · Philipp Hahn

Hello Ian,

Quoted 3 lines · On Friday 26 October 2012 13:25:01 Ian Campbell wrote:
Quoted 1 line

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

Quoted 2 lines

Sincerely
Philipp

#23 lore raw

RFC PATCH 1/1 blktap3: Introduce message types and structures

In reply to 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.

tools/blktap3/include/tapdisk-message.h new file +267 -0
diff -r 17eea5a9a20c -r 23e2537c7257 tools/blktap3/include/tapdisk-message.h
@@ -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__ */
#24 lore raw

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

Quoted 1 line · Thanos Makatos

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?

Quoted 4 lines

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?

tools/blktap3/include/tapdisk-message.h line 174

Quoted 6 lines
tools/blktap3/include/tapdisk-message.h new file +6 -0
+/**
+ * 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.

#25 lore raw

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

Quoted 8 lines

-----Original Message-----
From: Ian Campbell
Sent: 26 October 2012 12:28
To: Thanos Makatos
Cc: [email protected]
Subject: Re: [Xen-devel] [PATCH 08 of 16 RFC] blktap3: Introduce message types and structures

Quoted 5 lines
Quoted 1 line · Thanos Makatos

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.

Quoted 10 lines
Quoted 5 lines

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.

Quoted 10 lines
Quoted 3 lines

+/**
+ * Retrieves the human-readable representation of the type of the

message.

tools/blktap3/include/tapdisk-message.h line 174

Quoted 4 lines

+ */

tools/blktap3/include/tapdisk-message.h new file +3 -0
+static inline char *tapdisk_message_name(enum tapdisk_message_id id)
+{
+    switch (id) {

Would a static lookup table be nicer?

Definitely.

Quoted 2 lines

Ian.

#26 lore raw

RFC PATCH 1/1 blktap3: Introduce the tapdisk control function prototypes

In reply to 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.

tools/blktap3/include/tap-ctl.h new file +184 -0
diff -r 23e2537c7257 -r efb6b2844256 tools/blktap3/include/tap-ctl.h
@@ -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__ */
#27 lore raw

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

tools/blktap3/include/tap-ctl.h line 73

Quoted 4 lines · Thanos Makatos
tools/blktap3/include/tap-ctl.h new file +4 -0
+    /**
+     * 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.

#28 lore raw

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

Quoted 12 lines

-----Original Message-----
From: Ian Campbell
Sent: 26 October 2012 12:33
To: Thanos Makatos
Cc: [email protected]
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:

Quoted 7 lines

tools/blktap3/include/tap-ctl.h line 73

Quoted 4 lines
tools/blktap3/include/tap-ctl.h new file +4 -0
+    /**
+     * 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.

Quoted 7 lines

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.

#29 lore raw

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

Quoted 8 lines
Quoted 6 lines

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.

#30 lore raw

RFC PATCH 1/1 blktap3: Introduce the blktap header file

In reply to 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.

tools/blktap3/include/blktap.h new file +85 -0
diff -r efb6b2844256 -r a17c7217bef2 tools/blktap3/include/blktap.h
@@ -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__ */
#31 lore raw

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

Quoted 40 lines · 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

tools/blktap3/include/blktap.h new file +32 -0
@@ -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__

tools/blktap3/include/blktap.h line 42

Quoted 10 lines
tools/blktap3/include/blktap.h new file +10 -0
+
+#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?

tools/blktap3/include/blktap.h line 47

Quoted 6 lines
tools/blktap3/include/blktap.h new file +5 -0
+#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.

Quoted 10 lines
tools/blktap3/include/blktap.h new file +10 -0
+#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)
+
+/*

tools/blktap3/include/blktap.h line 67

Quoted 10 lines
tools/blktap3/include/blktap.h new file +10 -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.

tools/blktap3/include/blktap.h line 79

Quoted 12 lines
tools/blktap3/include/blktap.h new file +12 -0
+
+#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)

Quoted 11 lines

+ */

tools/blktap3/include/blktap.h new file +5 -0
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#endif /* ARRAY_SIZE */
+
+#endif /* __BLKTAP_3_H__ */

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

#32 lore raw

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

Quoted 18 lines

-----Original Message-----
From: Ian Campbell
Sent: 26 October 2012 12:37
To: Thanos Makatos
Cc: [email protected]
Subject: Re: [Xen-devel] [PATCH 10 of 16 RFC] blktap3: Introduce the blktap header file

tools/blktap3/include/blktap.h line 42

Quoted 4 lines
tools/blktap3/include/blktap.h new file +4 -0
+/*
+ * TODO Document what each define is.
+ */
+#define BLKTAP3_SYSFS_DIR                   "/sys/class/blktap3"

I thought there was no kernel side to blktap3?

Review

Quoted 2 lines
(quoted diff) +2 -0
+#define BLKTAP3_CONTROL_NAME                "blktap-control"
+#define BLKTAP3_CONTROL_DIR

"/var/run/"BLKTAP3_CONTROL_NAME

Quoted 7 lines

tools/blktap3/include/blktap.h

Quoted 4 lines
tools/blktap3/include/blktap.h new file +3 -0
+#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.

Quoted 20 lines

Review

Quoted 3 lines
(quoted diff) +2 -0
+#define BLKTAP3_IO_DEVICEBLKTAP3_DIRECTORY  "/tapdev"
+#define BLKTAP3_ENOSPC_SIGNAL_FILE          "/var/run/tapdisk3-

enospc"

Quoted 16 lines
tools/blktap3/include/blktap.h new file +16 -0
+
+/*
+ * 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?
+ */
Quoted 8 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+#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

Quoted 10 lines
Quoted 10 lines
tools/blktap3/include/blktap.h new file +9 -0
+
+#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);
+
Quoted 8 lines

tools/blktap3/include/blktap.h line 79

Quoted 4 lines
tools/blktap3/include/blktap.h new file +4 -0
+/*
+ * 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.

Quoted 13 lines
Quoted 12 lines

+ */

tools/blktap3/include/blktap.h new file +5 -0
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) #endif /*
+ARRAY_SIZE */
+
+#endif /* __BLKTAP_3_H__ */

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

#33 lore raw

RFC PATCH 1/1 blktap3: Provide empty implementation for the tap_ctl_list and tap_ctl_list_free functions

In reply to Thanos Makatos

They will be used in a later patch.

tools/blktap3/control/tap-ctl-list.c new file +44 -0
diff -r a17c7217bef2 -r 7cbf20d7e4d6 tools/blktap3/control/tap-ctl-list.c
@@ -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;
+}
#34 lore raw

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

tools/blktap3/control/tap-ctl-list.c line 32

Quoted 3 lines · Thanos Makatos
tools/blktap3/control/tap-ctl-list.c new file +3 -0
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif

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

Quoted 17 lines
tools/blktap3/control/tap-ctl-list.c new file +12 -0
+
+#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
[email protected]
http://lists.xen.org/xen-devel

#35 lore raw

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

Quoted 10 lines

-----Original Message-----
From: Ian Campbell
Sent: 26 October 2012 12:37
To: Thanos Makatos
Cc: [email protected]
Subject: Re: [Xen-devel] [PATCH 11 of 16 RFC] blktap3: Provide empty implementation for the tap_ctl_list and tap_ctl_list_free functions

Quoted 7 lines

tools/blktap3/control/tap-ctl-list.c line 32

Quoted 3 lines · Thanos Makatos
tools/blktap3/control/tap-ctl-list.c new file +3 -0
+#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.

Quoted 19 lines
Quoted 18 lines
tools/blktap3/control/tap-ctl-list.c new file +12 -0
+
+#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
[email protected]
http://lists.xen.org/xen-devel

#36 lore raw

RFC PATCH 1/1 blktap3: Provide empty implementation for tap_ctl_destroy

In reply to Thanos Makatos

It will be used in a later patch.

tools/blktap3/control/tap-ctl-destroy.c new file +42 -0
diff -r 7cbf20d7e4d6 -r 26faf531ed07 tools/blktap3/control/tap-ctl-destroy.c
@@ -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;
+}
#37 lore raw

RFC PATCH 1/1 blktap3: Provide empty implementation for tap_ctl_create

In reply to Thanos Makatos

It will be used in a later patch.

tools/blktap3/control/tap-ctl-create.c new file +43 -0
diff -r 26faf531ed07 -r 092fce05f530 tools/blktap3/control/tap-ctl-create.c
@@ -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;
+}
#38 lore raw

RFC PATCH 1/1 blktap3: Provide Makefile for compiling the blktap3 control library

In reply to Thanos Makatos

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

tools/blktap3/control/Makefile new file +57 -0
diff -r 092fce05f530 -r 9f5d1f1f9791 tools/blktap3/control/Makefile
@@ -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
#39 lore raw

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

tools/blktap3/control/Makefile line 4

Quoted 10 lines · Thanos Makatos

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

diff -r 092fce05f530 -r 9f5d1f1f9791 tools/blktap3/control/Makefile

tools/blktap3/control/Makefile new file +4 -0
@@ -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.

Quoted 33 lines
tools/blktap3/control/Makefile new file +33 -0
+
+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 $< $@
+

tools/blktap3/control/Makefile line 47

Quoted 10 lines
tools/blktap3/control/Makefile new file +10 -0
+$(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.

Quoted 15 lines
tools/blktap3/control/Makefile new file +10 -0
+	$(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
[email protected]
http://lists.xen.org/xen-devel

#40 lore raw

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

Quoted 62 lines

-----Original Message-----
From: Ian Campbell
Sent: 26 October 2012 12:41
To: Thanos Makatos
Cc: [email protected]
Subject: Re: [Xen-devel] [PATCH 14 of 16 RFC] blktap3: Provide Makefile for compiling the blktap3 control library

tools/blktap3/control/Makefile

Quoted 5 lines · Thanos Makatos

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

diff -r 092fce05f530 -r 9f5d1f1f9791 tools/blktap3/control/Makefile

tools/blktap3/control/Makefile new file +0 -0

+0100

tools/blktap3/control/Makefile line 4

Quoted 5 lines
tools/blktap3/control/Makefile new file +4 -0
@@ -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.

Quoted 37 lines
tools/blktap3/control/Makefile new file +37 -0
+
+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)
Quoted 8 lines

tools/blktap3/control/Makefile line 47

Quoted 6 lines
tools/blktap3/control/Makefile new file +6 -0
+	$(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?

Quoted 17 lines
Quoted 16 lines
tools/blktap3/control/Makefile new file +10 -0
+	$(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
[email protected]
http://lists.xen.org/xen-devel

#41 lore raw

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

Quoted 8 lines · Thanos Makatos
Quoted 6 lines

tools/blktap3/control/Makefile line 47

Quoted 4 lines
tools/blktap3/control/Makefile new file +4 -0
+
+# 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.

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
tools/blktap3/control/Makefile new file +3 -0
+	$(INSTALL_PROG) $(LIB_SHARED) $(DESTDIR)$(LIBDIR)
+	ln -sf $(LIBSONAME) $(DESTDIR)$(LIBDIR)/$(LIBNAME).so
+	ln -sf $(LIB_SHARED) $(DESTDIR)$(LIBDIR)/$(LIBSONAME)
#42 lore raw

RFC PATCH 1/1 blktap3: Include the blktap3/control Makefile in the blktap3 compilation

In reply to Thanos Makatos
tools/blktap3/Makefile +19 -6
diff -r 9f5d1f1f9791 -r b12c1bb767d3 tools/blktap3/Makefile
@@ -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-%
#43 lore raw

RFC PATCH 1/1 blktap3: Implement libxl__blktap_devpath and libxl__device_destroy_tapdisk

In reply to 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.

tools/libxl/Makefile +5 -2
diff -r b12c1bb767d3 -r 0f87cc018fb6 tools/libxl/Makefile
@@ -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)
tools/libxl/libxl_blktap3.c +170 -5

181 lines not on this page · raw message

#44 lore raw

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

Quoted 10 lines · 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

tools/libxl/Makefile LIBUUID_LIBS += -luuid · line 23

Quoted 5 lines
tools/libxl/Makefile +1 -1
@@ -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?

tools/libxl/Makefile CFLAGS_LIBXL += $(CFLAGS_libblktapctl) · line 34

Quoted 10 lines
tools/libxl/Makefile +3 -1
 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?

tools/libxl/Makefile libxenlight.so.$(MAJOR): libxenlight.so. · line 177

Quoted 8 lines
tools/libxl/Makefile +1 -0
 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.

Quoted 23 lines
tools/libxl/Makefile +0 -0
 	$(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

tools/libxl/libxl_blktap3.c +14 -2
 
-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)
+{

tools/libxl/libxl_blktap3.c

Quoted 10 lines
tools/libxl/libxl_blktap3.c +10 -0
+    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?

Quoted 11 lines

What if somebody

tools/libxl/libxl_blktap3.c +10 -0
+     * 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");
+
+    /*

tools/libxl/libxl_blktap3.c line 114

Quoted 10 lines
tools/libxl/libxl_blktap3.c +10 -0
+     * 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.

#45 lore raw

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

Quoted 24 lines

-----Original Message-----
From: Ian Campbell
Sent: 26 October 2012 12:46
To: Thanos Makatos
Cc: [email protected]
Subject: Re: [Xen-devel] [PATCH 16 of 16 RFC] blktap3: Implement libxl__blktap_devpath and libxl__device_destroy_tapdisk

Quoted 2 lines · Thanos Makatos

Provide implementation for the libxl__blktap_devpath and libxl__device_destroy_tapdisk functions: the former spawns the

tapdisk

Quoted 3 lines

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.

Quoted 6 lines

diff -r b12c1bb767d3 -r 0f87cc018fb6 tools/libxl/Makefile

tools/libxl/Makefile +0 -0
@@ -20,7 +20,7 @@ LIBUUID_LIBS += -luuid  endif
 
Quoted 8 lines

tools/libxl/Makefile LIBUUID_LIBS += -luuid endif · line 25

Quoted 6 lines
tools/libxl/Makefile +3 -1
 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.

Quoted 20 lines

tools/libxl/Makefile CFLAGS_LIBXL += $(CFLAGS_libblktapctl) · line 34

Quoted 10 lines
tools/libxl/Makefile +4 -1
 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?

Quoted 6 lines

LIBXL_LIBS += $(PTHREAD_LIBS)

LIBXLU_LIBS =

tools/libxl/Makefile +0 -0
@@ -172,6 +174,7 @@ libxenlight.so.$(MAJOR): libxenlight.so.
 	ln -sf $< $@
 
Quoted 7 lines

tools/libxl/Makefile libxenlight.so.$(MAJOR): libxenlight.so. · line 177

Quoted 2 lines
tools/libxl/Makefile +1 -0
 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?

Quoted 29 lines

tools/libxl/libxl_blktap3.c

Quoted 10 lines
tools/libxl/Makefile +0 -0
 	$(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

tools/libxl/libxl_blktap3.c +0 -1
 
-int libxl__device_destroy_tapdisk(libxl__gc *gc, const char

*be_path) {

Quoted 18 lines
tools/libxl/libxl_blktap3.c +17 -1
-    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;
Quoted 8 lines

tools/libxl/libxl_blktap3.c

Quoted 6 lines
tools/libxl/libxl_blktap3.c +6 -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?

Quoted 18 lines

tools/libxl/libxl_blktap3.c

Quoted 6 lines

What if somebody

tools/libxl/libxl_blktap3.c +4 -0
+     * 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);

Quoted 11 lines
tools/libxl/libxl_blktap3.c +11 -0
+        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))) {
Quoted 8 lines

tools/libxl/libxl_blktap3.c line 114

Quoted 5 lines
tools/libxl/libxl_blktap3.c +5 -0
+        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.

Quoted 2 lines

Ian.

#46 lore raw

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

Quoted 14 lines
Quoted 6 lines
Quoted 6 lines

LIBXL_LIBS += $(PTHREAD_LIBS)

LIBXLU_LIBS =

tools/libxl/Makefile +0 -0
@@ -172,6 +174,7 @@ libxenlight.so.$(MAJOR): libxenlight.so.
 	ln -sf $< $@
 
Quoted 7 lines

tools/libxl/Makefile libxenlight.so.$(MAJOR): libxenlight.so. · line 177

Quoted 2 lines
tools/libxl/Makefile +1 -0
 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.

Quoted 3 lines

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.

Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
tools/libxl/libxl_blktap3.c +7 -0
+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;
Quoted 8 lines

tools/libxl/libxl_blktap3.c

Quoted 6 lines
tools/libxl/libxl_blktap3.c +6 -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?

Quoted 4 lines

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

#47 lore raw

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

Quoted 24 lines

-----Original Message-----
From: Ian Campbell [mailto:[email protected]]
Sent: 30 October 2012 11:26
To: Thanos Makatos
Cc: [email protected]
Subject: Re: [Xen-devel] [PATCH 16 of 16 RFC] blktap3: Implement libxl__blktap_devpath and libxl__device_destroy_tapdisk

Quoted 14 lines
Quoted 13 lines

tools/libxl/Makefile libxenlight.so.$(MAJOR): libxenlight.so. · line 177

Quoted 8 lines

LIBXL_LIBS += $(PTHREAD_LIBS)

LIBXLU_LIBS =

tools/libxl/Makefile +1 -0
@@ -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.

Quoted 7 lines
Quoted 3 lines

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.

Quoted 26 lines
Quoted 16 lines
Quoted 15 lines

tools/libxl/libxl_blktap3.c

Quoted 13 lines
tools/libxl/libxl_blktap3.c +12 -0
+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?

Quoted 2 lines

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,

Quoted 2 lines

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

#48 lore raw
In reply to Thanos Makatos
Quoted 7 lines · 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.

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.

#49 lore raw
Quoted 8 lines

-----Original Message-----
From: Ian Campbell
Sent: 26 October 2012 12:15
To: Thanos Makatos
Cc: [email protected]
Subject: Re: [Xen-devel] [PATCH 00 of 16 RFC] add libxl support for blktap3 and introduce the blktap3 build system

Quoted 10 lines

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.