[PATCH RFC 0/4] tools: Upstream blktap "2.5" as an external repo

[PATCH RFC 0/4] tools: Upstream blktap "2.5" as an external repo

[George Dunlap]

For some time, the blktap2 in-tree has bitrotted.  Many years ago the
XenServer team at Citrix forked the code into a separate repository;
several attempts have been made to upstream those changes back into
Xen, to no avail.

The blktap code at the moment is the only source of performant vhd
format integration.  It's additionally in use by projects like the
COLO project.

This patch series removes the in-tree blktap2 code and treats the
XenServer blktap tree as an upstream.  I've gotten agreement from the
XenServer team to act as an upstream -- to accept patches fixing bugs,
to help track down errors, and to attempt to help fix build breakages
introduced by development.

At the moment we're using the "blktap2" branch of XenServer's
blktap.git.  (This has been sometimes known as "blktap 2.5".)  This
branch is maintianed in order to provide a buildroot for OpenStack,
and has also been used by the CentOS xen packages for several years
now.

The series consists of four patches:

1/4 Reorg tools/Makefile so all "external repo" targets are together

2/4 Add external blktap repo; build but don't install

3/4 Modify libxl to use external repo; stop building in-tree blktap2

4/4 Remove blktap2 from xen.git

The last patch is pretty massive and not very interesting, so it won't be sent with
this series.  The full patch series can be found here:

git://xenbits.xen.org/people/gdunlap/xen.git  out/blktap25/rfc-v1

This series has some niggles (noted in the patches themselves), and so it not yet
ready to apply as-is.

One of the big things to note is that blktap.git seems to require
autogen to build; at the moment including this patch will increase the
build dependencies to include autoconf and libtool.  It would be good
if we could remove this dependency before a release, if possible.

 -George Dunlap

CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Dave Scott <Dave.Scott@eu.citrix.com>
CC: Jonathan Ludlam <Jonathan.Ludlam@eu.citrix.com>
CC: Wen Congyang <wency@cn.fujitsu.com>
CC: Yang Hongyang <yanghy@cn.fujitsu.com>

[PATCH RFC 1/4] build: Reorganize and briefly document "external repo" template in tools/Makefile

[George Dunlap]

No functional changes.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Dave Scott <Dave.Scott@eu.citrix.com>
CC: Jonathan Ludlam <Jonathan.Ludlam@eu.citrix.com>
CC: Wen Congyang <wency@cn.fujitsu.com>
CC: Yang Hongyang <yanghy@cn.fujitsu.com>
---
 tools/Makefile | 52 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 44 insertions(+), 8 deletions(-)

diff --git a/tools/Makefile b/tools/Makefile
index 5d7a75f..966354a 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -129,6 +129,41 @@ ifneq ($(QEMU_ROOT),.)
 export QEMU_ROOT
 endif
 
+# Targets for external trees:
+#  ${target}-dir-find
+#    See if the directory exists and check it out if not.
+#  ${target}-dir-force-update
+#    Pull to the most recent update (as if you had checked it out for the
+#    first time)
+#  subdir-all-${target}-dir
+#    Do "make all" for ${target}, including all prerequisites (such as 
+#    configure)
+#  subdir-install-${target}-dir
+#    Do "make install" for $TARGET
+#  subdir-clean-${target}-dir
+#    Do "make clean" for $TARGET
+#
+# Directories for external trees:
+#  ${target}-dir
+#    Used for local builds.  Usually a link to ${target}-dir-remote
+#  ${target}-dir-remote
+#    Where remote repositories are cloned
+#  ${target}
+#    Where a copy of the source files are put when building a source 
+#    tarball for release
+#
+# Expected variables:
+#   ${TARGET}_URL
+#     A url from which to clone a git repo
+#   ${TARGET}_REVISION
+#     The target revision to check out when doing "find" or "force-update"
+#   ${TARGET}_INTREE
+#     The directory where the subtree can be found (usually used when building
+#     a source tarball)
+#   ${TARGET}_LOC
+#     The ultimate location of the source (either a local dir or remote URL)
+
+# External target: qemu-xen-traditional
 qemu-xen-traditional-dir-find:
 	set -ex; \
 	if test -d $(QEMU_TRADITIONAL_LOC); then \
@@ -138,14 +173,6 @@ qemu-xen-traditional-dir-find:
 		$(XEN_ROOT)/scripts/git-checkout.sh $(QEMU_TRADITIONAL_LOC) $(QEMU_TRADITIONAL_REVISION) qemu-xen-traditional-dir; \
 	fi
 
-qemu-xen-dir-find:
-	if test -d $(QEMU_UPSTREAM_LOC) ; then \
-		mkdir -p qemu-xen-dir; \
-	else \
-		export GIT=$(GIT); \
-		$(XEN_ROOT)/scripts/git-checkout.sh $(QEMU_UPSTREAM_LOC) $(QEMU_UPSTREAM_REVISION) qemu-xen-dir ; \
-	fi
-
 .PHONY: qemu-xen-traditional-dir-force-update
 qemu-xen-traditional-dir-force-update: qemu-xen-traditional-dir-find
 	set -ex; \
@@ -183,6 +210,15 @@ subdir-clean-qemu-xen-traditional-dir:
 		$(MAKE) -C qemu-xen-traditional-dir clean; \
 	fi
 
+# External target: qemu-xen
+qemu-xen-dir-find:
+	if test -d $(QEMU_UPSTREAM_LOC) ; then \
+		mkdir -p qemu-xen-dir; \
+	else \
+		export GIT=$(GIT); \
+		$(XEN_ROOT)/scripts/git-checkout.sh $(QEMU_UPSTREAM_LOC) $(QEMU_UPSTREAM_REVISION) qemu-xen-dir ; \
+	fi
+
 .PHONY: qemu-xen-dir-force-update
 qemu-xen-dir-force-update: qemu-xen-dir-find
 	set -ex; \
-- 
1.9.1

[PATCH RFC 2/4] build: Download and build extnernal blktap2.5 repo

[George Dunlap]

Download and build XenServer's blktap as an external tree, similar to
qemu-xen.

As of this patch we just download and build it, but don't install or
use it.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---

FIXME: Directly use the XenServer github repo for now, while we're
discussing things.  If we decide to take this series, we'll have to
clone the tree on xenbits and remove the FIXME line.

CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Dave Scott <Dave.Scott@eu.citrix.com>
CC: Jonathan Ludlam <Jonathan.Ludlam@eu.citrix.com>
CC: Wen Congyang <wency@cn.fujitsu.com>
CC: Yang Hongyang <yanghy@cn.fujitsu.com>
---
 .gitignore           |  3 +++
 Config.mk            |  9 +++++++++
 tools/Makefile       | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 tools/misc/mktarball |  2 ++
 4 files changed, 63 insertions(+)

diff --git a/.gitignore b/.gitignore
index c6185a0..1bc9602 100644
--- a/.gitignore
+++ b/.gitignore
@@ -261,6 +261,9 @@ tools/qemu-xen-dir
 tools/qemu-xen-traditional-dir-remote
 tools/qemu-xen-traditional-dir
 
+tools/blktap-dir-remote
+tools/blktap-dir
+
 tools/firmware/seabios-dir-remote
 tools/firmware/seabios-dir
 
diff --git a/Config.mk b/Config.mk
index b243fac..7359763 100644
--- a/Config.mk
+++ b/Config.mk
@@ -224,6 +224,7 @@ XEN_EXTFILES_URL ?= http://xenbits.xen.org/xen-extfiles
 # Where to look for inlined subtrees (for example, from a tarball)
 QEMU_UPSTREAM_INTREE ?= $(XEN_ROOT)/tools/qemu-xen
 QEMU_TRADITIONAL_INTREE ?= $(XEN_ROOT)/tools/qemu-xen-traditional
+BLKTAP_UPSTREAM_INTREE ?= $(XEN_ROOT)/tools/blktap
 
 
 # Handle legacy options
@@ -240,19 +241,24 @@ ifneq (,$(QEMU_TAG))
 QEMU_TRADITIONAL_REVISION ?= $(QEMU_TAG)
 endif
 
+# FIXME
+BLKTAP_UPSTREAM_URL ?= https://github.com/xapi-project/blktap.git
 ifeq ($(GIT_HTTP),y)
 OVMF_UPSTREAM_URL ?= http://xenbits.xen.org/git-http/ovmf.git
 QEMU_UPSTREAM_URL ?= http://xenbits.xen.org/git-http/qemu-upstream-unstable.git
 QEMU_TRADITIONAL_URL ?= http://xenbits.xen.org/git-http/qemu-xen-unstable.git
 SEABIOS_UPSTREAM_URL ?= http://xenbits.xen.org/git-http/seabios.git
 MINIOS_UPSTREAM_URL ?= http://xenbits.xen.org/git-http/mini-os.git
+BLKTAP_UPSTREAM_URL ?= http://xenbits.xen.org/git-http/blktap.git
 else
 OVMF_UPSTREAM_URL ?= git://xenbits.xen.org/ovmf.git
 QEMU_UPSTREAM_URL ?= git://xenbits.xen.org/qemu-upstream-unstable.git
 QEMU_TRADITIONAL_URL ?= git://xenbits.xen.org/qemu-xen-unstable.git
 SEABIOS_UPSTREAM_URL ?= git://xenbits.xen.org/seabios.git
 MINIOS_UPSTREAM_URL ?= git://xenbits.xen.org/mini-os.git
+BLKTAP_UPSTREAM_URL ?= git://xenbits.xen.org/blktap.git
 endif
+BLKTAP_UPSTREAM_REVISION ?= d73c74874a449c18dc1528076e5c0671cc5ed409
 OVMF_UPSTREAM_REVISION ?= a065efc7c7ce8bb3e5cb3e463099d023d4a92927
 QEMU_UPSTREAM_REVISION ?= master
 MINIOS_UPSTREAM_REVISION ?= edfd5aae6ec5ba7d0a8834a3e9dfe5e69424150a
@@ -281,6 +287,9 @@ QEMU_TRADITIONAL_LOC ?= $(call or,$(wildcard $(QEMU_TRADITIONAL_INTREE)),\
 QEMU_UPSTREAM_LOC ?= $(call or,$(wildcard $(QEMU_UPSTREAM_INTREE)),\
                                $(QEMU_UPSTREAM_URL))
 
+BLKTAP_UPSTREAM_LOC ?= $(call or,$(wildcard $(BLKTAP_UPSTREAM_INTREE)),\
+                               $(BLKTAP_UPSTREAM_URL))
+
 # Short answer -- do not enable this unless you know what you are
 # doing and are prepared for some pain.
 
diff --git a/tools/Makefile b/tools/Makefile
index 966354a..b0b2a02 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -25,6 +25,7 @@ SUBDIRS-$(CONFIG_Linux) += libvchan
 ifneq "$(MAKECMDGOALS)" "distclean"
 SUBDIRS-$(CONFIG_QEMU_TRAD) += qemu-xen-traditional-dir
 SUBDIRS-$(CONFIG_QEMU_XEN) += qemu-xen-dir
+SUBDIRS-$(CONFIG_BLKTAP2) += blktap-dir
 endif
 
 SUBDIRS-y += xenpmd
@@ -108,6 +109,7 @@ clean: subdirs-clean
 distclean: subdirs-distclean
 	rm -rf qemu-xen-traditional-dir qemu-xen-traditional-dir-remote
 	rm -rf qemu-xen-dir qemu-xen-dir-remote
+	rm -rf blktap-dir blktap-dir-remote
 	rm -rf ../config/Tools.mk config.h config.log config.status \
 		config.cache autom4te.cache
 
@@ -276,6 +278,49 @@ subdir-clean-qemu-xen-dir:
 		$(MAKE) -C qemu-xen-dir clean; \
 	fi
 
+# External target: blktap
+blktap-dir-find:
+	if test -d $(BLKTAP_UPSTREAM_LOC) ; then \
+		mkdir -p blktap-dir; \
+	else \
+		export GIT=$(GIT); \
+		$(XEN_ROOT)/scripts/git-checkout.sh $(BLKTAP_UPSTREAM_LOC) $(BLKTAP_UPSTREAM_REVISION) blktap-dir ; \
+	fi
+
+.PHONY: blktap-dir-force-update
+blktap-dir-force-update: blktap-dir-find
+	set -ex; \
+	if [ "$(BLKTAP_UPSTREAM_REVISION)" ]; then \
+		cd blktap-dir-remote; \
+		$(GIT) fetch origin; \
+		$(GIT) reset --hard $(BLKTAP_UPSTREAM_REVISION); \
+	fi
+
+subdir-all-blktap-dir: blktap-dir-find
+	if test -d $(BLKTAP_UPSTREAM_LOC) ; then \
+		source=$(BLKTAP_UPSTREAM_LOC); \
+	else \
+		source=.; \
+	fi; \
+	cd blktap-dir; \
+	$$source/autogen.sh ; \
+	$$source/configure --prefix=$(PREFIX) \
+		--libdir=$(LIBDIR) \
+		--libexecdir=$(LIBEXEC) ; \
+	$(MAKE) all
+
+# Don't install upstream blktap for now.
+subdir-install-blktap-dir: subdir-all-blktap-dir
+	if false ; do cd blktap-dir; \
+	$(MAKE) install \
+	fi
+
+subdir-clean-blktap-dir:
+	set -e; if test -d blktap-dir/.; then \
+		$(MAKE) -C blktap-dir clean; \
+	fi
+
+
 subdir-clean-debugger/gdbsx subdir-distclean-debugger/gdbsx: .phony
 	$(MAKE) -C debugger/gdbsx clean
 
@@ -305,9 +350,13 @@ endif
 ifeq ($(CONFIG_QEMU_TRAD),y)
 	$(MAKE) qemu-xen-traditional-dir-force-update
 endif
+ifeq ($(CONFIG_BLKTAP2),y)
+	$(MAKE) blktap-dir-force-update
+endif
 	$(MAKE) -C firmware subtree-force-update
 
 subtree-force-update-all:
 	$(MAKE) qemu-xen-dir-force-update
 	$(MAKE) qemu-xen-traditional-dir-force-update
+	$(MAKE) blktap-dir-force-update
 	$(MAKE) -C firmware subtree-force-update-all
diff --git a/tools/misc/mktarball b/tools/misc/mktarball
index 73282b5..6506f63 100755
--- a/tools/misc/mktarball
+++ b/tools/misc/mktarball
@@ -33,6 +33,8 @@ git_archive_into $xen_root/tools/qemu-xen-dir-remote $tdir/xen-$desc/tools/qemu-
 
 git_archive_into $xen_root/tools/qemu-xen-traditional-dir-remote $tdir/xen-$desc/tools/qemu-xen-traditional
 
+git_archive_into $xen_root/tools/blktap-dir-remote $tdir/xen-$desc/tools/blktap
+
 git_archive_into $xen_root/extras/mini-os-remote $tdir/xen-$desc/extras/mini-os
 
 GZIP=-9v tar cz -f $xen_root/dist/xen-$desc.tar.gz -C $tdir xen-$desc
-- 
1.9.1

[PATCH RFC 3/4] libxl: Use XenServer's blktap2.5

[George Dunlap]

Switch to installing blktap2.5 and not building or installing in-tree
blktap2.

Port libxl to run on the newest blktap2.5.

tap_ctl_find() has gone away, so in some cases we use
tap_ctl_find_minor(), and in other cases we use a
locally-reimplemented version of tap_ctl_find().

tap_ctl_create() has an extra three parameters, including a flags
parameter which can be used to designate a disk image as read-only.
Pass the readwrite disk flag here so we can now open backends on
read-only storage.

tap_ctl_destroy() now has two extra arguments as well; add sensible
default values.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

---

NOTE: I'm pretty sure this won't build from a source tarball at the
moment, since XEN_BLKTAP2 is set to tools/blktap-dir, which (IIRC)
will be just an empty directory if tools/bltkap exists.  (And
tools/blktap is where the release tarball script will put the blktap
subdir.)

If this gets traction, I'll figure out how to do fix that before I
send a non-RFC series.


CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Dave Scott <Dave.Scott@eu.citrix.com>
CC: Jonathan Ludlam <Jonathan.Ludlam@eu.citrix.com>
CC: Wen Congyang <wency@cn.fujitsu.com>
CC: Yang Hongyang <yanghy@cn.fujitsu.com>
---
 tools/Makefile               |  7 ++----
 tools/Rules.mk               |  8 +++---
 tools/libxl/libxl.c          |  6 +++--
 tools/libxl/libxl_blktap2.c  | 58 ++++++++++++++++++++++++++++++++++++--------
 tools/libxl/libxl_dm.c       |  3 ++-
 tools/libxl/libxl_internal.h |  3 ++-
 6 files changed, 62 insertions(+), 23 deletions(-)

diff --git a/tools/Makefile b/tools/Makefile
index b0b2a02..06374bf 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -16,7 +16,6 @@ SUBDIRS-y += console
 SUBDIRS-y += xenmon
 SUBDIRS-y += xenstat
 SUBDIRS-$(CONFIG_Linux) += memshr 
-SUBDIRS-$(CONFIG_BLKTAP2) += blktap2
 SUBDIRS-$(CONFIG_NetBSD) += xenbackendd
 SUBDIRS-y += libfsimage
 SUBDIRS-$(CONFIG_Linux) += libvchan
@@ -309,11 +308,9 @@ subdir-all-blktap-dir: blktap-dir-find
 		--libexecdir=$(LIBEXEC) ; \
 	$(MAKE) all
 
-# Don't install upstream blktap for now.
 subdir-install-blktap-dir: subdir-all-blktap-dir
-	if false ; do cd blktap-dir; \
-	$(MAKE) install \
-	fi
+	cd blktap-dir; \
+	$(MAKE) install
 
 subdir-clean-blktap-dir:
 	set -e; if test -d blktap-dir/.; then \
diff --git a/tools/Rules.mk b/tools/Rules.mk
index 3c29d07..f104072 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -14,7 +14,7 @@ XEN_LIBXC          = $(XEN_ROOT)/tools/libxc
 XEN_XENLIGHT       = $(XEN_ROOT)/tools/libxl
 XEN_XENSTORE       = $(XEN_ROOT)/tools/xenstore
 XEN_LIBXENSTAT     = $(XEN_ROOT)/tools/xenstat/libxenstat/src
-XEN_BLKTAP2        = $(XEN_ROOT)/tools/blktap2
+XEN_BLKTAP2        = $(XEN_ROOT)/tools/blktap-dir
 XEN_LIBVCHAN       = $(XEN_ROOT)/tools/libvchan
 
 CFLAGS_xeninclude = -I$(XEN_INCLUDE)
@@ -64,9 +64,9 @@ endif
 LIBXL_BLKTAP ?= $(CONFIG_BLKTAP2)
 
 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_BLKTAP2)/include $(CFLAGS_xeninclude)
+LDLIBS_libblktapctl = -L$(XEN_BLKTAP2)/control/.libs -lblktapctl
+SHLIB_libblktapctl  = -Wl,-rpath-link=$(XEN_BLKTAP2)/control/.libs
 else
 CFLAGS_libblktapctl =
 LDLIBS_libblktapctl =
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 94b4d59..0270753 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2437,7 +2437,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
             case LIBXL_DISK_BACKEND_TAP:
                 if (dev == NULL) {
                     dev = libxl__blktap_devpath(gc, disk->pdev_path,
-                                                disk->format);
+                                                disk->format,
+                                                disk->readwrite);
                     if (!dev) {
                         LOG(ERROR, "failed to get blktap devpath for %p\n",
                             disk->pdev_path);
@@ -3031,7 +3032,8 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
                 break;
             case LIBXL_DISK_FORMAT_VHD:
                 dev = libxl__blktap_devpath(gc, disk->pdev_path,
-                                            disk->format);
+                                            disk->format,
+                                            disk->readwrite);
                 break;
             case LIBXL_DISK_FORMAT_QCOW:
             case LIBXL_DISK_FORMAT_QCOW2:
diff --git a/tools/libxl/libxl_blktap2.c b/tools/libxl/libxl_blktap2.c
index 2053403..24939d7 100644
--- a/tools/libxl/libxl_blktap2.c
+++ b/tools/libxl/libxl_blktap2.c
@@ -23,26 +23,62 @@ int libxl__blktap_enabled(libxl__gc *gc)
     return !tap_ctl_check(&msg);
 }
 
+static int tap_ctl_find(const char *type, const char *disk, tap_list_t *tap) 
+{
+    int err;
+    struct list_head list = LIST_HEAD_INIT(list);
+    tap_list_t *entry;
+
+    err = tap_ctl_list(&list);
+    if (err)
+        return err;
+
+    err = ERROR_FAIL;
+
+    tap_list_for_each_entry(entry, &list) {
+        if (type && (!entry->type || strcmp(entry->type, type)))
+            continue;
+        
+        if (disk && (!entry->path || strcmp(entry->path, disk)))
+            continue;
+        
+        tap->minor = entry->minor;
+        tap->pid = entry->pid;
+        err = 0;
+        break;
+    }
+    tap_ctl_list_free(&list);
+    
+    return err;
+}
+
 char *libxl__blktap_devpath(libxl__gc *gc,
                             const char *disk,
-                            libxl_disk_format format)
+                            libxl_disk_format format,
+                            int readwrite)
 {
     const char *type;
     char *params, *devname = NULL;
-    tap_list_t tap;
     int err;
+    int minor;
+    int flags;
 
     type = libxl__device_disk_string_of_format(format);
-    err = tap_ctl_find(type, disk, &tap);
-    if (err == 0) {
-        devname = libxl__sprintf(gc, "/dev/xen/blktap-2/tapdev%d", tap.minor);
+
+    minor = tap_ctl_find_minor(type, disk);
+    if (minor >= 0) {
+        devname = libxl__sprintf(gc, "/dev/xen/blktap-2/tapdev%d", minor);
         if (devname)
             return devname;
     }
 
     params = libxl__sprintf(gc, "%s:%s", type, disk);
-    err = tap_ctl_create(params, &devname);
+    fprintf(stderr, "DEBUG %s %d %s\n", __func__, __LINE__, params);
+    flags = readwrite ? 0 : TAPDISK_MESSAGE_FLAG_RDONLY;
+
+    err = tap_ctl_create(params, &devname, flags, -1, 0, 0);
     if (!err) {
+        fprintf(stderr, "DEBUG %s %d %s\n", __func__, __LINE__, devname);
         libxl__ptr_add(gc, devname);
         return devname;
     }
@@ -55,7 +91,8 @@ int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *params)
 {
     char *type, *disk;
     int err;
-    tap_list_t tap;
+    struct list_head list = LIST_HEAD_INIT(list);
+    tap_list_t tap = { .minor=-1, .pid=-1 };
 
     type = libxl__strdup(gc, params);
 
@@ -65,19 +102,20 @@ int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *params)
         return ERROR_INVAL;
     }
 
+    fprintf(stderr, "DEBUG %s %d type=%s disk=%s\n",__func__,__LINE__,type,disk);
     *disk++ = '\0';
 
     err = tap_ctl_find(type, disk, &tap);
-    if (err < 0) {
+    if (err) {
         /* returns -errno */
         LOGEV(ERROR, -err, "Unable to find type %s disk %s", type, disk);
         return ERROR_FAIL;
     }
 
-    err = tap_ctl_destroy(tap.id, tap.minor);
+    err = tap_ctl_destroy(tap.pid, tap.minor, 1, NULL);
     if (err < 0) {
         LOGEV(ERROR, -err, "Failed to destroy tap device id %d minor %d",
-              tap.id, tap.minor);
+              tap.pid, tap.minor);
         return ERROR_FAIL;
     }
 
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index a8b08f2..0d91b54 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -826,7 +826,8 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                 if (disks[i].backend == LIBXL_DISK_BACKEND_TAP) {
                     format = qemu_disk_format_string(LIBXL_DISK_FORMAT_RAW);
                     pdev_path = libxl__blktap_devpath(gc, disks[i].pdev_path,
-                                                      disks[i].format);
+                                                      disks[i].format,
+                                                      disks[i].readwrite);
                 } else {
                     pdev_path = disks[i].pdev_path;
                 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 934465a..441d722 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1550,7 +1550,8 @@ _hidden int libxl__blktap_enabled(libxl__gc *gc);
  */
 _hidden char *libxl__blktap_devpath(libxl__gc *gc,
                                     const char *disk,
-                                    libxl_disk_format format);
+                                    libxl_disk_format format,
+                                    int readwrite);
 
 /* libxl__device_destroy_tapdisk:
  *   Destroys any tapdisk process associated with the backend represented
-- 
1.9.1

[PATCH RFC 4/4] tools: Remove in-tree blktap2

[George Dunlap]

This patch is pure removal; the switch to using upstream blktap
happened in a previous patch.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

---
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Dave Scott <Dave.Scott@eu.citrix.com>
CC: Jonathan Ludlam <Jonathan.Ludlam@eu.citrix.com>
CC: Wen Congyang <wency@cn.fujitsu.com>
CC: Yang Hongyang <yanghy@cn.fujitsu.com>
---
 tools/blktap2/Makefile                     |   20 -
 tools/blktap2/README                       |  321 ---
 tools/blktap2/control/Makefile             |   80 -
 tools/blktap2/control/tap-ctl-allocate.c   |  242 --
 tools/blktap2/control/tap-ctl-attach.c     |   61 -
 tools/blktap2/control/tap-ctl-check.c      |   79 -
 tools/blktap2/control/tap-ctl-close.c      |   87 -
 tools/blktap2/control/tap-ctl-create.c     |   67 -
 tools/blktap2/control/tap-ctl-destroy.c    |   56 -
 tools/blktap2/control/tap-ctl-detach.c     |   61 -
 tools/blktap2/control/tap-ctl-free.c       |   54 -
 tools/blktap2/control/tap-ctl-ipc.c        |  249 ---
 tools/blktap2/control/tap-ctl-list.c       |  536 -----
 tools/blktap2/control/tap-ctl-major.c      |   69 -
 tools/blktap2/control/tap-ctl-open.c       |   75 -
 tools/blktap2/control/tap-ctl-pause.c      |   59 -
 tools/blktap2/control/tap-ctl-spawn.c      |  174 --
 tools/blktap2/control/tap-ctl-unpause.c    |   64 -
 tools/blktap2/control/tap-ctl.c            |  815 -------
 tools/blktap2/control/tap-ctl.h            |  101 -
 tools/blktap2/drivers/Makefile             |  113 -
 tools/blktap2/drivers/aes.c                | 1319 -----------
 tools/blktap2/drivers/aes.h                |   28 -
 tools/blktap2/drivers/atomicio.c           |   61 -
 tools/blktap2/drivers/blk.h                |   36 -
 tools/blktap2/drivers/blk_linux.c          |   43 -
 tools/blktap2/drivers/blk_netbsd.c         |   41 -
 tools/blktap2/drivers/block-aio.c          |  258 ---
 tools/blktap2/drivers/block-cache.c        |  787 -------
 tools/blktap2/drivers/block-log.c          |  665 ------
 tools/blktap2/drivers/block-qcow.c         | 1501 -------------
 tools/blktap2/drivers/block-ram.c          |  256 ---
 tools/blktap2/drivers/block-remus.c        | 1733 --------------
 tools/blktap2/drivers/block-vhd.c          | 2322 -------------------
 tools/blktap2/drivers/bswap.h              |  179 --
 tools/blktap2/drivers/check_gcrypt         |   18 -
 tools/blktap2/drivers/hashtable.c          |  279 ---
 tools/blktap2/drivers/hashtable.h          |  204 --
 tools/blktap2/drivers/hashtable_itr.c      |  195 --
 tools/blktap2/drivers/hashtable_itr.h      |   96 -
 tools/blktap2/drivers/hashtable_private.h  |   90 -
 tools/blktap2/drivers/hashtable_utility.c  |   71 -
 tools/blktap2/drivers/hashtable_utility.h  |   55 -
 tools/blktap2/drivers/img2qcow.c           |  316 ---
 tools/blktap2/drivers/io-optimize.c        |  671 ------
 tools/blktap2/drivers/io-optimize.h        |   68 -
 tools/blktap2/drivers/libaio-compat.h      |  104 -
 tools/blktap2/drivers/lock.c               | 1000 ---------
 tools/blktap2/drivers/lock.h               |   51 -
 tools/blktap2/drivers/log.h                |  123 -
 tools/blktap2/drivers/md5.c                |  278 ---
 tools/blktap2/drivers/md5.h                |   15 -
 tools/blktap2/drivers/profile.h            |  191 --
 tools/blktap2/drivers/qcow-create.c        |  121 -
 tools/blktap2/drivers/qcow.h               |  131 --
 tools/blktap2/drivers/qcow2raw.c           |  443 ----
 tools/blktap2/drivers/scheduler.c          |  265 ---
 tools/blktap2/drivers/scheduler.h          |   65 -
 tools/blktap2/drivers/tapdisk-client.c     |  496 -----
 tools/blktap2/drivers/tapdisk-control.c    |  837 -------
 tools/blktap2/drivers/tapdisk-control.h    |   35 -
 tools/blktap2/drivers/tapdisk-diff.c       |  802 -------
 tools/blktap2/drivers/tapdisk-disktype.c   |  204 --
 tools/blktap2/drivers/tapdisk-disktype.h   |   62 -
 tools/blktap2/drivers/tapdisk-driver.c     |  101 -
 tools/blktap2/drivers/tapdisk-driver.h     |   62 -
 tools/blktap2/drivers/tapdisk-filter.c     |  272 ---
 tools/blktap2/drivers/tapdisk-filter.h     |   67 -
 tools/blktap2/drivers/tapdisk-image.c      |  169 --
 tools/blktap2/drivers/tapdisk-image.h      |   56 -
 tools/blktap2/drivers/tapdisk-interface.c  |  259 ---
 tools/blktap2/drivers/tapdisk-interface.h  |   54 -
 tools/blktap2/drivers/tapdisk-log.c        |  257 ---
 tools/blktap2/drivers/tapdisk-log.h        |   51 -
 tools/blktap2/drivers/tapdisk-queue.c      |  743 ------
 tools/blktap2/drivers/tapdisk-queue.h      |  125 --
 tools/blktap2/drivers/tapdisk-ring.c       |  439 ----
 tools/blktap2/drivers/tapdisk-ring.h       |   87 -
 tools/blktap2/drivers/tapdisk-server.c     |  345 ---
 tools/blktap2/drivers/tapdisk-server.h     |   67 -
 tools/blktap2/drivers/tapdisk-stream.c     |  605 -----
 tools/blktap2/drivers/tapdisk-utils.c      |  214 --
 tools/blktap2/drivers/tapdisk-utils.h      |   45 -
 tools/blktap2/drivers/tapdisk-vbd.c        | 1723 --------------
 tools/blktap2/drivers/tapdisk-vbd.h        |  207 --
 tools/blktap2/drivers/tapdisk.h            |  169 --
 tools/blktap2/drivers/tapdisk2.c           |  138 --
 tools/blktap2/drivers/td.c                 |  691 ------
 tools/blktap2/drivers/xmsnap               |   78 -
 tools/blktap2/include/Makefile             |   17 -
 tools/blktap2/include/atomicio.h           |   33 -
 tools/blktap2/include/blktap2.h            |   67 -
 tools/blktap2/include/blktaplib.h          |  242 --
 tools/blktap2/include/libvhd-journal.h     |   68 -
 tools/blktap2/include/libvhd.h             |  326 ---
 tools/blktap2/include/list.h               |  125 --
 tools/blktap2/include/lvm-util.h           |   71 -
 tools/blktap2/include/relative-path.h      |   43 -
 tools/blktap2/include/tapdisk-message.h    |  203 --
 tools/blktap2/include/vhd-util.h           |   44 -
 tools/blktap2/include/vhd-uuid.h           |   63 -
 tools/blktap2/include/vhd.h                |  219 --
 tools/blktap2/lvm/Makefile                 |   36 -
 tools/blktap2/lvm/lvm-util.c               |  349 ---
 tools/blktap2/vhd/Makefile                 |   51 -
 tools/blktap2/vhd/lib/Makefile             |   82 -
 tools/blktap2/vhd/lib/atomicio.c           |   61 -
 tools/blktap2/vhd/lib/libvhd-journal.c     | 1534 -------------
 tools/blktap2/vhd/lib/libvhd.c             | 3348 ----------------------------
 tools/blktap2/vhd/lib/relative-path.c      |  299 ---
 tools/blktap2/vhd/lib/vhd-util-check.c     |  980 --------
 tools/blktap2/vhd/lib/vhd-util-coalesce.c  |  218 --
 tools/blktap2/vhd/lib/vhd-util-create.c    |   80 -
 tools/blktap2/vhd/lib/vhd-util-fill.c      |  105 -
 tools/blktap2/vhd/lib/vhd-util-modify.c    |  132 --
 tools/blktap2/vhd/lib/vhd-util-query.c     |  159 --
 tools/blktap2/vhd/lib/vhd-util-read.c      |  742 ------
 tools/blktap2/vhd/lib/vhd-util-repair.c    |   84 -
 tools/blktap2/vhd/lib/vhd-util-resize.c    | 1131 ----------
 tools/blktap2/vhd/lib/vhd-util-revert.c    |  106 -
 tools/blktap2/vhd/lib/vhd-util-scan.c      | 1317 -----------
 tools/blktap2/vhd/lib/vhd-util-set-field.c |  106 -
 tools/blktap2/vhd/lib/vhd-util-snapshot.c  |  216 --
 tools/blktap2/vhd/lib/vhd-util-uuid.c      |  128 --
 tools/blktap2/vhd/vhd-update.c             |  259 ---
 tools/blktap2/vhd/vhd-util.c               |  163 --
 126 files changed, 40129 deletions(-)
 delete mode 100644 tools/blktap2/Makefile
 delete mode 100644 tools/blktap2/README
 delete mode 100644 tools/blktap2/control/Makefile
 delete mode 100644 tools/blktap2/control/tap-ctl-allocate.c
 delete mode 100644 tools/blktap2/control/tap-ctl-attach.c
 delete mode 100644 tools/blktap2/control/tap-ctl-check.c
 delete mode 100644 tools/blktap2/control/tap-ctl-close.c
 delete mode 100644 tools/blktap2/control/tap-ctl-create.c
 delete mode 100644 tools/blktap2/control/tap-ctl-destroy.c
 delete mode 100644 tools/blktap2/control/tap-ctl-detach.c
 delete mode 100644 tools/blktap2/control/tap-ctl-free.c
 delete mode 100644 tools/blktap2/control/tap-ctl-ipc.c
 delete mode 100644 tools/blktap2/control/tap-ctl-list.c
 delete mode 100644 tools/blktap2/control/tap-ctl-major.c
 delete mode 100644 tools/blktap2/control/tap-ctl-open.c
 delete mode 100644 tools/blktap2/control/tap-ctl-pause.c
 delete mode 100644 tools/blktap2/control/tap-ctl-spawn.c
 delete mode 100644 tools/blktap2/control/tap-ctl-unpause.c
 delete mode 100644 tools/blktap2/control/tap-ctl.c
 delete mode 100644 tools/blktap2/control/tap-ctl.h
 delete mode 100644 tools/blktap2/drivers/Makefile
 delete mode 100644 tools/blktap2/drivers/aes.c
 delete mode 100644 tools/blktap2/drivers/aes.h
 delete mode 100644 tools/blktap2/drivers/atomicio.c
 delete mode 100644 tools/blktap2/drivers/blk.h
 delete mode 100644 tools/blktap2/drivers/blk_linux.c
 delete mode 100644 tools/blktap2/drivers/blk_netbsd.c
 delete mode 100644 tools/blktap2/drivers/block-aio.c
 delete mode 100644 tools/blktap2/drivers/block-cache.c
 delete mode 100644 tools/blktap2/drivers/block-log.c
 delete mode 100644 tools/blktap2/drivers/block-qcow.c
 delete mode 100644 tools/blktap2/drivers/block-ram.c
 delete mode 100644 tools/blktap2/drivers/block-remus.c
 delete mode 100644 tools/blktap2/drivers/block-vhd.c
 delete mode 100644 tools/blktap2/drivers/bswap.h
 delete mode 100644 tools/blktap2/drivers/check_gcrypt
 delete mode 100644 tools/blktap2/drivers/hashtable.c
 delete mode 100644 tools/blktap2/drivers/hashtable.h
 delete mode 100644 tools/blktap2/drivers/hashtable_itr.c
 delete mode 100644 tools/blktap2/drivers/hashtable_itr.h
 delete mode 100644 tools/blktap2/drivers/hashtable_private.h
 delete mode 100644 tools/blktap2/drivers/hashtable_utility.c
 delete mode 100644 tools/blktap2/drivers/hashtable_utility.h
 delete mode 100644 tools/blktap2/drivers/img2qcow.c
 delete mode 100644 tools/blktap2/drivers/io-optimize.c
 delete mode 100644 tools/blktap2/drivers/io-optimize.h
 delete mode 100644 tools/blktap2/drivers/libaio-compat.h
 delete mode 100644 tools/blktap2/drivers/lock.c
 delete mode 100644 tools/blktap2/drivers/lock.h
 delete mode 100644 tools/blktap2/drivers/log.h
 delete mode 100644 tools/blktap2/drivers/md5.c
 delete mode 100644 tools/blktap2/drivers/md5.h
 delete mode 100644 tools/blktap2/drivers/profile.h
 delete mode 100644 tools/blktap2/drivers/qcow-create.c
 delete mode 100644 tools/blktap2/drivers/qcow.h
 delete mode 100644 tools/blktap2/drivers/qcow2raw.c
 delete mode 100644 tools/blktap2/drivers/scheduler.c
 delete mode 100644 tools/blktap2/drivers/scheduler.h
 delete mode 100644 tools/blktap2/drivers/tapdisk-client.c
 delete mode 100644 tools/blktap2/drivers/tapdisk-control.c
 delete mode 100644 tools/blktap2/drivers/tapdisk-control.h
 delete mode 100644 tools/blktap2/drivers/tapdisk-diff.c
 delete mode 100644 tools/blktap2/drivers/tapdisk-disktype.c
 delete mode 100644 tools/blktap2/drivers/tapdisk-disktype.h
 delete mode 100644 tools/blktap2/drivers/tapdisk-driver.c
 delete mode 100644 tools/blktap2/drivers/tapdisk-driver.h
 delete mode 100644 tools/blktap2/drivers/tapdisk-filter.c
 delete mode 100644 tools/blktap2/drivers/tapdisk-filter.h
 delete mode 100644 tools/blktap2/drivers/tapdisk-image.c
 delete mode 100644 tools/blktap2/drivers/tapdisk-image.h
 delete mode 100644 tools/blktap2/drivers/tapdisk-interface.c
 delete mode 100644 tools/blktap2/drivers/tapdisk-interface.h
 delete mode 100644 tools/blktap2/drivers/tapdisk-log.c
 delete mode 100644 tools/blktap2/drivers/tapdisk-log.h
 delete mode 100644 tools/blktap2/drivers/tapdisk-queue.c
 delete mode 100644 tools/blktap2/drivers/tapdisk-queue.h
 delete mode 100644 tools/blktap2/drivers/tapdisk-ring.c
 delete mode 100644 tools/blktap2/drivers/tapdisk-ring.h
 delete mode 100644 tools/blktap2/drivers/tapdisk-server.c
 delete mode 100644 tools/blktap2/drivers/tapdisk-server.h
 delete mode 100644 tools/blktap2/drivers/tapdisk-stream.c
 delete mode 100644 tools/blktap2/drivers/tapdisk-utils.c
 delete mode 100644 tools/blktap2/drivers/tapdisk-utils.h
 delete mode 100644 tools/blktap2/drivers/tapdisk-vbd.c
 delete mode 100644 tools/blktap2/drivers/tapdisk-vbd.h
 delete mode 100644 tools/blktap2/drivers/tapdisk.h
 delete mode 100644 tools/blktap2/drivers/tapdisk2.c
 delete mode 100644 tools/blktap2/drivers/td.c
 delete mode 100644 tools/blktap2/drivers/xmsnap
 delete mode 100644 tools/blktap2/include/Makefile
 delete mode 100644 tools/blktap2/include/atomicio.h
 delete mode 100644 tools/blktap2/include/blktap2.h
 delete mode 100644 tools/blktap2/include/blktaplib.h
 delete mode 100644 tools/blktap2/include/libvhd-journal.h
 delete mode 100644 tools/blktap2/include/libvhd.h
 delete mode 100644 tools/blktap2/include/list.h
 delete mode 100644 tools/blktap2/include/lvm-util.h
 delete mode 100644 tools/blktap2/include/relative-path.h
 delete mode 100644 tools/blktap2/include/tapdisk-message.h
 delete mode 100644 tools/blktap2/include/vhd-util.h
 delete mode 100644 tools/blktap2/include/vhd-uuid.h
 delete mode 100644 tools/blktap2/include/vhd.h
 delete mode 100644 tools/blktap2/lvm/Makefile
 delete mode 100644 tools/blktap2/lvm/lvm-util.c
 delete mode 100644 tools/blktap2/vhd/Makefile
 delete mode 100644 tools/blktap2/vhd/lib/Makefile
 delete mode 100644 tools/blktap2/vhd/lib/atomicio.c
 delete mode 100644 tools/blktap2/vhd/lib/libvhd-journal.c
 delete mode 100644 tools/blktap2/vhd/lib/libvhd.c
 delete mode 100644 tools/blktap2/vhd/lib/relative-path.c
 delete mode 100644 tools/blktap2/vhd/lib/vhd-util-check.c
 delete mode 100644 tools/blktap2/vhd/lib/vhd-util-coalesce.c
 delete mode 100644 tools/blktap2/vhd/lib/vhd-util-create.c
 delete mode 100644 tools/blktap2/vhd/lib/vhd-util-fill.c
 delete mode 100644 tools/blktap2/vhd/lib/vhd-util-modify.c
 delete mode 100644 tools/blktap2/vhd/lib/vhd-util-query.c
 delete mode 100644 tools/blktap2/vhd/lib/vhd-util-read.c
 delete mode 100644 tools/blktap2/vhd/lib/vhd-util-repair.c
 delete mode 100644 tools/blktap2/vhd/lib/vhd-util-resize.c
 delete mode 100644 tools/blktap2/vhd/lib/vhd-util-revert.c
 delete mode 100644 tools/blktap2/vhd/lib/vhd-util-scan.c
 delete mode 100644 tools/blktap2/vhd/lib/vhd-util-set-field.c
 delete mode 100644 tools/blktap2/vhd/lib/vhd-util-snapshot.c
 delete mode 100644 tools/blktap2/vhd/lib/vhd-util-uuid.c
 delete mode 100644 tools/blktap2/vhd/vhd-update.c
 delete mode 100644 tools/blktap2/vhd/vhd-util.c

[REMOVED]

Re: [PATCH RFC 2/4] build: Download and build extnernal blktap2.5 repo

[Ian Campbell]

On Thu, 2015-03-26 at 12:46 +0000, George Dunlap wrote:

Typo in $subject.

> FIXME: Directly use the XenServer github repo for now, while we're
> discussing things.  If we decide to take this series, we'll have to
> clone the tree on xenbits and remove the FIXME line.

We would also need to decide whether to have a push gate or not (I think
we should) and whether to track a branch automatically or manually
update the revision (I don't know, depends). Either way I think osstest
needs to learn a bit about blktap2.

Overall I'm in favour of the general idea of this change though.

Ian.

Re: [PATCH RFC 0/4] tools: Upstream blktap "2.5" as an external repo

[George Dunlap]

On Thu, Mar 26, 2015 at 12:46 PM, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> For some time, the blktap2 in-tree has bitrotted.  Many years ago the
> XenServer team at Citrix forked the code into a separate repository;
> several attempts have been made to upstream those changes back into
> Xen, to no avail.
>
> The blktap code at the moment is the only source of performant vhd
> format integration.  It's additionally in use by projects like the
> COLO project.
>
> This patch series removes the in-tree blktap2 code and treats the
> XenServer blktap tree as an upstream.  I've gotten agreement from the
> XenServer team to act as an upstream -- to accept patches fixing bugs,
> to help track down errors, and to attempt to help fix build breakages
> introduced by development.

That is to say -- I think I remember getting such agreement -- please
do contradict me if I'm wrong. :-)

 -George

Re: [PATCH RFC 2/4] build: Download and build extnernal blktap2.5 repo

[George Dunlap]

On Thu, Mar 26, 2015 at 12:55 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Thu, 2015-03-26 at 12:46 +0000, George Dunlap wrote:
>
> Typo in $subject.
>
>> FIXME: Directly use the XenServer github repo for now, while we're
>> discussing things.  If we decide to take this series, we'll have to
>> clone the tree on xenbits and remove the FIXME line.
>
> We would also need to decide whether to have a push gate or not (I think
> we should) and whether to track a branch automatically or manually
> update the revision (I don't know, depends).

I think ideally we'd track the "blktap2" branch during normal
development, and grab a specific commit ID when we go into feature
freeze.  But at the moment, that only works if I put "origin/blktap2"
in the REVISION, which doesn't seem quite right.  On my to-do list is
making the git checkout capable of looking for remote branches.
(AFAICT "master" works because a local branch called "master" is made
by default on checkout.)

 -George

Re: [PATCH RFC 0/4] tools: Upstream blktap "2.5" as an external repo

[Jon Ludlam]

On Thu, Mar 26, 2015 at 12:46:06PM +0000, George Dunlap wrote:
> For some time, the blktap2 in-tree has bitrotted.  Many years ago the
> XenServer team at Citrix forked the code into a separate repository;
> several attempts have been made to upstream those changes back into
> Xen, to no avail.
> 
> The blktap code at the moment is the only source of performant vhd
> format integration.  It's additionally in use by projects like the
> COLO project.
> 
> This patch series removes the in-tree blktap2 code and treats the
> XenServer blktap tree as an upstream.  I've gotten agreement from the
> XenServer team to act as an upstream -- to accept patches fixing bugs,
> to help track down errors, and to attempt to help fix build breakages
> introduced by development.
> 
> At the moment we're using the "blktap2" branch of XenServer's
> blktap.git.  (This has been sometimes known as "blktap 2.5".)  This
> branch is maintianed in order to provide a buildroot for OpenStack,
> and has also been used by the CentOS xen packages for several years
> now.

It's probably worth mentioning again that there is a kernel patch
required. Some years ago I did some work to make the patch into a dkms
module, but since then the patch and the kernel have moved on and I
couldn't quite make it work any more; I'm afraid my kernel knowledge
is a bit lacking.

The current patches used in XenServer are on github here:
https://github.com/xenserver/linux-3.x.pg/tree/master/master

and the old dkms code is here:
https://github.com/xapi-project/blktap-dkms

In case anyone is interested...!

Jon

Re: [PATCH RFC 0/4] tools: Upstream blktap "2.5" as an external repo

[George Dunlap]

On 03/26/2015 03:47 PM, Jon Ludlam wrote:
> On Thu, Mar 26, 2015 at 12:46:06PM +0000, George Dunlap wrote:
>> For some time, the blktap2 in-tree has bitrotted.  Many years ago the
>> XenServer team at Citrix forked the code into a separate repository;
>> several attempts have been made to upstream those changes back into
>> Xen, to no avail.
>>
>> The blktap code at the moment is the only source of performant vhd
>> format integration.  It's additionally in use by projects like the
>> COLO project.
>>
>> This patch series removes the in-tree blktap2 code and treats the
>> XenServer blktap tree as an upstream.  I've gotten agreement from the
>> XenServer team to act as an upstream -- to accept patches fixing bugs,
>> to help track down errors, and to attempt to help fix build breakages
>> introduced by development.
>>
>> At the moment we're using the "blktap2" branch of XenServer's
>> blktap.git.  (This has been sometimes known as "blktap 2.5".)  This
>> branch is maintianed in order to provide a buildroot for OpenStack,
>> and has also been used by the CentOS xen packages for several years
>> now.
> 
> It's probably worth mentioning again that there is a kernel patch
> required. Some years ago I did some work to make the patch into a dkms
> module, but since then the patch and the kernel have moved on and I
> couldn't quite make it work any more; I'm afraid my kernel knowledge
> is a bit lacking.
> 
> The current patches used in XenServer are on github here:
> https://github.com/xenserver/linux-3.x.pg/tree/master/master
> 
> and the old dkms code is here:
> https://github.com/xapi-project/blktap-dkms
> 
> In case anyone is interested...!

Another thing I'd like to explore (since this took all of about an
afternoon to get working) is what it would take to switch to using
blktap3 instead.  As I understand from my conversations with the
XenServer team, they use a kernel module in XenServer when mounting an
image in dom0 for scalability reasons; but there's no reason XenProject
need to do the same thing.

I'd probably need some guidance from the XenServer team about an
appropriate way to start that. :-)

 -George

Re: [PATCH RFC 0/4] tools: Upstream blktap "2.5" as an external repo

[Felipe Franciosi]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of George Dunlap
> Sent: 26 March 2015 16:09
> To: Jonathan Ludlam
> Cc: Wei Liu; Dave Scott; Wen Congyang; Jonathan Ludlam; xen-
> devel@lists.xen.org; Ian Jackson; Yang Hongyang; Ian Campbell
> Subject: Re: [Xen-devel] [PATCH RFC 0/4] tools: Upstream blktap "2.5" as an
> external repo
> 
> On 03/26/2015 03:47 PM, Jon Ludlam wrote:
> > On Thu, Mar 26, 2015 at 12:46:06PM +0000, George Dunlap wrote:
> >> For some time, the blktap2 in-tree has bitrotted.  Many years ago the
> >> XenServer team at Citrix forked the code into a separate repository;
> >> several attempts have been made to upstream those changes back into
> >> Xen, to no avail.
> >>
> >> The blktap code at the moment is the only source of performant vhd
> >> format integration.  It's additionally in use by projects like the
> >> COLO project.
> >>
> >> This patch series removes the in-tree blktap2 code and treats the
> >> XenServer blktap tree as an upstream.  I've gotten agreement from the
> >> XenServer team to act as an upstream -- to accept patches fixing
> >> bugs, to help track down errors, and to attempt to help fix build
> >> breakages introduced by development.
> >>
> >> At the moment we're using the "blktap2" branch of XenServer's
> >> blktap.git.  (This has been sometimes known as "blktap 2.5".)  This
> >> branch is maintianed in order to provide a buildroot for OpenStack,
> >> and has also been used by the CentOS xen packages for several years
> >> now.
> >
> > It's probably worth mentioning again that there is a kernel patch
> > required. Some years ago I did some work to make the patch into a dkms
> > module, but since then the patch and the kernel have moved on and I
> > couldn't quite make it work any more; I'm afraid my kernel knowledge
> > is a bit lacking.
> >
> > The current patches used in XenServer are on github here:
> > https://github.com/xenserver/linux-3.x.pg/tree/master/master
> >
> > and the old dkms code is here:
> > https://github.com/xapi-project/blktap-dkms
> >
> > In case anyone is interested...!
> 
> Another thing I'd like to explore (since this took all of about an afternoon to get
> working) is what it would take to switch to using
> blktap3 instead.  As I understand from my conversations with the XenServer
> team, they use a kernel module in XenServer when mounting an image in dom0
> for scalability reasons; but there's no reason XenProject need to do the same
> thing.

Today, to access virtual disks in dom0 we use the blktap2 kernel module that Jonathan Ludlam mentioned. This provides a block device (in /dev/xen/blktap-2/tapdev<minor>). In the (somewhat distant) past, we actually had blkfront in dom0.

> I'd probably need some guidance from the XenServer team about an
> appropriate way to start that. :-)

What is also needed to get blktap3 working is a gntdev supporting grant copy. I believe this is the order they are applied in 3.10:
https://github.com/xenserver/linux-3.x.pg/blob/master/master/0001-xen-install-xen-gntdev.h-and-xen-gntalloc.h.patch
https://github.com/xenserver/linux-3.x.pg/blob/master/master/xen-gntdev-grant-copy.patch
https://github.com/xenserver/linux-3.x.pg/blob/master/master/0002-xen-gntdev-mark-userspace-PTEs-as-special-on-x86-PV-.patch
https://github.com/xenserver/linux-3.x.pg/blob/master/master/0003-xen-gntdev-provide-a-set-of-pages-for-the-VMA.patch

The _main_ difference between tapdisk2 and 3 is that tapdisk3 can connect directly to blkfront. It does all I/O via grant copies which has some implications in the way memory is handled in dom0.

Cheers,
Felipe



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

Re: [PATCH RFC 0/4] tools: Upstream blktap "2.5" as an external repo

[Jon Ludlam]

On Thu, Mar 26, 2015 at 04:08:41PM +0000, George Dunlap wrote:
> On 03/26/2015 03:47 PM, Jon Ludlam wrote:
> > On Thu, Mar 26, 2015 at 12:46:06PM +0000, George Dunlap wrote:
> >> For some time, the blktap2 in-tree has bitrotted.  Many years ago the
> >> XenServer team at Citrix forked the code into a separate repository;
> >> several attempts have been made to upstream those changes back into
> >> Xen, to no avail.
> >>
> >> The blktap code at the moment is the only source of performant vhd
> >> format integration.  It's additionally in use by projects like the
> >> COLO project.
> >>
> >> This patch series removes the in-tree blktap2 code and treats the
> >> XenServer blktap tree as an upstream.  I've gotten agreement from the
> >> XenServer team to act as an upstream -- to accept patches fixing bugs,
> >> to help track down errors, and to attempt to help fix build breakages
> >> introduced by development.
> >>
> >> At the moment we're using the "blktap2" branch of XenServer's
> >> blktap.git.  (This has been sometimes known as "blktap 2.5".)  This
> >> branch is maintianed in order to provide a buildroot for OpenStack,
> >> and has also been used by the CentOS xen packages for several years
> >> now.
> > 
> > It's probably worth mentioning again that there is a kernel patch
> > required. Some years ago I did some work to make the patch into a dkms
> > module, but since then the patch and the kernel have moved on and I
> > couldn't quite make it work any more; I'm afraid my kernel knowledge
> > is a bit lacking.
> > 
> > The current patches used in XenServer are on github here:
> > https://github.com/xenserver/linux-3.x.pg/tree/master/master
> > 
> > and the old dkms code is here:
> > https://github.com/xapi-project/blktap-dkms
> > 
> > In case anyone is interested...!
> 
> Another thing I'd like to explore (since this took all of about an
> afternoon to get working) is what it would take to switch to using
> blktap3 instead.  As I understand from my conversations with the
> XenServer team, they use a kernel module in XenServer when mounting an
> image in dom0 for scalability reasons; but there's no reason XenProject
> need to do the same thing.
> 
> I'd probably need some guidance from the XenServer team about an
> appropriate way to start that. :-)
>

Good question. The right people to speak to are probably Germano and/or
Felipe (cc'd)

Jon


>  -George

Re: [PATCH RFC 0/4] tools: Upstream blktap "2.5" as an external repo

[George Dunlap]

On 03/26/2015 04:25 PM, Felipe Franciosi wrote:
>> Another thing I'd like to explore (since this took all of about an afternoon to get
>> working) is what it would take to switch to using
>> blktap3 instead.  As I understand from my conversations with the XenServer
>> team, they use a kernel module in XenServer when mounting an image in dom0
>> for scalability reasons; but there's no reason XenProject need to do the same
>> thing.
> 
> Today, to access virtual disks in dom0 we use the blktap2 kernel module that Jonathan Ludlam mentioned. This provides a block device (in /dev/xen/blktap-2/tapdev<minor>). In the (somewhat distant) past, we actually had blkfront in dom0.

OK -- so the XenServer blktap2 kernel module is definitely being
maintained.  That's good to know.

> What is also needed to get blktap3 working is a gntdev supporting grant copy. I believe this is the order they are applied in 3.10:
> https://github.com/xenserver/linux-3.x.pg/blob/master/master/0001-xen-install-xen-gntdev.h-and-xen-gntalloc.h.patch
> https://github.com/xenserver/linux-3.x.pg/blob/master/master/xen-gntdev-grant-copy.patch
> https://github.com/xenserver/linux-3.x.pg/blob/master/master/0002-xen-gntdev-mark-userspace-PTEs-as-special-on-x86-PV-.patch
> https://github.com/xenserver/linux-3.x.pg/blob/master/master/0003-xen-gntdev-provide-a-set-of-pages-for-the-VMA.patch
> 
> The _main_ difference between tapdisk2 and 3 is that tapdisk3 can connect directly to blkfront. It does all I/O via grant copies which has some implications in the way memory is handled in dom0.

Right, I didn't realize blktap3 also required kernel changes.  Are those
reasonably upstream-able, and is there any plan by the XenServer team to
upstream them?

 -George

Re: [PATCH RFC 0/4] tools: Upstream blktap "2.5" as an external repo

[Andrew Cooper]

On 26/03/15 17:05, George Dunlap wrote:
> On 03/26/2015 04:25 PM, Felipe Franciosi wrote:
>>> Another thing I'd like to explore (since this took all of about an afternoon to get
>>> working) is what it would take to switch to using
>>> blktap3 instead.  As I understand from my conversations with the XenServer
>>> team, they use a kernel module in XenServer when mounting an image in dom0
>>> for scalability reasons; but there's no reason XenProject need to do the same
>>> thing.
>> Today, to access virtual disks in dom0 we use the blktap2 kernel module that Jonathan Ludlam mentioned. This provides a block device (in /dev/xen/blktap-2/tapdev<minor>). In the (somewhat distant) past, we actually had blkfront in dom0.
> OK -- so the XenServer blktap2 kernel module is definitely being
> maintained.  That's good to know.
>
>> What is also needed to get blktap3 working is a gntdev supporting grant copy. I believe this is the order they are applied in 3.10:
>> https://github.com/xenserver/linux-3.x.pg/blob/master/master/0001-xen-install-xen-gntdev.h-and-xen-gntalloc.h.patch
>> https://github.com/xenserver/linux-3.x.pg/blob/master/master/xen-gntdev-grant-copy.patch
>> https://github.com/xenserver/linux-3.x.pg/blob/master/master/0002-xen-gntdev-mark-userspace-PTEs-as-special-on-x86-PV-.patch
>> https://github.com/xenserver/linux-3.x.pg/blob/master/master/0003-xen-gntdev-provide-a-set-of-pages-for-the-VMA.patch
>>
>> The _main_ difference between tapdisk2 and 3 is that tapdisk3 can connect directly to blkfront. It does all I/O via grant copies which has some implications in the way memory is handled in dom0.
> Right, I didn't realize blktap3 also required kernel changes.  Are those
> reasonably upstream-able, and is there any plan by the XenServer team to
> upstream them?

They are all in Linux 3.19 iirc.  These are not patches for blktap3 
specially; they are all patches fixing the use of userspace grant maps 
with /dev/xen/gnttab, which never functioned correctly in the past.

~Andrew

Re: [PATCH RFC 0/4] tools: Upstream blktap "2.5" as an external repo

[George Dunlap]

On 03/26/2015 05:12 PM, Andrew Cooper wrote:
> On 26/03/15 17:05, George Dunlap wrote:
>> On 03/26/2015 04:25 PM, Felipe Franciosi wrote:
>>>> Another thing I'd like to explore (since this took all of about an
>>>> afternoon to get
>>>> working) is what it would take to switch to using
>>>> blktap3 instead.  As I understand from my conversations with the
>>>> XenServer
>>>> team, they use a kernel module in XenServer when mounting an image
>>>> in dom0
>>>> for scalability reasons; but there's no reason XenProject need to do
>>>> the same
>>>> thing.
>>> Today, to access virtual disks in dom0 we use the blktap2 kernel
>>> module that Jonathan Ludlam mentioned. This provides a block device
>>> (in /dev/xen/blktap-2/tapdev<minor>). In the (somewhat distant) past,
>>> we actually had blkfront in dom0.
>> OK -- so the XenServer blktap2 kernel module is definitely being
>> maintained.  That's good to know.
>>
>>> What is also needed to get blktap3 working is a gntdev supporting
>>> grant copy. I believe this is the order they are applied in 3.10:
>>> https://github.com/xenserver/linux-3.x.pg/blob/master/master/0001-xen-install-xen-gntdev.h-and-xen-gntalloc.h.patch
>>>
>>> https://github.com/xenserver/linux-3.x.pg/blob/master/master/xen-gntdev-grant-copy.patch
>>>
>>> https://github.com/xenserver/linux-3.x.pg/blob/master/master/0002-xen-gntdev-mark-userspace-PTEs-as-special-on-x86-PV-.patch
>>>
>>> https://github.com/xenserver/linux-3.x.pg/blob/master/master/0003-xen-gntdev-provide-a-set-of-pages-for-the-VMA.patch
>>>
>>>
>>> The _main_ difference between tapdisk2 and 3 is that tapdisk3 can
>>> connect directly to blkfront. It does all I/O via grant copies which
>>> has some implications in the way memory is handled in dom0.
>> Right, I didn't realize blktap3 also required kernel changes.  Are those
>> reasonably upstream-able, and is there any plan by the XenServer team to
>> upstream them?
> 
> They are all in Linux 3.19 iirc.  These are not patches for blktap3
> specially; they are all patches fixing the use of userspace grant maps
> with /dev/xen/gnttab, which never functioned correctly in the past.

OK, good.  So with my XenProject hat on, nothing needs to be done per se
to use blktap3; we just need to tell people to use linux >= 3.19.  (And
make sure there's a robust way to detect the necessary kernel support
before trying to use blktap3.)

With my CentOS hat on, if I want to use an older version of Linux, I
need to import / port those patches to whatever version I'm using; but
that's not directly relevant to this discussion.

 -George

Re: [PATCH RFC 0/4] tools: Upstream blktap "2.5" as an external repo

[Felipe Franciosi]

> -----Original Message-----
> From: George Dunlap [mailto:george.dunlap@eu.citrix.com]
> Sent: 26 March 2015 17:23
> To: Andrew Cooper; Felipe Franciosi; George Dunlap; Jonathan Ludlam
> Cc: Dave Scott; Wei Liu; Ian Campbell; Wen Congyang; xen-devel@lists.xen.org;
> Ian Jackson; Yang Hongyang
> Subject: Re: [Xen-devel] [PATCH RFC 0/4] tools: Upstream blktap "2.5" as an
> external repo
> 
> On 03/26/2015 05:12 PM, Andrew Cooper wrote:
> > On 26/03/15 17:05, George Dunlap wrote:
> >> On 03/26/2015 04:25 PM, Felipe Franciosi wrote:
> >>>> Another thing I'd like to explore (since this took all of about an
> >>>> afternoon to get
> >>>> working) is what it would take to switch to using
> >>>> blktap3 instead.  As I understand from my conversations with the
> >>>> XenServer team, they use a kernel module in XenServer when mounting
> >>>> an image in dom0 for scalability reasons; but there's no reason
> >>>> XenProject need to do the same thing.
> >>> Today, to access virtual disks in dom0 we use the blktap2 kernel
> >>> module that Jonathan Ludlam mentioned. This provides a block device
> >>> (in /dev/xen/blktap-2/tapdev<minor>). In the (somewhat distant)
> >>> past, we actually had blkfront in dom0.
> >> OK -- so the XenServer blktap2 kernel module is definitely being
> >> maintained.  That's good to know.
> >>
> >>> What is also needed to get blktap3 working is a gntdev supporting
> >>> grant copy. I believe this is the order they are applied in 3.10:
> >>> https://github.com/xenserver/linux-3.x.pg/blob/master/master/0001-xe
> >>> n-install-xen-gntdev.h-and-xen-gntalloc.h.patch
> >>>
> >>> https://github.com/xenserver/linux-3.x.pg/blob/master/master/xen-gnt
> >>> dev-grant-copy.patch
> >>>
> >>> https://github.com/xenserver/linux-3.x.pg/blob/master/master/0002-xe
> >>> n-gntdev-mark-userspace-PTEs-as-special-on-x86-PV-.patch
> >>>
> >>> https://github.com/xenserver/linux-3.x.pg/blob/master/master/0003-xe
> >>> n-gntdev-provide-a-set-of-pages-for-the-VMA.patch
> >>>
> >>>
> >>> The _main_ difference between tapdisk2 and 3 is that tapdisk3 can
> >>> connect directly to blkfront. It does all I/O via grant copies which
> >>> has some implications in the way memory is handled in dom0.
> >> Right, I didn't realize blktap3 also required kernel changes.  Are
> >> those reasonably upstream-able, and is there any plan by the
> >> XenServer team to upstream them?
> >
> > They are all in Linux 3.19 iirc.  These are not patches for blktap3
> > specially; they are all patches fixing the use of userspace grant maps
> > with /dev/xen/gnttab, which never functioned correctly in the past.

I don't think they are all in 3.19. I just had a quick look and our gntdev.c looks very different than upstream's.

The grant-copy patch is definitely missing on Linux 4.0-rc5.

Cheers,
F.

> 
> OK, good.  So with my XenProject hat on, nothing needs to be done per se to
> use blktap3; we just need to tell people to use linux >= 3.19.  (And make sure
> there's a robust way to detect the necessary kernel support before trying to use
> blktap3.)
> 
> With my CentOS hat on, if I want to use an older version of Linux, I need to
> import / port those patches to whatever version I'm using; but that's not
> directly relevant to this discussion.
> 
>  -George

Re: [PATCH RFC 1/4] build: Reorganize and briefly document "external repo" template in tools/Makefile

[Ian Campbell]

On Thu, 2015-03-26 at 12:46 +0000, George Dunlap wrote:
> No functional changes.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Although the series as a whole is RFC I can't see any reason to hold
back on this particular improvement, so Acked + applied.

Re: [PATCH RFC 2/4] build: Download and build extnernal blktap2.5 repo

[Wei Liu]

On Thu, Mar 26, 2015 at 12:46:08PM +0000, George Dunlap wrote:
> Download and build XenServer's blktap as an external tree, similar to
> qemu-xen.
> 
> As of this patch we just download and build it, but don't install or
> use it.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> 
> FIXME: Directly use the XenServer github repo for now, while we're
> discussing things.  If we decide to take this series, we'll have to
> clone the tree on xenbits and remove the FIXME line.
> 

IMHO we need to support --with-system-blktap= in configure in case
distro wants to package blktap separately. Not sure if in practice this
makes sense since AIUI blktap is only used by Xen.

Wei.

Re: [PATCH RFC 2/4] build: Download and build extnernal blktap2.5 repo

[Ian Campbell]

On Mon, 2015-03-30 at 15:33 +0100, Wei Liu wrote:
> On Thu, Mar 26, 2015 at 12:46:08PM +0000, George Dunlap wrote:
> > Download and build XenServer's blktap as an external tree, similar to
> > qemu-xen.
> > 
> > As of this patch we just download and build it, but don't install or
> > use it.
> > 
> > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > ---
> > 
> > FIXME: Directly use the XenServer github repo for now, while we're
> > discussing things.  If we decide to take this series, we'll have to
> > clone the tree on xenbits and remove the FIXME line.
> > 
> 
> IMHO we need to support --with-system-blktap= in configure in case
> distro wants to package blktap separately. Not sure if in practice this
> makes sense since AIUI blktap is only used by Xen.

I think you are right, there are at least some distros which ship a
blktap package not from Xen. e.g. Debian ship, or have in the past
shipped, the blktap2.5 packages.

blktap is a bit different to some of the other similar cases though in
that there is a circular dependency, although George indicated last week
that he thought that might be an easily fixed one.

Ian.

Re: [PATCH RFC 2/4] build: Download and build extnernal blktap2.5 repo

[George Dunlap]

On 03/30/2015 03:33 PM, Wei Liu wrote:
> On Thu, Mar 26, 2015 at 12:46:08PM +0000, George Dunlap wrote:
>> Download and build XenServer's blktap as an external tree, similar to
>> qemu-xen.
>>
>> As of this patch we just download and build it, but don't install or
>> use it.
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>> ---
>>
>> FIXME: Directly use the XenServer github repo for now, while we're
>> discussing things.  If we decide to take this series, we'll have to
>> clone the tree on xenbits and remove the FIXME line.
>>
> 
> IMHO we need to support --with-system-blktap= in configure in case
> distro wants to package blktap separately. Not sure if in practice this
> makes sense since AIUI blktap is only used by Xen.

I agree that would be ideal; however, it's not so simple, because at the
moment libxl links directly against libblktap.  This would mean:

1) Changing libxl so that it could dynamically detect the presence of
the proper version of libblktap at runtime and use the stubbed-out
defaults if not available.

2) Changing libxl to exec() tapctl rather than making library calls.
(This might involve making sure that tapctl actually has all the
functionality we need.)

#1 is even more difficult because at the moment blktap isn't really
designed to have stable external versions -- it would involve adding a
library version and all the fun stuff we already do with libxl.

#2 has another potential advantage: using the command-line may make it
easier to be forward-compatible, since adding options won't break
(unlike adding arguments to a function signature).

(Although, I think for API compatibility, changing the public functions
to use structs is probably a more reliable thing to do.)

Either way, I think that's something we should start to look at after
pivoting to the external blktap repo.

 -George

Re: [PATCH RFC 2/4] build: Download and build extnernal blktap2.5 repo

[George Dunlap]

On 03/30/2015 03:41 PM, Ian Campbell wrote:
> On Mon, 2015-03-30 at 15:33 +0100, Wei Liu wrote:
>> On Thu, Mar 26, 2015 at 12:46:08PM +0000, George Dunlap wrote:
>>> Download and build XenServer's blktap as an external tree, similar to
>>> qemu-xen.
>>>
>>> As of this patch we just download and build it, but don't install or
>>> use it.
>>>
>>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>>> ---
>>>
>>> FIXME: Directly use the XenServer github repo for now, while we're
>>> discussing things.  If we decide to take this series, we'll have to
>>> clone the tree on xenbits and remove the FIXME line.
>>>
>>
>> IMHO we need to support --with-system-blktap= in configure in case
>> distro wants to package blktap separately. Not sure if in practice this
>> makes sense since AIUI blktap is only used by Xen.
> 
> I think you are right, there are at least some distros which ship a
> blktap package not from Xen. e.g. Debian ship, or have in the past
> shipped, the blktap2.5 packages.

Bob has said that Fedora ship a blktap2.5 package that can't actually be
installed at the same time as the Fedora xen packages, making it
somewhat useless...

> blktap is a bit different to some of the other similar cases though in
> that there is a circular dependency, although George indicated last week
> that he thought that might be an easily fixed one.

I suspect this has probably already been fixed in blktap2.5, since (I'm
pretty sure) XenServer ship blktap and the things that depend on it
(like xen and xapi) in separate rpms; but it's certainly something we'd
have to look at.

 -George

Re: [PATCH RFC 2/4] build: Download and build extnernal blktap2.5 repo

[Andrew Cooper]

On 30/03/15 15:41, Ian Campbell wrote:
> On Mon, 2015-03-30 at 15:33 +0100, Wei Liu wrote:
>> On Thu, Mar 26, 2015 at 12:46:08PM +0000, George Dunlap wrote:
>>> Download and build XenServer's blktap as an external tree, similar to
>>> qemu-xen.
>>>
>>> As of this patch we just download and build it, but don't install or
>>> use it.
>>>
>>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>>> ---
>>>
>>> FIXME: Directly use the XenServer github repo for now, while we're
>>> discussing things.  If we decide to take this series, we'll have to
>>> clone the tree on xenbits and remove the FIXME line.
>>>
>> IMHO we need to support --with-system-blktap= in configure in case
>> distro wants to package blktap separately. Not sure if in practice this
>> makes sense since AIUI blktap is only used by Xen.
> I think you are right, there are at least some distros which ship a
> blktap package not from Xen. e.g. Debian ship, or have in the past
> shipped, the blktap2.5 packages.
>
> blktap is a bit different to some of the other similar cases though in
> that there is a circular dependency, although George indicated last week
> that he thought that might be an easily fixed one.

I seem to recall that some bits of blktap need xen-devel (depending on
exactly what is built).

libxl will need blktap-devel, so these dependences can be resolved.

~Andrew

Re: [PATCH RFC 2/4] build: Download and build extnernal blktap2.5 repo

[Wei Liu]

On Mon, Mar 30, 2015 at 03:43:07PM +0100, George Dunlap wrote:
> On 03/30/2015 03:33 PM, Wei Liu wrote:
> > On Thu, Mar 26, 2015 at 12:46:08PM +0000, George Dunlap wrote:
> >> Download and build XenServer's blktap as an external tree, similar to
> >> qemu-xen.
> >>
> >> As of this patch we just download and build it, but don't install or
> >> use it.
> >>
> >> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> >> ---
> >>
> >> FIXME: Directly use the XenServer github repo for now, while we're
> >> discussing things.  If we decide to take this series, we'll have to
> >> clone the tree on xenbits and remove the FIXME line.
> >>
> > 
> > IMHO we need to support --with-system-blktap= in configure in case
> > distro wants to package blktap separately. Not sure if in practice this
> > makes sense since AIUI blktap is only used by Xen.
> 
> I agree that would be ideal; however, it's not so simple, because at the
> moment libxl links directly against libblktap.  This would mean:
> 
> 1) Changing libxl so that it could dynamically detect the presence of
> the proper version of libblktap at runtime and use the stubbed-out
> defaults if not available.
> 

This should be done in ./configure too, not during libxl build /
runtime.  If libblktap is not present during ./configure  then libxl
just use stubs.

> 2) Changing libxl to exec() tapctl rather than making library calls.
> (This might involve making sure that tapctl actually has all the
> functionality we need.)
> 

There is no need to change to exec() at runtime. See above.

> #1 is even more difficult because at the moment blktap isn't really
> designed to have stable external versions -- it would involve adding a
> library version and all the fun stuff we already do with libxl.
> 

It blktap is to be maintained as external project to get wider usage
outside of Xen then this is a thing they have to do, isn't it?
Otherwise this is just an effort to separate blktap to an external tree,
Xen is still coupled with a certain blktap changeset. Not saying this
itself is a problem but it would be better to clarify what's the future
expectation of blktap as a project.

> #2 has another potential advantage: using the command-line may make it
> easier to be forward-compatible, since adding options won't break
> (unlike adding arguments to a function signature).
> 
> (Although, I think for API compatibility, changing the public functions
> to use structs is probably a more reliable thing to do.)
> 
> Either way, I think that's something we should start to look at after
> pivoting to the external blktap repo.
> 

Agreed. I don't mean to have --with-system-blktap= as prerequisite of
this series being accepted.

Wei.

>  -George

Re: [PATCH RFC 2/4] build: Download and build extnernal blktap2.5 repo

[George Dunlap]

On Mon, Mar 30, 2015 at 4:38 PM, Wei Liu <wei.liu2@citrix.com> wrote:
>> > IMHO we need to support --with-system-blktap= in configure in case
>> > distro wants to package blktap separately. Not sure if in practice this
>> > makes sense since AIUI blktap is only used by Xen.
>>
>> I agree that would be ideal; however, it's not so simple, because at the
>> moment libxl links directly against libblktap.  This would mean:
>>
>> 1) Changing libxl so that it could dynamically detect the presence of
>> the proper version of libblktap at runtime and use the stubbed-out
>> defaults if not available.
>>
>
> This should be done in ./configure too, not during libxl build /
> runtime.  If libblktap is not present during ./configure  then libxl
> just use stubs.

It sounds like you're talking about introducing a hard dependency,
such that packages that use a libxl built this way won't function
without blktap installed.  Yeah, that's simple enough.

I'm not super experienced in the distro packaging mindset, but since
(AFAIK) no other programs or projects use blktap, is there much point
to having a separate repo if you can't "opt-out" of installing it?

>> #1 is even more difficult because at the moment blktap isn't really
>> designed to have stable external versions -- it would involve adding a
>> library version and all the fun stuff we already do with libxl.
>>
>
> It blktap is to be maintained as external project to get wider usage
> outside of Xen then this is a thing they have to do, isn't it?
> Otherwise this is just an effort to separate blktap to an external tree,
> Xen is still coupled with a certain blktap changeset. Not saying this
> itself is a problem but it would be better to clarify what's the future
> expectation of blktap as a project.

My understanding was:

1. The XenServer team develop and maintain blktap primarily for their
own product.

2. They don't have a particular desire for blktap to get wider usage.
It's actually other projects -- CentOS and COLO in particular -- that
want to be able to use a maintained version of blktap.

3. The XenServer developers are, as individuals, happy to have other
people using it; and are willing to make small amounts of efforts to
be an "upstream" -- namely, accepting bug fixes and reviewing minor
contributions.

4. However, the XenServer team is primarily focused on building their
own product. They won't be able to spend a significant amount of time
making blktap more "community-friendly".  Adding features that are
useful to other downstreams but not to XenServer will probably be
accepted, but I wouldn't be surprised if large architectural changes
which help downstreams (such as COLO) but don't have any benefit for
XenServer were rejected.

I figured it was worth giving it a try.  If things don't work out,
then we can either fork (if someone is willing to maintain it), or
remove blktap support entirely.

 -George

Re: [PATCH RFC 2/4] build: Download and build extnernal blktap2.5 repo

[Stefano Stabellini]

On Tue, 31 Mar 2015, George Dunlap wrote:
> On Mon, Mar 30, 2015 at 4:38 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> >> > IMHO we need to support --with-system-blktap= in configure in case
> >> > distro wants to package blktap separately. Not sure if in practice this
> >> > makes sense since AIUI blktap is only used by Xen.
> >>
> >> I agree that would be ideal; however, it's not so simple, because at the
> >> moment libxl links directly against libblktap.  This would mean:
> >>
> >> 1) Changing libxl so that it could dynamically detect the presence of
> >> the proper version of libblktap at runtime and use the stubbed-out
> >> defaults if not available.
> >>
> >
> > This should be done in ./configure too, not during libxl build /
> > runtime.  If libblktap is not present during ./configure  then libxl
> > just use stubs.
> 
> It sounds like you're talking about introducing a hard dependency,
> such that packages that use a libxl built this way won't function
> without blktap installed.  Yeah, that's simple enough.
> 
> I'm not super experienced in the distro packaging mindset, but since
> (AFAIK) no other programs or projects use blktap, is there much point
> to having a separate repo if you can't "opt-out" of installing it?

It is not an hard dependency: as long as one doesn't use VHDs, ones
doesn't see any difference whether blktap is installed on not.


> >> #1 is even more difficult because at the moment blktap isn't really
> >> designed to have stable external versions -- it would involve adding a
> >> library version and all the fun stuff we already do with libxl.
> >>
> >
> > It blktap is to be maintained as external project to get wider usage
> > outside of Xen then this is a thing they have to do, isn't it?
> > Otherwise this is just an effort to separate blktap to an external tree,
> > Xen is still coupled with a certain blktap changeset. Not saying this
> > itself is a problem but it would be better to clarify what's the future
> > expectation of blktap as a project.
> 
> My understanding was:
> 
> 1. The XenServer team develop and maintain blktap primarily for their
> own product.
> 
> 2. They don't have a particular desire for blktap to get wider usage.
> It's actually other projects -- CentOS and COLO in particular -- that
> want to be able to use a maintained version of blktap.
> 
> 3. The XenServer developers are, as individuals, happy to have other
> people using it; and are willing to make small amounts of efforts to
> be an "upstream" -- namely, accepting bug fixes and reviewing minor
> contributions.
> 
> 4. However, the XenServer team is primarily focused on building their
> own product. They won't be able to spend a significant amount of time
> making blktap more "community-friendly".  Adding features that are
> useful to other downstreams but not to XenServer will probably be
> accepted, but I wouldn't be surprised if large architectural changes
> which help downstreams (such as COLO) but don't have any benefit for
> XenServer were rejected.

In that case we should probably not have blktap in the xen-unstable
tree, because we wouldn't want to have so divergent community models
for different component in the same source repository.

However we could make it easier to built libxl and blktap together, we
could also add blktap to OSSTest and Raisin
(http://marc.info/?l=xen-devel&m=142779794216955).


> I figured it was worth giving it a try.  If things don't work out,
> then we can either fork (if someone is willing to maintain it), or
> remove blktap support entirely.

Re: [PATCH RFC 2/4] build: Download and build extnernal blktap2.5 repo

[George Dunlap]

On 03/31/2015 11:40 AM, Stefano Stabellini wrote:
> On Tue, 31 Mar 2015, George Dunlap wrote:
>> On Mon, Mar 30, 2015 at 4:38 PM, Wei Liu <wei.liu2@citrix.com> wrote:
>>>>> IMHO we need to support --with-system-blktap= in configure in case
>>>>> distro wants to package blktap separately. Not sure if in practice this
>>>>> makes sense since AIUI blktap is only used by Xen.
>>>>
>>>> I agree that would be ideal; however, it's not so simple, because at the
>>>> moment libxl links directly against libblktap.  This would mean:
>>>>
>>>> 1) Changing libxl so that it could dynamically detect the presence of
>>>> the proper version of libblktap at runtime and use the stubbed-out
>>>> defaults if not available.
>>>>
>>>
>>> This should be done in ./configure too, not during libxl build /
>>> runtime.  If libblktap is not present during ./configure  then libxl
>>> just use stubs.
>>
>> It sounds like you're talking about introducing a hard dependency,
>> such that packages that use a libxl built this way won't function
>> without blktap installed.  Yeah, that's simple enough.
>>
>> I'm not super experienced in the distro packaging mindset, but since
>> (AFAIK) no other programs or projects use blktap, is there much point
>> to having a separate repo if you can't "opt-out" of installing it?
> 
> It is not an hard dependency: as long as one doesn't use VHDs, ones
> doesn't see any difference whether blktap is installed on not.

I'm talking about a hard dependency *for the resulting package*.

If libxl built linked against libblktap, and libblktap is not installed
on the system, then won't running the binary at all result in "Can't
find shared library" errors?

In any case, I'm pretty sure that if you build an RPM and link against a
particular library, that the RPM will then refuse to install if that
library isn't available.

> In that case we should probably not have blktap in the xen-unstable
> tree, because we wouldn't want to have so divergent community models
> for different component in the same source repository.

Community model doesn't matter to me; what matters to me is the
practical question of whether we can upstream what we need to upstream.
 An upstream that was in theory open but in practice difficult to work
with or hostile to our patches would be a lot worse than an upstream
that was in theory closed but in practice cooperative and took
everything we sent them.

I don't expect there to be any major changes required upstream, and so I
think that in practice this won't be an issue.  If it ever does become
an issue, then we can decide what to do about it at that time.

The reason I put it so strongly is that I want to make sure that there
are no hard feelings if we end up having to go separate ways.  The
XenServer team are doing us a favor, and haven't promised a large amount
of development time or commitment.  I think what they're willing to
offer will be sufficient for what we need.

> However we could make it easier to built libxl and blktap together, we
> could also add blktap to OSSTest and Raisin
> (http://marc.info/?l=xen-devel&m=142779794216955).

I wouldn't mind moving all external repos (qemu-*, seabios, ovmf,
blktap, &c) out of the Xen tree as soon as Raisin is mature enough to do so.

 -George

Re: [PATCH RFC 2/4] build: Download and build extnernal blktap2.5 repo

[Ian Campbell]

On Tue, 2015-03-31 at 12:10 +0100, George Dunlap wrote:
> > However we could make it easier to built libxl and blktap together, we
> > could also add blktap to OSSTest and Raisin
> > (http://marc.info/?l=xen-devel&m=142779794216955).
> 
> I wouldn't mind moving all external repos (qemu-*, seabios, ovmf,
> blktap, &c) out of the Xen tree as soon as Raisin is mature enough to do so.

Given the intertwining of blktap2 with libxl etc making it a
cloned-by-xen.git tree instead of in-xen.git might be a convenient first
step? The raisin development would then proceed using the
--with-system-blktap option and eventually the in tree stuff could be
dropped.

Ian.

Re: [PATCH RFC 2/4] build: Download and build extnernal blktap2.5 repo

[Ian Campbell]

On Tue, 2015-03-31 at 12:10 +0100, George Dunlap wrote:
> On 03/31/2015 11:40 AM, Stefano Stabellini wrote:
> > On Tue, 31 Mar 2015, George Dunlap wrote:
> >> On Mon, Mar 30, 2015 at 4:38 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> >>>>> IMHO we need to support --with-system-blktap= in configure in case
> >>>>> distro wants to package blktap separately. Not sure if in practice this
> >>>>> makes sense since AIUI blktap is only used by Xen.
> >>>>
> >>>> I agree that would be ideal; however, it's not so simple, because at the
> >>>> moment libxl links directly against libblktap.  This would mean:
> >>>>
> >>>> 1) Changing libxl so that it could dynamically detect the presence of
> >>>> the proper version of libblktap at runtime and use the stubbed-out
> >>>> defaults if not available.
> >>>>
> >>>
> >>> This should be done in ./configure too, not during libxl build /
> >>> runtime.  If libblktap is not present during ./configure  then libxl
> >>> just use stubs.
> >>
> >> It sounds like you're talking about introducing a hard dependency,
> >> such that packages that use a libxl built this way won't function
> >> without blktap installed.  Yeah, that's simple enough.
> >>
> >> I'm not super experienced in the distro packaging mindset, but since
> >> (AFAIK) no other programs or projects use blktap, is there much point
> >> to having a separate repo if you can't "opt-out" of installing it?
> > 
> > It is not an hard dependency: as long as one doesn't use VHDs, ones
> > doesn't see any difference whether blktap is installed on not.
> 
> I'm talking about a hard dependency *for the resulting package*.
> 
> If libxl built linked against libblktap, and libblktap is not installed
> on the system, then won't running the binary at all result in "Can't
> find shared library" errors?
> 
> In any case, I'm pretty sure that if you build an RPM and link against a
> particular library, that the RPM will then refuse to install if that
> library isn't available.

Correct (for Debian too), but this is more about the RPM maintainers
freedom to choose to enable it or not. Once they chosen their users will
just end up with whatever they chose.

For arch and gentoo folks I suppose this would be a USE var or whatever
they call it.

Ian.