[PATCH 0 of 7 v2] blktap3: Introduce the tapback daemon (most of blkback in user-space).

[PATCH 0 of 7 v2] blktap3: Introduce the tapback daemon (most of blkback in user-space).

[Thanos Makatos]

This patch series introduces the tapback daemon, the user space daemon that
acts as a device's back-end, essentially most of blkback in user space. The
daemon is responsible for coordinating the front-end and tapdisk. It instructs
the tapdisk to connect to/disconnect from the shared ring, and manages the
state of the back-end.

The shared ring between the front-end and tapdisk is created by a library that
will be introduced in another patch series (the entry point is function
tap_ctl_connect_xenblkif in frontend.c). This library also contains the
functionality that enables tapdisk to directly access the shared ring.

This series requires the RFC series described in
http://lists.xen.org/archives/html/xen-devel/2012-12/msg00254.html
in order to compile.

---
Changes since v1:
The series has been largely reorganised:
* Renamed the daemon from xenio to tapback.
* Improved description in patch 0.
* Merged structures and functions.
* Disaggregated functionality from the core daemon source file to smaller ones
  in order to facilitate the review process and improve maintenance.

[PATCH 1 of 7 v2] blktap3/tapback: Introduce core defines and structure definitions

[Thanos Makatos]

diff -r fbf47a5d98ff -r 0777319f0a6e tools/blktap3/tapback/tapback.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/tapback/tapback.h	Fri Jan 04 11:54:51 2013 +0000
@@ -0,0 +1,253 @@
+/*
+ * Copyright (C) 2012      Citrix Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
+ * USA.
+ */
+
+#ifndef __TAPBACK_H__
+#define __TAPBACK_H__
+
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <assert.h>
+#include <string.h>
+
+#include <xen/xen.h>
+#include <xenstore.h>
+#include <xen/io/xenbus.h>
+#include <xen/event_channel.h>
+#include <xen/grant_table.h>
+
+#include "tap-ctl.h"
+
+void tapback_log(int prio, const char *fmt, ...);
+void (*tapback_vlog) (int prio, const char *fmt, va_list ap);
+
+#define DBG(_fmt, _args...)  tapback_log(LOG_DEBUG, "%s:%d "_fmt, __FILE__, \
+        __LINE__, ##_args)
+#define INFO(_fmt, _args...) tapback_log(LOG_INFO, _fmt, ##_args)
+#define WARN(_fmt, _args...) tapback_log(LOG_WARNING, "%s:%d "_fmt, __FILE__, \
+        __LINE__, ##_args)
+
+#define WARN_ON(_cond, fmt, ...)    \
+    if (unlikely(_cond)) {          \
+        printf(fmt, ##__VA_ARGS__); \
+    }
+
+#define BUG_ON(_cond)       \
+    if (unlikely(_cond)) {  \
+        assert(0);          \
+    }
+
+/*
+ * XenStore path components.
+ *
+ * To avoid confusion with blktap2, we'll use a new kind of device for libxl
+ * defining it in tools/libxl/libxl_types_internal.idl. This will be done by
+ * the patch that adds libxl support for blktap3. TODO When that patch is sent,
+ * use the definition from there instead of hard-coding it here.
+ */
+#define XENSTORE_BACKEND        "backend"
+#define BLKTAP3_BACKEND_NAME    "xenio"
+#define BLKTAP3_BACKEND_PATH    XENSTORE_BACKEND"/"BLKTAP3_BACKEND_NAME
+#define BLKTAP3_BACKEND_TOKEN   XENSTORE_BACKEND"-"BLKTAP3_BACKEND_NAME
+#define BLKTAP3_FRONTEND_TOKEN  "otherend-state"
+#define BLKTAP3_SERIAL          BLKTAP3_BACKEND_NAME"-serial"
+
+/*
+ * TODO Put the rest of the front-end nodes defined in blkif.h here and group
+ * them. e.g. FRONTEND_NODE_xxx.
+ */
+#define RING_REF                "ring-ref"
+#define FEAT_PERSIST            "feature-persistent"
+
+/**
+ * A Virtual Block Device (VBD), represents a block device in a guest VM.
+ * Contains all relevant information.
+ */
+typedef struct vbd {
+
+    /**
+     * Device name, as retrieved from XenStore at probe-time.
+     */
+    char *name;
+    
+    /**
+     * The device ID. Same as vbd.name, we keep it around because the tapdisk
+     * control libreary wants it as an int and not as a string.
+     */
+    int devid;
+
+    /**
+     * For linked lists.
+     */
+     TAILQ_ENTRY(vbd) backend_entry;
+
+    /**
+     * TODO Supposed to solve some kind of race condition with libxl?
+     */
+    long long serial;
+
+    /**
+     * The domain ID this VBD belongs to.
+     */
+    domid_t domid;
+
+    /**
+     * The root directory in XenStore for this VBD. This is where all
+     * directories and key/value pairs related to this VBD are stored.
+     */
+    char *frontend_path;
+
+    /**
+     * XenStore path to the VBD's state. This is just
+     * vbd.frontend_path + "/state", we keep it around so we don't have to
+     * allocate/free memory all the time.
+     */
+    char *frontend_state_path;
+
+    /**
+     * FIXME This is used but isn't initialised anywhere! This reflects
+     * tap_list.minor.
+     */
+    dev_t dev;
+
+    /**
+     * Indicates whether the tapdisk is connected to the shared ring.
+     */
+    bool connected;
+
+    /**
+     * Descriptor of the tapdisk process serving this virtual block device. We
+     * need this until the end of the VBD's lifetime in order to disconnect
+     * the tapdisk from the shared ring.
+     */
+    tap_list_t tap;
+
+    /*
+     * XXX We keep sector_size, sectors, and info because we need to
+     * communicate them to the front-end not only when the front-end goes to
+     * XenbusStateInitialised, but to XenbusStateConnected as well.
+     */
+
+    /**
+     * Sector size, supplied by the tapdisk, communicated to blkfront.
+     */
+    unsigned int sector_size;
+
+    /**
+     * Number of sectors, supplied by the tapdisk, communicated to blkfront.
+     */
+    unsigned long long sectors;
+
+    /**
+     * VDISK_???, defined in include/xen/interface/io/blkif.h.
+     */
+    unsigned int info;
+
+} vbd_t;
+
+TAILQ_HEAD(tqh_vbd, vbd);
+
+/**
+ * The collection of all necessary handles and descriptors.
+ */
+struct _blktap3_daemon {
+
+    /**
+     * A handle to XenStore.
+     */
+    struct xs_handle *xs;
+
+    /**
+     * For executing transacted operations on XenStore.
+     */
+    xs_transaction_t xst;
+
+    /**
+     * The list of virtual block devices.
+     *
+     * TODO We sometimes have to scan the whole list to find the device/domain
+     * we're interested in, should we optimize this? E.g. use a hash table
+     * for O(1) access?
+     */
+    struct tqh_vbd devices;
+
+    /**
+     * For allocating serials.
+     */
+    long long serial;
+
+    /**
+     * TODO From xen/include/public/io/blkif.h: "The maximum supported size of
+     * the request ring buffer" 
+     */
+    int max_ring_page_order;
+};
+
+extern struct _blktap3_daemon blktap3_daemon;
+
+#define tapback_backend_for_each_device(_device, _next)	\
+	TAILQ_FOREACH_SAFE(_device, &blktap3_daemon.devices, backend_entry, _next)
+
+/**
+ * Iterates over all devices and returns the one for which the condition is
+ * true.
+ */
+#define tapback_backend_find_device(_device, _cond)		\
+do {													\
+	vbd_t *__next;								        \
+	int found = 0;										\
+	tapback_backend_for_each_device(_device, __next) {	\
+		if (_cond) {									\
+			found = 1;									\
+			break;										\
+		}												\
+	}													\
+	if (!found)											\
+		_device = NULL;									\
+} while (0)
+
+/**
+ * Act in response to a change in the front-end XenStore path.
+ *
+ * @param path the front-end's XenStore path that changed
+ * @returns 0 on success, an error code otherwise
+ *
+ * XXX Only called by tapback_read_watch
+ */
+int
+tapback_backend_handle_otherend_watch(const char * const path);
+
+/**
+ * Act in response to a change in the back-end directory in XenStore.
+ *
+ * If the path is "/backend" or "/backend/<backend name>", all devices are
+ * probed. Otherwise, the path should be
+ * "backend/<backend name>/<domid>/<device name>"
+ * (i.e. backend/<backend name>/1/51712), and in this case this specific device
+ * is probed.
+ *
+ * @param path the back-end's XenStore path that changed @returns 0 on success,
+ * an error code otherwise
+ *
+ * XXX Only called by tapback_read_watch.
+ */
+int
+tapback_backend_handle_backend_watch(char * const path);
+
+#endif /* __TAPBACK_H__ */

[PATCH 2 of 7 v2] blktap3/tapback: Introduces functionality required to access XenStore

[Thanos Makatos]

This patch introduces convenience functions that read/write values from/to
XenStore.

diff -r 0777319f0a6e -r efd3f343f7c7 tools/blktap3/tapback/xenstore.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/tapback/xenstore.c	Fri Jan 04 11:54:51 2013 +0000
@@ -0,0 +1,188 @@
+/*
+ * Copyright (C) 2012      Citrix Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
+ * USA.
+ */
+
+#include <stdarg.h>
+#include <stdio.h>
+#include <xenstore.h>
+#include <assert.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "blktap3.h"
+#include "tapback.h"
+#include "xenstore.h"
+
+/**
+ * Prints the supplied arguments to a buffer and returns it. The buffer must
+ * be deallocated by the user.
+ */
+static char *
+vmprintf(const char * const fmt, va_list ap)
+{
+    char *s = NULL;
+
+    if (vasprintf(&s, fmt, ap) < 0)
+        s = NULL;
+
+    return s;
+}
+
+__printf(1, 2)
+char *
+mprintf(const char * const fmt, ...)
+{
+    va_list ap;
+    char *s = NULL;
+
+    va_start(ap, fmt);
+    s = vmprintf(fmt, ap);
+    va_end(ap);
+
+    return s;
+}
+
+char *
+tapback_xs_vread(struct xs_handle * const xs, xs_transaction_t xst,
+        const char * const fmt, va_list ap)
+{
+    char *path = NULL, *data = NULL;
+    unsigned int len = 0;
+
+    assert(xs);
+
+    path = vmprintf(fmt, ap);
+    data = xs_read(xs, xst, path, &len);
+    free(path);
+
+    return data;
+}
+
+__printf(3, 4)
+char *
+tapback_xs_read(struct xs_handle * const xs, xs_transaction_t xst,
+        const char * const fmt, ...)
+{
+    va_list ap;
+    char *s = NULL;
+
+    assert(xs);
+
+    va_start(ap, fmt);
+    s = tapback_xs_vread(xs, xst, fmt, ap);
+    va_end(ap);
+
+    return s;
+}
+
+char *
+tapback_device_read(const vbd_t * const device, const char * const path)
+{
+    assert(device);
+    assert(path);
+
+    return tapback_xs_read(blktap3_daemon.xs, blktap3_daemon.xst, "%s/%d/%s/%s",
+            BLKTAP3_BACKEND_PATH, device->domid, device->name, path);
+}
+
+char *
+tapback_device_read_otherend(vbd_t * const device,
+        const char * const path)
+{
+    assert(device);
+    assert(path);
+
+    return tapback_xs_read(blktap3_daemon.xs, blktap3_daemon.xst, "%s/%s",
+            device->frontend_path, path);
+}
+
+__scanf(3, 4)
+int
+tapback_device_scanf_otherend(vbd_t * const device,
+        const char * const path, const char * const fmt, ...)
+{
+    va_list ap;
+    int n = 0;
+    char *s = NULL;
+
+    assert(device);
+    assert(path);
+
+    if (!(s = tapback_device_read_otherend(device, path)))
+        return -1;
+    va_start(ap, fmt);
+    n = vsscanf(s, fmt, ap);
+    free(s);
+    va_end(ap);
+
+    return n;
+}
+
+__printf(4, 5)
+int
+tapback_device_printf(vbd_t * const device, const char * const key,
+        const bool mkread, const char * const fmt, ...)
+{
+    va_list ap;
+    int err = 0;
+    char *path = NULL, *val = NULL;
+    bool nerr = false;
+
+    assert(device);
+    assert(key);
+
+    if (!(path = mprintf("%s/%d/%s/%s", BLKTAP3_BACKEND_PATH, device->domid,
+                    device->name, key))) {
+        err = -errno;
+        goto fail;
+    }
+
+    va_start(ap, fmt);
+    val = vmprintf(fmt, ap);
+    va_end(ap);
+
+    if (!val) {
+        err = -errno;
+        goto fail;
+    }
+
+    if (!(nerr = xs_write(blktap3_daemon.xs, blktap3_daemon.xst, path, val,
+                    strlen(val)))) {
+        err = -errno;
+        goto fail;
+    }
+
+    if (mkread) {
+        struct xs_permissions perms = {
+            device->domid,
+            XS_PERM_READ
+        };
+
+        if (!(nerr = xs_set_permissions(blktap3_daemon.xs, blktap3_daemon.xst,
+                        path, &perms, 1))) {
+            err = -errno;
+            goto fail;
+        }
+    }
+
+fail:
+    free(path);
+    free(val);
+
+    return err;
+}
diff -r 0777319f0a6e -r efd3f343f7c7 tools/blktap3/tapback/xenstore.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/tapback/xenstore.h	Fri Jan 04 11:54:51 2013 +0000
@@ -0,0 +1,103 @@
+/*
+ * Copyright (C) 2012      Citrix Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
+ * USA.
+ */
+
+/**
+ * Prints the supplied arguments to a buffer and returns it. The buffer must
+ * be deallocated by the user.
+ */
+__printf(1, 2)
+char *
+mprintf(const char * const fmt, ...);
+
+/**
+ * Retrieves the XenStore value of the specified key of the VBD's front-end.
+ * The caller must free the returned buffer.
+ *
+ * @param device the VBD
+ * @param path key under the front-end directory
+ * @returns a buffer containing the value, or NULL on error
+ */
+char *
+tapback_device_read_otherend(vbd_t * const device,
+        const char * const path);
+
+/**
+ * Writes to XenStore backened/tapback/<domid>/<devname>/@key = @fmt.
+ *
+ * @param device the VBD
+ * @param key the key to write to
+ * @param mkread TODO
+ * @param fmt format
+ * @returns 0 on success, an error code otherwise
+ */
+__printf(4, 5)
+int
+tapback_device_printf(vbd_t * const device, const char * const key,
+        const bool mkread, const char * const fmt, ...);
+
+/**
+ * Reads the specified XenStore path under the front-end directory in a
+ * scanf-like manner.
+ *
+ * @param device the VBD
+ * @param path XenStore path to read
+ * @param fmt format
+ */
+__scanf(3, 4)
+int
+tapback_device_scanf_otherend(vbd_t * const device,
+        const char * const path, const char * const fmt, ...);
+
+/**
+ * Retrieves the value of the specified of the device from XenStore,
+ * i.e. backend/tapback/<domid>/<devname>/@path
+ * The caller must free the returned buffer.
+ *
+ * @param device the VBD 
+ * @param path the XenStore key
+ * @returns a buffer containing the value, or NULL on error
+ */
+char *
+tapback_device_read(const vbd_t * const device, const char * const path);
+
+/**
+ * Reads the specified XenStore path. The caller must free the returned buffer.
+ *
+ * @param xs handle to XenStore
+ * @param xst XenStore transaction 
+ * @param fmt format
+ * @param ap arguments
+ * @returns a buffer containing the value, or NULL on error
+ */
+char *
+tapback_xs_vread(struct xs_handle * const xs, xs_transaction_t xst,
+        const char * const fmt, va_list ap);
+
+/**
+ * Reads the specified XenStore path. The caller must free the returned buffer.
+ *
+ * @param xs handle to XenStore
+ * @param xst XenStore transaction
+ * @param fmt format
+ * @returns a buffer containing the value, or NULL on error
+ */
+__printf(3, 4)
+char *
+tapback_xs_read(struct xs_handle * const xs, xs_transaction_t xst,
+        const char * const fmt, ...);

[PATCH 3 of 7 v2] blktap3/tapback: Logging for the tapback daemon and libxenio

[Thanos Makatos]

diff -r efd3f343f7c7 -r a9beb303b541 tools/blktap3/tapback/log.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/tapback/log.c	Fri Jan 04 12:08:03 2013 +0000
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2012      Citrix Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
+ * USA.
+ */
+
+#include "compiler.h"
+#include <stdarg.h>
+#include <syslog.h>
+
+void (*tapback_vlog) (int prio, const char *fmt, va_list ap) = vsyslog;
+
+__printf(2, 3) void
+tapback_log(int prio, const char *fmt, ...)
+{
+    va_list ap;
+    va_start(ap, fmt);
+    tapback_vlog(prio, fmt, ap);
+    va_end(ap);
+}

[PATCH 4 of 7 v2] blktap3/tapback: Introduce back-end XenStore path handler

[Thanos Makatos]

This patch introduces the handler executed when the back-end XenStore path gets
modified. A back-end XenStore path is modified as a result of a device
creation/removal request. The device is created/removed depending on whether
its path exists/does not exist in XenStore.

Creating a device comprises creating the in-memory representation of it and
adding it to the list of devices, locating the tapdisk designated to serve
this VBD, and setting a XenStore watch to the front-end path of the
newly-created device.

Deleting a device comprises removing that XenStore watch and deallocating its
in-memory representation.

The main issues in this patch are the following:

* Function tapback_backend_handle_backend_watch calls tapback_backend_scan
  even when we know in which domain a device changed, wouldn't it make sense
  from a performance perspective to only probe the devices of that domain
  instead of probing ALL devices of ALL domains?

* Is there a race condition between the tapback daemon and the toolstack
  regarding partially brought up devices? I'm trying to figure out what the
  "serial" thing does (check functions tapback_backend_create_device and
  tapback_backend_probe_device) but I don't get it. Even if there is such a
  race condition, I'm not convinced that the way that "serial" thing is
  implemented fixes it.

diff -r a9beb303b541 -r 8094db37a249 tools/blktap3/tapback/backend.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/tapback/backend.c	Fri Jan 04 12:09:05 2013 +0000
@@ -0,0 +1,434 @@
+/*
+ * Copyright (C) 2012      Citrix Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
+ * USA.
+ *
+ * This file contains the handler executed when the back-end XenStore path gets
+ * modified.
+ */
+
+#include "tapback.h"
+#include "xenstore.h"
+#include "tap-ctl-info.h"
+
+/**
+ * Removes the XenStore watch from the front-end.
+ *
+ * @param device the VBD whose front-end XenStore path should stop being
+ * watched
+ */
+static void
+tapback_device_unwatch_frontend_state(vbd_t * const device)
+{
+    assert(device);
+
+    if (device->frontend_state_path)
+        xs_unwatch(blktap3_daemon.xs, device->frontend_state_path,
+                BLKTAP3_FRONTEND_TOKEN);
+
+    free(device->frontend_state_path);
+    device->frontend_state_path = NULL;
+}
+
+/**
+ * Destroys and deallocates the back-end part of a VBD.
+ *
+ * @param device the VBD to destroy
+ */
+static void
+tapback_backend_destroy_device(vbd_t * const device)
+{
+    assert(device);
+
+    TAILQ_REMOVE(&blktap3_daemon.devices, device, backend_entry);
+
+    tapback_device_unwatch_frontend_state(device);
+
+    if (device->frontend_path) {
+        free(device->frontend_path);
+        device->frontend_path = NULL;
+    }
+
+    if (device->name) {
+        free(device->name);
+        device->name = NULL;
+    }
+
+    /*
+     * XXX device->bdev is expected to have been freed
+     */
+
+    free(device);
+}
+
+/**
+ * Retrieves the tapdisk designated to serve this device, storing this
+ * information in the supplied VBD handle.
+ *
+ * @param bdev the VBD handle for whose tapdisk handle should be retrieved
+ * @returns 0 on success, an error code otherwise
+ *
+ * FIXME How does this thing work since bdev->dev is never initialised?
+ * Probably because they're both initialised to 0 and we use one tapdisk per
+ * VBD?
+ *
+ * XXX Only called by blkback_probe.
+ */
+static inline int
+blkback_find_tapdisk(vbd_t * const bdev)
+{
+    struct tqh_tap_list list;
+    tap_list_t *tap = NULL;
+    int err = 0;
+
+    assert(bdev);
+
+    if ((err = tap_ctl_list(&list))) {
+        WARN("error listing tapdisks: %s\n", strerror(err));
+        return err;
+    }
+
+    err = ESRCH;    
+    if (!TAILQ_EMPTY(&list)) {
+        tap_list_for_each_entry(tap, &list)
+            if (tap->minor == minor(bdev->dev)) {
+                err = 0;
+                memcpy(&bdev->tap, tap, sizeof(bdev->tap));
+                break;
+            }
+        tap_ctl_list_free(&list);
+    } else
+        WARN("no tapdisks\n");
+
+    return err;
+}
+
+/**
+ * Creates a device and adds it to the list of devices.
+ * Initiates a XenStore watch to the front-end state.
+ *
+ * Creating the device implies initializing the handle and retrieving all the
+ * information of the tapdisk serving this VBD.
+ *
+ * @param domid the ID of the domain where the VBD is created
+ * @param name the name of the device
+ * @returns 0 on success, an error code otherwise
+ */
+static inline int
+tapback_backend_create_device(const domid_t domid, const char * const name)
+{
+    vbd_t *device = NULL;
+    int err = 0;
+    char *end = NULL;
+
+    assert(name);
+
+    DBG("creating device %d/%s\n", domid, name);
+
+    if (!(device = calloc(1, sizeof(*device)))) {
+        WARN("error allocating memory\n");
+        err = -errno;
+        goto fail;
+    }
+
+	/*
+	 * TODO replace with alloc_serial() and check for overflow
+	 */
+    device->serial = blktap3_daemon.serial++;
+    device->domid = domid;
+
+    TAILQ_INSERT_TAIL(&blktap3_daemon.devices, device, backend_entry);
+
+    if (!(device->name = strdup(name))) {
+        err = -errno;
+        goto fail;
+    }
+
+    /*
+     * Get the front-end path from XenStore. We need this to talk to front-end.
+     */
+    if (!(device->frontend_path = tapback_device_read(device, "frontend"))) {
+        err = -errno;
+        goto fail;
+    }
+
+    /*
+     * Write to XenStore tapback-serial.
+     */
+    if ((err = tapback_device_printf(device, BLKTAP3_SERIAL, false, "%lld",
+                    device->serial)))
+        goto fail;
+
+    /*
+     * Get the tapdisk that is serving this virtual block device, along with
+     * it's parameters.
+     */
+    device->devid = strtoul(name, &end, 0);
+    if (*end != 0 || end == name) {
+        err = -EINVAL;
+        goto fail;
+    }
+
+    if ((err = blkback_find_tapdisk(device))) {
+        WARN("error looking for tapdisk: %s", strerror(err));
+        goto fail;
+    }
+
+    /*
+     * get the VBD parameters from the tapdisk
+     */
+    if ((err = tap_ctl_info(device->tap.pid, device->tap.minor,
+                    &device->sectors, &device->sector_size, &device->info))) {
+        WARN("error retrieving disk characteristics: %s\n", strerror(err));
+        goto fail;
+    }
+
+    DBG("got %s-%d-%d with tapdev %d/%d\n", BLKTAP3_BACKEND_NAME,
+        device->domid, device->devid, device->tap.pid, device->tap.minor);
+
+	/*
+	 * Finally, watch the front-end path in XenStore for changes, i.e.
+     * /local/domain/<domid>/device/vbd/<devname>/state
+     * After this, we wait for the front-end to switch state to continue with
+     * the initialisation.
+	 */
+    if (!(device->frontend_state_path= mprintf("%s/state",
+                    device->frontend_path))) {
+        err = -errno;
+        goto fail;
+    }
+    /*
+     * We use the same token for all front-end watches. We don't have to use a
+     * unique token for each front-end watch because when a front-end watch
+     * fires we are given the XenStore path that changed.
+     */
+    if (!xs_watch(blktap3_daemon.xs, device->frontend_state_path,
+                BLKTAP3_FRONTEND_TOKEN)) {
+        free(device->frontend_state_path);
+        err = -errno;
+        goto fail;
+    }
+
+    return 0;
+
+fail:
+    WARN("error creating device: domid=%d name=%s err=%d (%s)\n",
+            domid, name, err, strerror(-err));
+    if (device)
+        tapback_backend_destroy_device(device);
+
+    return err;
+}
+
+/**
+ * Creates (removes) a device depending on the existence (non-existence) of the
+ * "backend/<backend name>/@domid/@devname" XenStore path.
+ *
+ * @param domid the ID of the domain where the VBD is created
+ * @param devname device name
+ * @returns 0 on success, an error code otherwise
+ */
+static int
+tapback_backend_probe_device(const domid_t domid, const char * const devname)
+{
+    int should_exist = 0, create = 0, remove = 0;
+    vbd_t *device = NULL;
+    char * s = NULL;
+
+    assert(devname);
+
+    DBG("probe device domid=%d name=%s\n", domid, devname);
+
+    /*
+     * Ask XenStore if the device _should_ exist.
+     */
+    s = tapback_xs_read(blktap3_daemon.xs, blktap3_daemon.xst, "%s/%d/%s",
+            BLKTAP3_BACKEND_PATH, domid, devname);
+    should_exist = s != NULL;
+    free(s);
+
+    /*
+     * Search the device list for this specific device.
+     */
+    tapback_backend_find_device(device,
+            device->domid == domid && !strcmp(device->name, devname));
+
+    /*
+	 * If XenStore says that the device should exist but it's not in our device 
+     * list, we must create it. If it's the other way round, this is a removal.
+     * If XenStore says that the device should exist and it's already in our
+     * device list, we must compare the serials in case the device got removed
+     * and re-created.
+     */
+    remove = device && !should_exist;
+    create = !device && should_exist;
+
+    DBG("should exist=%d device=%p remove=%d create=%d\n",
+        should_exist, device, remove, create);
+
+    /*
+     * The device exists in our list of device and XenStore says it should
+     * exist, we need to check if the device has been removed and re-added to
+     * XenStore. If this has happened the device's serial in XenStore will be
+     * different from the one in our list (sync with fast remove/re-create
+     * cycles).
+     */
+    if (device && should_exist) {
+        char *s = NULL;
+        long long serial = 0;
+
+        /*
+         * read the serial
+         */
+        if (!(s = tapback_device_read(device, BLKTAP3_SERIAL))) {
+            WARN("error reading serial for %s: %s\n", devname,
+                    strerror(errno));
+            return -errno;
+        }
+        serial = atoll(s);
+        free(s);
+
+        remove = create = serial != device->serial;
+
+        /*
+         * TODO Is it possible that neither create nor remove are true? If so,
+         * why was this function called in the first place?
+         */
+        WARN_ON(!remove && !create, "%d/%s neither created nor removed\n",
+                domid, devname);
+    }
+
+    /*
+     * Remember that remove and create may both be true at the same time, as
+     * this indicates that the device has been removed and re-created too fast.
+     * In this case, we too need to remove and re-create the device,
+     * respectively.
+     */
+
+    if (remove)
+        tapback_backend_destroy_device(device);
+
+    if (create) {
+        const int err = tapback_backend_create_device(domid, devname);
+        if (0 != err) {
+            WARN("error creating device %d on domain %d: %s\n", devname, domid,
+                    strerror(-err));
+            return err;
+        }
+    }
+
+    return 0;
+}
+
+/**
+ * Scans XenStore for all blktap3 devices and probes each one of them.
+ *
+ * XXX Only called by tapback_backend_handle_backend_watch.
+ */
+static int
+tapback_backend_scan(void)
+{
+    vbd_t *device = NULL, *next = NULL;
+    unsigned int i = 0, j = 0, n = 0, m = 0;
+    char **dir = NULL;
+
+    /*
+     * scrap all non-existent devices
+     * FIXME Why do we do this?
+     * FIXME Is this costly?
+     */
+
+    tapback_backend_for_each_device(device, next)
+        tapback_backend_probe_device(device->domid, device->name);
+
+    /*
+     * probe the new ones
+     *
+     * FIXME We're checking each and every device in each and every domain,
+     * could there be a performance issue in the presence of many VMs/VBDs?
+     * (e.g. boot-storm)
+     */
+    if (!(dir = xs_directory(blktap3_daemon.xs, blktap3_daemon.xst,
+                    BLKTAP3_BACKEND_PATH, &n)))
+        return 0;
+
+    for (i = 0; i < n; i++) { /* for each domain */
+        char *path = NULL, **sub = NULL, *end = NULL;
+        domid_t domid = 0;
+
+        /*
+         * Get the domain ID.
+         */
+        domid = strtoul(dir[i], &end, 0);
+        if (*end != 0 || end == dir[i])
+            continue;
+
+        /*
+         * Read the devices of this domain.
+         */
+        path = mprintf("%s/%d", BLKTAP3_BACKEND_PATH, domid);
+        assert(path != NULL);
+        sub = xs_directory(blktap3_daemon.xs, blktap3_daemon.xst, path, &m);
+        free(path);
+
+        /*
+         * Probe each device.
+         */
+        for (j = 0; j < m; j++)
+            tapback_backend_probe_device(domid, sub[j]);
+
+        free(sub);
+    }
+
+    free(dir);
+    return 0;
+}
+
+int
+tapback_backend_handle_backend_watch(char * const path)
+{
+    char *s = NULL, *end = NULL, *name = NULL;
+    domid_t domid = 0;
+
+    assert(path);
+
+    s = strtok(path, "/");
+    assert(!strcmp(s, XENSTORE_BACKEND));
+    if (!(s = strtok(NULL, "/")))
+        return tapback_backend_scan();
+
+    assert(!strcmp(s, BLKTAP3_BACKEND_NAME));
+    if (!(s = strtok(NULL, "/")))
+        return tapback_backend_scan();
+
+    domid = strtoul(s, &end, 0);
+    if (*end != 0 || end == s) {
+        WARN("invalid domain ID \'%s\'\n", s);
+        return -EINVAL;
+    }
+
+    /*
+     * FIXME since we know which domain changed, why scan the whole thing?
+     * Add the domid as an optional parameter to tapback_backend_scan.
+     */
+    if (!(name = strtok(NULL, "/")))
+        return tapback_backend_scan();
+
+    /*
+     * Create or remove a specific device.
+     */
+    return tapback_backend_probe_device(domid, name);
+}

[PATCH 5 of 7 v2] blktap3/tapback: Introduce front-end XenStore path handler

[Thanos Makatos]

This patch introduces the handler executed when the front-end XenStore path of
a VBD gets modified. This is done when the front-end switches state. The core
of this functionality is connecting/disconnecting the tapdisk to/from the
shared ring.

When the front-end goes to Initialised or Connected state, the daemon reads all
the necessary information from XenStore provided by the front-end, initiates
the shared ring creation (the actual creation is performed by a library that
will be introduced by another patch series), and instructs the tapdisk
designated to serve this VBD to connect to the ring. Finally, it communicates
to the front-end all the necessary disk parameters.

When the front-end switches to Closed, the tapdisk is disconnected from the
shared ring.

diff -r 8094db37a249 -r d332adcebe40 tools/blktap3/include/blktap3.h
--- a/tools/blktap3/include/blktap3.h	Fri Jan 04 12:09:05 2013 +0000
+++ b/tools/blktap3/include/blktap3.h	Fri Jan 04 12:10:09 2013 +0000
@@ -44,4 +44,19 @@
 	TAILQ_REMOVE(src, node, entry);				\
 	TAILQ_INSERT_TAIL(dst, node, entry);
 
+/**
+ * Block I/O protocol
+ *
+ * Taken from linux/drivers/block/xen-blkback/common.h so that blkfront can
+ * work both with blktap2 and blktap3.
+ *
+ * TODO linux/drivers/block/xen-blkback/common.h contains other definitions
+ * necessary for allowing tapdisk3 to talk directly to blkfront. Find a way to
+ * use the definitions from there.
+ */
+enum blkif_protocol {
+       BLKIF_PROTOCOL_NATIVE = 1,
+       BLKIF_PROTOCOL_X86_32 = 2,
+       BLKIF_PROTOCOL_X86_64 = 3,
+};
 #endif /* __BLKTAP_3_H__ */
diff -r 8094db37a249 -r d332adcebe40 tools/blktap3/tapback/frontend.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/tapback/frontend.c	Fri Jan 04 12:10:09 2013 +0000
@@ -0,0 +1,396 @@
+/*
+ * Copyright (C) 2012      Citrix Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
+ * USA.
+ *
+ * This file contains the handler executed when the front-end XenStore path of
+ * a VBD gets modified. 
+ */
+
+#include "tapback.h"
+#include "xenstore.h"
+#include "tap-ctl-xen.h"
+
+#include <xen/io/protocols.h>
+
+/**
+ * Switches the back-end state of the device by writing to XenStore.
+ *
+ * @param device the VBD
+ * @param state the state to switch to
+ * @returns 0 on success, an error code otherwise
+ */
+static int
+tapback_device_switch_state(vbd_t * const device,
+        const XenbusState state)
+{
+    assert(device);
+
+    /*
+     * TODO Ensure @state contains a legitimate XenbusState value.
+     * TODO Check for valid state transitions?
+     */
+
+    return tapback_device_printf(device, "state", false, "%u", state);
+}
+
+/**
+ * Core functions that instructs the tapdisk to connect to the shared ring (if
+ * not already connected) and communicates essential information to the
+ * front-end.
+ *
+ * If the tapdisk is not already connected, all the necessary information is
+ * read from XenStore and the tapdisk gets connected using this information.
+ *
+ * TODO Why should this function be called on an already connected VBD? Why
+ * re-write the sector size etc. in XenStore for an already connected VBD?
+ * TODO rename function (no blkback, not only connects to tapdisk)
+ *
+ * @param xbdev the VBD the tapdisk should connect to
+ * @param state unused
+ * @returns 0 on success, an error code otherwise
+ *
+ * XXX Only called by blkback_frontend_changed, when the front-end switches to
+ * Initialised and Connected.
+ */
+static int
+blkback_connect_tap(vbd_t * const bdev,
+        const XenbusState state __attribute__((unused)))
+{
+    evtchn_port_t port = 0;
+    grant_ref_t *gref = NULL;
+    int err = 0;
+    char *proto_str = NULL;
+    char *persistent_grants_str = NULL;
+
+    assert(bdev);
+
+    if (bdev->connected) {
+        DBG("front-end already connected to tapdisk.\n");
+    } else {
+        /*
+         * How can we make sure we're not missing a node written by the
+         * front-end?
+         */
+        int nr_pages = 0, proto = 0, order = 0;
+        bool persistent_grants = false;
+
+        if (1 != tapback_device_scanf_otherend(bdev, "ring-page-order", "%d",
+                    &order))
+            order = 0;
+
+         nr_pages = 1 << order;
+
+        if (!(gref = calloc(nr_pages, sizeof(grant_ref_t)))) {
+            WARN("Failed to allocate memory for grant refs.\n");
+            err = ENOMEM;
+            goto fail;
+        }
+
+        /*
+         * Read the grant references.
+         */
+        if (order) {
+            int i = 0;
+            /*
+             * +10 is for INT_MAX, +1 for NULL termination
+             */
+            static const size_t len = sizeof(RING_REF) + 10 + 1;
+            char ring_ref[len];
+            for (i = 0; i < nr_pages; i++) {
+                if (snprintf(ring_ref, len, "%s%d", RING_REF, i) >= len) {
+                    DBG("error printing to buffer\n");
+                    err = EINVAL;
+                    goto fail;
+                }
+                if (1 != tapback_device_scanf_otherend(bdev, ring_ref, "%u",
+                            &gref[i])) {
+                    WARN("Failed to read grant ref 0x%x.\n", i);
+                    err = ENOENT;
+                    goto fail;
+                }
+            }
+        } else {
+            if (1 != tapback_device_scanf_otherend(bdev, RING_REF, "%u",
+                        &gref[0])) {
+                WARN("Failed to read grant ref.\n");
+                err = ENOENT;
+                goto fail;
+            }
+        }
+
+        /*
+         * Read the event channel.
+         */
+        if (1 != tapback_device_scanf_otherend(bdev, "event-channel", "%u",
+                    &port)) {
+            WARN("Failed to read event channel.\n");
+            err = ENOENT;
+            goto fail;
+        }
+
+        /*
+         * Read the guest VM's ABI.
+         */
+        if (!(proto_str = tapback_device_read_otherend(bdev, "protocol")))
+            proto = BLKIF_PROTOCOL_NATIVE;
+        else if (!strcmp(proto_str, XEN_IO_PROTO_ABI_X86_32))
+            proto = BLKIF_PROTOCOL_X86_32;
+        else if (!strcmp(proto_str, XEN_IO_PROTO_ABI_X86_64))
+            proto = BLKIF_PROTOCOL_X86_64;
+        else {    
+            WARN("unsupported protocol %s\n", proto_str);
+            err = EINVAL;
+            goto fail;
+        }
+
+        /*
+         * Does the front-end support persistent grants?
+         */
+        if (!(persistent_grants_str = tapback_device_read_otherend(bdev,
+                FEAT_PERSIST))) {
+            WARN("error reading %s\n", FEAT_PERSIST);
+            err = EINVAL;
+            goto fail;
+        }
+        if (!strcmp(persistent_grants_str, "0"))
+            persistent_grants = false;
+        else if (!strcmp(persistent_grants_str, "1"))
+            persistent_grants = true;
+        else {
+            WARN("invalid %s value: %s\n", FEAT_PERSIST,
+                    persistent_grants_str);
+            err = EINVAL;
+            goto fail;
+        }
+
+        /*
+         * persistent grants are not yet supported
+         */
+        if (persistent_grants)
+            WARN("front-end supports persistent grants but we don't\n");
+
+        /*
+         * Create the shared ring and ask the tapdisk to connect to it.
+         */
+        if ((err = tap_ctl_connect_xenblkif(bdev->tap.pid, bdev->tap.minor,
+                        bdev->domid, bdev->devid, gref, order, port, proto,
+                        NULL))) {
+            WARN("tapdisk failed to connect to the shared ring: %s",
+                    strerror(err));
+            goto fail;
+        }
+
+        bdev->connected = true;
+    }
+
+    /*
+     * Write the number of sectors, sector size, and info to the back-end path
+     * in XenStore so that the front-end creates a VBD with the appropriate
+     * characteristics.
+     */
+    if ((err = tapback_device_printf(bdev, "sector-size", true, "%u",
+                    bdev->sector_size))) {
+        WARN("Failed to write sector-size.\n");
+        goto fail;
+    }
+
+    if ((err = tapback_device_printf(bdev, "sectors", true, "%llu",
+                    bdev->sectors))) {
+        WARN("Failed to write sectors.\n");
+        goto fail;
+    }
+
+    if ((err = tapback_device_printf(bdev, "info", true, "%u", bdev->info))) {
+        WARN("Failed to write info.\n");
+        goto fail;
+    }
+
+    if ((err = tapback_device_switch_state(bdev, XenbusStateConnected))) {
+        WARN("Failed to switch state %d\n", err);
+    }
+
+fail:
+    if (err && bdev->connected) {
+        const int err2 = tap_ctl_disconnect_xenblkif(bdev->tap.pid,
+                bdev->tap.minor, bdev->domid, bdev->devid, NULL);
+        if (err2) {
+            WARN("error disconnecting tapdisk from the shared ring (ignored):"
+                    " %s\n", strerror(err2));
+        }
+
+        bdev->connected = false;
+    }
+
+    free(gref);
+    free(proto_str);
+    free(persistent_grants_str);
+
+    return err;
+}
+
+/**
+ * Callback that is executed when the front-end goes to StateClosed.
+ *
+ * Instructs the tapdisk to disconnect itself from the shared ring and switches
+ * the back-end state to StateClosed.
+ *
+ * @param xbdev the VBD whose tapdisk should be disconnected
+ * @param state unused
+ * @returns 0 on success, an error code otherwise
+ *
+ * XXX Only called by blkback_frontend_changed.
+ */
+static inline int
+backend_close(vbd_t * const bdev,
+        const XenbusState state __attribute__((unused)))
+{
+    int err = 0;
+
+    assert(bdev);
+
+    if (!bdev->connected) {
+        /*
+         * TODO Is this safe? Shouldn't we report an error?
+         */
+        DBG("tapdisk not connected\n");
+        return 0;
+    }
+
+    DBG("disconnecting vbd-%d-%d from tapdisk %d minor %d\n",
+        bdev->domid, bdev->devid, bdev->tap.pid, bdev->tap.minor);
+
+    if ((err = tap_ctl_disconnect_xenblkif(bdev->tap.pid, bdev->tap.minor,
+            bdev->domid, bdev->devid, NULL))){
+
+        /*
+         * TODO I don't see how tap_ctl_disconnect_xenblkif can return
+         * ESRCH, so this is probably wrong. Probably there's another error
+         * code indicating that there's no tapdisk process.
+         */
+        if (errno == -ESRCH) {
+            WARN("tapdisk not running\n");
+        } else {
+            WARN("error disconnecting tapdisk from front-end: %s\n",
+                    strerror(err));
+            return err;
+        }
+    }
+
+    bdev->connected = false;
+
+    return tapback_device_switch_state(bdev, XenbusStateClosed);
+}
+
+/**
+ * Acts on changes in the front-end state.
+ *
+ * TODO The back-end blindly follows the front-ends state transitions, should
+ * we check whether unexpected transitions are performed?
+ *
+ * @param xbdev the VBD whose front-end state changed
+ * @param state the new state
+ * @returns 0 on success, an error code otherwise
+ *
+ * XXX Only called by tapback_device_check_front-end_state.
+ */
+static inline int
+blkback_frontend_changed(vbd_t * const xbdev, const XenbusState state)
+{
+    /*
+     * XXX The size of the array (9) comes from the XenbusState enum.
+     *
+     * TODO Send a patch that adds XenbusStateMin, XenbusStateMax,
+     * XenbusStateInvalid and in the XenbusState enum (located in xenbus.h).
+     */
+    struct frontend_state_change {
+        int (*fn)(vbd_t * const, const XenbusState);
+        const XenbusState state;
+    } static const frontend_state_change_map[] = {
+        {NULL, 0},                                          /* Unknown */
+        {tapback_device_switch_state, XenbusStateInitWait}, /* Initialising */
+        {NULL, 0},                                          /* InitWait */
+        {blkback_connect_tap, 0},                           /* Initialised */
+        {blkback_connect_tap, 0},                           /* Connected */
+        {tapback_device_switch_state, XenbusStateClosing},  /* Closing */
+        {backend_close, 0},                                 /* StateClosed */
+        {NULL, 0},                                          /* Reconfiguring */
+        {NULL, 0}                                           /* Reconfigured */
+    };
+
+    assert(xbdev);
+    assert(state >= XenbusStateUnknown && state <= XenbusStateReconfigured);
+
+    DBG("frontend_changed %s-%d-%s state=%d\n", BLKTAP3_BACKEND_NAME,
+            xbdev->domid, xbdev->name, state);
+
+    if (frontend_state_change_map[state].fn)
+        return frontend_state_change_map[state].fn(xbdev,
+                frontend_state_change_map[state].state);
+    return 0;
+}
+
+int
+tapback_backend_handle_otherend_watch(const char * const path)
+{
+    vbd_t *device = NULL;
+    int err = 0, state = 0;
+    char *s = NULL, *end = NULL;
+
+    assert(path);
+
+    /*
+     * Find the device that has the same front-end state path.
+     *
+     * There should definitely be such a device in our list, otherwise this
+     * function would not have executed at all, since we would not be waiting
+     * on that XenStore path.  The XenStore path we wait for is:
+     * /local/domain/<domid>/device/vbd/<devname>/state. In order to watch this
+     * path, it means that we have received a device create request, so the
+     * device will be there.
+     *
+     * TODO Instead of this linear search we could do better (hash table etc).
+     */
+    tapback_backend_find_device(device,
+            !strcmp(device->frontend_state_path, path));
+    if (!device) {
+        WARN("path \'%s\' does not correspond to a known device\n", path);
+        return ENODEV;
+    }
+
+    DBG("device: domid=%d name=%s\n", device->domid, device->name);
+
+    /*
+     * Read the new front-end's state.
+     */
+    if (!(s = tapback_xs_read(blktap3_daemon.xs, blktap3_daemon.xst, "%s",
+                    device->frontend_state_path))) {
+        err = -errno;
+        goto fail;
+    }
+    state = strtol(s, &end, 0);
+    if (*end != 0 || end == s) {
+        err = -EINVAL;
+        goto fail;
+    }
+
+    err = blkback_frontend_changed(device, state);
+	DBG("change from state %d: %d\n", state, err);
+
+fail:
+    free(s);
+    return err;
+}

[PATCH 6 of 7 v2] blktap3/tapback: Introduce the tapback daemon

[Thanos Makatos]

This patch introduces the core of the tapback daemon, the user space daemon
that acts as a device's back-end, essentially most of blkback in user space.
Similar to blkback, the daemon monitors XenStore for device creation/removal
requests and front-end state changes, and acts in response to them
(communicates to the front-end necessary information, switches the back-end's
state etc.).

The main issue in this patch is the following architectural change: in blktap2,
libxl spawns the tapdisk and then instructs it to connect to the shared ring
created by blktap.  In blktap3, libxl spawns the tapdisk but the tapdisk just
sits there.  When the tapback daemon detects that the front-end has switched
into the Initialised/Connected state, _it_ instructs the tapdisk to connect to
the shared ring (the one shared between tapdisk and the front-end). I don't
have the details of this change, but I don't see a reason why this shouldn't
work, or why it could be better compared to the blktap2 approach. Function
blkback_find_tapdisk is broken (see the FIXME in the code) works by chance,
once we decide on this architectural I'll fix it.

diff -r d332adcebe40 -r 407ebde55295 tools/blktap3/tapback/tapback.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/tapback/tapback.c	Fri Jan 04 12:10:37 2013 +0000
@@ -0,0 +1,286 @@
+/*
+ * Copyright (C) 2012      Citrix Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
+ * USA.
+ *
+ * This file contains the core of the tapback daemon, the user space daemon
+ * that acts as a device's back-end.
+ */
+
+/* 
+ * TODO Some of these includes may be useless.
+ * TODO Replace hard-coding strings with defines/const string.
+ */
+#include <stdlib.h>
+#include <stdarg.h>
+#include <assert.h>
+#include <syslog.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <libgen.h>
+#include <getopt.h>
+#include <sys/ioctl.h>
+#include <sys/mount.h>
+#include <syslog.h>
+
+#include "blktap3.h"
+#include "stdio.h" /* TODO tap-ctl.h needs to include stdio.h */
+#include "tap-ctl.h"
+#include "tapback.h"
+
+void tapback_log(int prio, const char *fmt, ...);
+void (*tapback_vlog) (int prio, const char *fmt, va_list ap);
+
+struct _blktap3_daemon blktap3_daemon;
+
+/**
+ * Read changes that occurred on the "backend/<backend name>" XenStore path
+ * or one of the front-end paths and act accordingly.
+ */
+static inline void
+tapback_read_watch(void)
+{
+    char **watch = NULL, *path = NULL, *token = NULL;
+    unsigned int n = 0;
+    int err = 0, _abort = 0;
+
+    /* read the change */
+    watch = xs_read_watch(blktap3_daemon.xs, &n);
+    path = watch[XS_WATCH_PATH];
+    token = watch[XS_WATCH_TOKEN];
+
+    /*
+     * TODO Put the body of "again:" into a loop instead of using goto.
+     */
+again:
+    if (!(blktap3_daemon.xst = xs_transaction_start(blktap3_daemon.xs))) {
+        WARN("error starting transaction\n");
+        goto fail;
+    }
+
+    /*
+     * The token indicates which XenStore watch triggered, the front-end one or
+     * the back-end one.
+     */
+    if (!strcmp(token, BLKTAP3_FRONTEND_TOKEN)) 
+        err = tapback_backend_handle_otherend_watch(path);
+    else if (!strcmp(token, BLKTAP3_BACKEND_TOKEN))
+        /*
+         * TODO The XenStore watch may trigger for a not yet
+         * discovered/understood reason.  The result is
+         * tapback_backend_handle_backend_watch not doing anything interesting,
+         * it only checks the 'tapback-serial'.
+         */
+        err = tapback_backend_handle_backend_watch(path);
+    else {
+        WARN("invalid token \'%s\'\n", token);
+        err = EINVAL;
+    }
+
+    _abort = !!err;
+    if (_abort)
+        DBG("aborting transaction: %s\n", strerror(-err));
+
+    err = xs_transaction_end(blktap3_daemon.xs, blktap3_daemon.xst, _abort);
+    blktap3_daemon.xst = 0;
+    if (!err) {
+        err = -errno;
+        /*
+         * This is OK according to xs_transaction_end's semantics.
+         */
+        if (EAGAIN == errno)
+            goto again;
+        DBG("error ending transaction: %s\n", strerror(err));
+    }
+
+fail:
+    free(watch);
+    return;
+}
+
+static void
+tapback_backend_destroy(void)
+{
+    if (blktap3_daemon.xs) {
+        xs_daemon_close(blktap3_daemon.xs);
+        blktap3_daemon.xs = NULL;
+    }
+}
+
+/**
+ * Initializes the back-end descriptor. There is one back-end per tapback
+ * process. Also, it initiates a watch to XenStore on backend/<backend name>.
+ *
+ * @returns 0 on success, an error code otherwise
+ */
+static inline int
+tapback_backend_create(void)
+{
+    int err = -EINVAL;    
+
+    TAILQ_INIT(&blktap3_daemon.devices);
+    blktap3_daemon.xst = XBT_NULL;
+
+    if (!(blktap3_daemon.xs = xs_daemon_open())) {
+        err = -EINVAL;
+        goto fail;
+    }
+
+    /*
+     * Watch the back-end.
+     */
+    if (!xs_watch(blktap3_daemon.xs, BLKTAP3_BACKEND_PATH,
+                BLKTAP3_BACKEND_TOKEN)) {
+        err = -errno;
+        goto fail;
+    }
+
+    return 0;
+
+fail:
+	tapback_backend_destroy();
+
+    return -err;
+}
+
+/**
+ * Runs the daemon.
+ *
+ * Watches backend/<backend name> and the front-end devices.
+ */
+static inline int
+tapback_backend_run(void)
+{
+    const int fd = xs_fileno(blktap3_daemon.xs);
+	int err;
+
+    do {
+        fd_set rfds;
+        int nfds = 0;
+
+        FD_ZERO(&rfds);
+        FD_SET(fd, &rfds);
+
+        /* poll the fd for changes in the XenStore path we're interested in */
+        if ((nfds = select(fd + 1, &rfds, NULL, NULL, NULL)) < 0) {
+            perror("error monitoring XenStore");
+            err = -errno;
+            break;
+        }
+
+        if (FD_ISSET(fd, &rfds))
+            tapback_read_watch();
+    } while (1);
+
+    return err;
+}
+
+static char *blkback_ident;
+
+static void
+blkback_vlog_fprintf(const int prio, const char * const fmt, va_list ap)
+{
+    const char *strprio[] = {
+        [LOG_DEBUG] = "DBG",
+        [LOG_INFO] = "INF",
+        [LOG_WARNING] = "WRN"
+    };
+
+    assert(prio >= 0);
+    BUG_ON(prio > sizeof(strprio) / sizeof(strprio[0]));
+    BUG_ON(!strprio[prio]);
+
+    fprintf(stderr, "%s[%s] ", blkback_ident, strprio[prio]);
+    vfprintf(stderr, fmt, ap);
+}
+
+/**
+ * Print tapback's usage instructions.
+ */
+static void
+usage(FILE * const stream, const char * const prog)
+{
+    assert(stream);
+    assert(prog);
+
+    fprintf(stream,
+            "usage: %s\n"
+            "\t[-D|--debug]\n"
+			"\t[-h|--help]\n", prog);
+}
+
+int main(int argc, char **argv)
+{
+    const char *prog = NULL;
+    int opt_debug = 0;
+    int err = 0;
+
+    prog = basename(argv[0]);
+
+    opt_debug = 0;
+
+    do {
+        const struct option longopts[] = {
+            {"help", 0, NULL, 'h'},
+            {"debug", 0, NULL, 'D'},
+        };
+        int c;
+
+        c = getopt_long(argc, argv, "h:D", longopts, NULL);
+        if (c < 0)
+            break;
+
+        switch (c) {
+        case 'h':
+            usage(stdout, prog);
+            return 0;
+        case 'D':
+            opt_debug = 1;
+            break;
+        case '?':
+            goto usage;
+        }
+    } while (1);
+
+    blkback_ident = BLKTAP3_BACKEND_TOKEN;
+    if (opt_debug)
+        tapback_vlog = blkback_vlog_fprintf;
+    else
+        openlog(blkback_ident, 0, LOG_DAEMON);
+
+    if (!opt_debug) {
+        if ((err = daemon(0, 0))) {
+            err = -errno;
+            goto fail;
+        }
+    }
+
+	if ((err = tapback_backend_create())) {
+        WARN("error creating blkback: %s\n", strerror(err));
+        goto fail;
+    }
+
+    err = tapback_backend_run();
+
+    tapback_backend_destroy();
+
+fail:
+    return err ? -err : 0;
+
+usage:
+    usage(stderr, prog);
+    return 1;
+}

[PATCH 7 of 7 v2] blktap3/tapback: Introduce tapback daemon Makefile

[Thanos Makatos]

This patch introduces the Makefile that builds the tapback daemon. This
Makefile is not yet hooked into the build system.

diff -r 407ebde55295 -r 84f9d3e16cf1 tools/blktap3/tapback/Makefile
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/blktap3/tapback/Makefile	Fri Jan 04 12:10:40 2013 +0000
@@ -0,0 +1,48 @@
+XEN_ROOT := $(CURDIR)/../../../
+include $(XEN_ROOT)/tools/Rules.mk
+
+BLKTAP_ROOT := ..
+
+INST_DIR ?= /usr/bin
+
+IBIN = tapback
+
+# -D_GNU_SOURCE is required by vasprintf.
+override CFLAGS += \
+	-I$(BLKTAP_ROOT)/include \
+	-I$(BLKTAP_ROOT)/control \
+	-D_GNU_SOURCE \
+    $(CFLAGS_libxenstore) \
+    $(CFLAGS_libxenctrl) \
+	$(CFLAGS_xeninclude) \
+    -Wall \
+    -Wextra \
+    -Werror
+
+# FIXME cause trouble
+override CFLAGS += \
+    -Wno-old-style-declaration \
+    -Wno-sign-compare \
+    -Wno-type-limits
+
+override LDFLAGS += \
+    $(LDLIBS_libxenstore) \
+    $(LDFLAGS_libxenctrl)
+
+TAPBACK-OBJS := log.o xenstore.o frontend.o backend.o
+
+TAPBACK-LIBS := $(BLKTAP_ROOT)/control/libblktapctl.so.1.0.0
+
+all: $(IBIN)
+
+$(IBIN): $(TAPBACK-OBJS) tapback.o
+	$(CC) -o $@ $^ $(TAPBACK-LIBS) $(LDFLAGS)
+
+install: all
+	$(INSTALL_DIR) -p $(DESTDIR)$(INST_DIR)
+	$(INSTALL_PROG) $(IBIN) $(DESTDIR)$(INST_DIR)
+
+clean:
+	rm -f *.o *.o.d .*.o.d $(IBIN)
+
+.PHONY: clean install

Re: [PATCH 1 of 7 v2] blktap3/tapback: Introduce core defines and structure definitions

[Ian Campbell]

On Fri, 2013-01-04 at 12:14 +0000, Thanos Makatos wrote:
> +#define BUG_ON(_cond)       \
> +    if (unlikely(_cond)) {  \
> +        assert(0);          \

This is basically a fancy way to write "assert(!_cond)" I think? I don't
imagine that the unlikely will make a measurable difference for you but
the downside is that a clever assert will print out the condition, so
you get
	assertion failed: 0
instead of 
	assertion failed: <_cond>

So you might almost as well use either abort() or	
	if (unlikely(_cond))
		assert(!_cond)

(evaluating cond twice won't hurt much under the circumstances, and the
compiler will probably optimise that anyway)

> +    }
> +
> +/*
> + * XenStore path components.
> + *
> + * To avoid confusion with blktap2, we'll use a new kind of device for libxl
> + * defining it in tools/libxl/libxl_types_internal.idl. This will be done by
> + * the patch that adds libxl support for blktap3. TODO When that patch is sent,
> + * use the definition from there instead of hard-coding it here.
> + */
> +#define XENSTORE_BACKEND        "backend"
> +#define BLKTAP3_BACKEND_NAME    "xenio"

tapback?

> +#define BLKTAP3_BACKEND_PATH    XENSTORE_BACKEND"/"BLKTAP3_BACKEND_NAME
> +#define BLKTAP3_BACKEND_TOKEN   XENSTORE_BACKEND"-"BLKTAP3_BACKEND_NAME
> +#define BLKTAP3_FRONTEND_TOKEN  "otherend-state"
> +#define BLKTAP3_SERIAL          BLKTAP3_BACKEND_NAME"-serial"

I'm not sure all of these are worth a #define, but I'm not yet sure what
they are used for.

> + * TODO Put the rest of the front-end nodes defined in blkif.h here and group
> + * them. e.g. FRONTEND_NODE_xxx.
> + */
> +#define RING_REF                "ring-ref"
> +#define FEAT_PERSIST            "feature-persistent"
> +/**
> + * Iterates over all devices and returns the one for which the condition is
> + * true.
> + */
> +#define tapback_backend_find_device(_device, _cond)		\
> +do {													\
> +	vbd_t *__next;								        \
> +	int found = 0;										\
> +	tapback_backend_for_each_device(_device, __next) {	\
> +		if (_cond) {									\
> +			found = 1;									\
> +			break;										\
> +		}												\
> +	}													\
> +	if (!found)											\
> +		_device = NULL;									\
> +} while (0)

Whitespace...

Re: [PATCH 2 of 7 v2] blktap3/tapback: Introduces functionality required to access XenStore

[Ian Campbell]

On Fri, 2013-01-04 at 12:14 +0000, Thanos Makatos wrote:
> This patch introduces convenience functions that read/write values from/to
> XenStore.
> 
> diff -r 0777319f0a6e -r efd3f343f7c7 tools/blktap3/tapback/xenstore.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/blktap3/tapback/xenstore.c	Fri Jan 04 11:54:51 2013 +0000
> @@ -0,0 +1,188 @@
> +/*
> + * Copyright (C) 2012      Citrix Ltd.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * 
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + * 
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
> + * USA.
> + */
> +
> +#include <stdarg.h>
> +#include <stdio.h>
> +#include <xenstore.h>
> +#include <assert.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include "blktap3.h"
> +#include "tapback.h"
> +#include "xenstore.h"
> +
> +/**
> + * Prints the supplied arguments to a buffer and returns it. The buffer must
> + * be deallocated by the user.

I'm not sure how much value this function has for callers over and above
just using (v)asprintf directly, likewise for mprintf.

> + */
> +static char *
> +vmprintf(const char * const fmt, va_list ap)
> +{
> +    char *s = NULL;

No need to init s.

> +
> +    if (vasprintf(&s, fmt, ap) < 0)
> +        s = NULL;
> +
> +    return s;
> +}
> +
> +__printf(1, 2)
> +char *
> +mprintf(const char * const fmt, ...)
> +{
> +    va_list ap;
> +    char *s = NULL;

No need to init s.

> +
> +    va_start(ap, fmt);
> +    s = vmprintf(fmt, ap);
> +    va_end(ap);
> +
> +    return s;
> +}
> +
> +char *
> +tapback_xs_vread(struct xs_handle * const xs, xs_transaction_t xst,
> +        const char * const fmt, va_list ap)

This might be an interesting/useful addition to libxenstore?

> +{
> +    char *path = NULL, *data = NULL;

No need to init path.

> +    unsigned int len = 0;
> +
> +    assert(xs);
> +
> +    path = vmprintf(fmt, ap);
> +    data = xs_read(xs, xst, path, &len);

This uses path without checking if it is NULL.

I think you can pass len == NULL if you don't care about the length of
the result.

However xenstore values generally can contain nulls and are not
necessarily NULL terminated (see docs/misc/xenstore.txt), but perhaps
the block protocol tightens this restriction such that you can rely on
NULL termination (at least in practice)?

> +    free(path);
> +
> +    return data;
> +}
> +
> +__printf(3, 4)
> +char *
> +tapback_xs_read(struct xs_handle * const xs, xs_transaction_t xst,
> +        const char * const fmt, ...)
> +{
> +    va_list ap;
> +    char *s = NULL;

No need to init s.

> +
> +    assert(xs);
> +
> +    va_start(ap, fmt);
> +    s = tapback_xs_vread(xs, xst, fmt, ap);
> +    va_end(ap);
> +
> +    return s;
> +}
> +

> +__scanf(3, 4)
> +int
> +tapback_device_scanf_otherend(vbd_t * const device,
> +        const char * const path, const char * const fmt, ...)
> +{
> +    va_list ap;
> +    int n = 0;
> +    char *s = NULL;
> +
> +    assert(device);
> +    assert(path);
> +
> +    if (!(s = tapback_device_read_otherend(device, path)))
> +        return -1;

Since s here is read from the frontend you probably can't trust it to be
NULL terminated.

> +    va_start(ap, fmt);
> +    n = vsscanf(s, fmt, ap);
> +    free(s);
> +    va_end(ap);
> +
> +    return n;
> +}
> +

Ian.

Re: [PATCH 4 of 7 v2] blktap3/tapback: Introduce back-end XenStore path handler

[Ian Campbell]

On Fri, 2013-01-04 at 12:14 +0000, Thanos Makatos wrote:
> This patch introduces the handler executed when the back-end XenStore path gets
> modified. A back-end XenStore path is modified as a result of a device
> creation/removal request. The device is created/removed depending on whether
> its path exists/does not exist in XenStore.
> 
> Creating a device comprises creating the in-memory representation of it and
> adding it to the list of devices, locating the tapdisk designated to serve
> this VBD, and setting a XenStore watch to the front-end path of the
> newly-created device.
> 
> Deleting a device comprises removing that XenStore watch and deallocating its
> in-memory representation.
> 
> The main issues in this patch are the following:
> 
> * Function tapback_backend_handle_backend_watch calls tapback_backend_scan
>   even when we know in which domain a device changed, wouldn't it make sense
>   from a performance perspective to only probe the devices of that domain
>   instead of probing ALL devices of ALL domains?

I suppose it would...

> * Is there a race condition between the tapback daemon and the toolstack
>   regarding partially brought up devices? I'm trying to figure out what the
>   "serial" thing does (check functions tapback_backend_create_device and
>   tapback_backend_probe_device) but I don't get it. Even if there is such a
>   race condition, I'm not convinced that the way that "serial" thing is
>   implemented fixes it.

There isn't a "serial thing" in the generic protocol so I don't know
what that is but normally the toolstack will write the entire initial
backend/frontend directories in xenstore in a single transaction.

Also I think it is customary for backends to ignore directories with no
"state" node -- and toolstacks write that last.

(Having now seen the serial stuff you refer I've no idea WTF it is
trying to do either ;-))

Re: [PATCH 5 of 7 v2] blktap3/tapback: Introduce front-end XenStore path handler

[Ian Campbell]

On Fri, 2013-01-04 at 12:14 +0000, Thanos Makatos wrote:
> +static inline int
> +blkback_frontend_changed(vbd_t * const xbdev, const XenbusState state)
> +{
> +    /*
> +     * XXX The size of the array (9) comes from the XenbusState enum.
> +     *
> +     * TODO Send a patch that adds XenbusStateMin, XenbusStateMax,
> +     * XenbusStateInvalid and in the XenbusState enum (located in xenbus.h).
> +     */
> +    struct frontend_state_change {
> +        int (*fn)(vbd_t * const, const XenbusState);
> +        const XenbusState state;

Is this the next backend state or the current or?...

> +    } static const frontend_state_change_map[] = {
> +        {NULL, 0},                                          /* Unknown */

	[XenbusStateUnknown] = {NULL,0},
	[XenbusStateInitialising] = {tapback_device_switch_state, XenbusStateInitWait},

? Bit less error prone.

> +        {tapback_device_switch_state, XenbusStateInitWait}, /* Initialising */
> +        {NULL, 0},                                          /* InitWait */
> +        {blkback_connect_tap, 0},                           /* Initialised */
> +        {blkback_connect_tap, 0},                           /* Connected */
> +        {tapback_device_switch_state, XenbusStateClosing},  /* Closing */
> +        {backend_close, 0},                                 /* StateClosed */
> +        {NULL, 0},                                          /* Reconfiguring */
> +        {NULL, 0}                                           /* Reconfigured */
> +    }; 

Re: [PATCH 7 of 7 v2] blktap3/tapback: Introduce tapback daemon Makefile

[Ian Campbell]

On Fri, 2013-01-04 at 12:14 +0000, Thanos Makatos wrote:
> This patch introduces the Makefile that builds the tapback daemon. This
> Makefile is not yet hooked into the build system.

At this point could I run make -C tools/blktap3/tapback and expect
something useful to happen? (Assuming I'd done the same for the
necessary prerequisite subdirs already).

> diff -r 407ebde55295 -r 84f9d3e16cf1 tools/blktap3/tapback/Makefile
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/blktap3/tapback/Makefile	Fri Jan 04 12:10:40 2013 +0000
> @@ -0,0 +1,48 @@
> +XEN_ROOT := $(CURDIR)/../../../
> +include $(XEN_ROOT)/tools/Rules.mk
> +
> +BLKTAP_ROOT := ..
> +
> +INST_DIR ?= /usr/bin

Please use whichever of the existing $(FOO_DIR) is relevant.

> +
> +IBIN = tapback
> +
> +# -D_GNU_SOURCE is required by vasprintf.
> +override CFLAGS += \
> +	-I$(BLKTAP_ROOT)/include \
> +	-I$(BLKTAP_ROOT)/control \
> +	-D_GNU_SOURCE \
> +    $(CFLAGS_libxenstore) \
> +    $(CFLAGS_libxenctrl) \
> +	$(CFLAGS_xeninclude) \
> +    -Wall \
> +    -Wextra \
> +    -Werror

Whitespace is all messed up here.

Ian.

Re: [PATCH 1 of 7 v2] blktap3/tapback: Introduce core defines and structure definitions

[Thanos Makatos]

> On Fri, 2013-01-04 at 12:14 +0000, Thanos Makatos wrote:
> > +#define BUG_ON(_cond)       \
> > +    if (unlikely(_cond)) {  \
> > +        assert(0);          \
> 
> This is basically a fancy way to write "assert(!_cond)" I think? I
> don't
> imagine that the unlikely will make a measurable difference for you but
> the downside is that a clever assert will print out the condition, so
> you get
> 	assertion failed: 0
> instead of
> 	assertion failed: <_cond>
> 
> So you might almost as well use either abort() or
> 	if (unlikely(_cond))
> 		assert(!_cond)
> 
> (evaluating cond twice won't hurt much under the circumstances, and the
> compiler will probably optimise that anyway)

This #define shouldn't be there in the first place (that's the kernel's way), assert(!_cond) should suffice.

> 
> > +    }
> > +
> > +/*
> > + * XenStore path components.
> > + *
> > + * To avoid confusion with blktap2, we'll use a new kind of device
> for libxl
> > + * defining it in tools/libxl/libxl_types_internal.idl. This will be
> done by
> > + * the patch that adds libxl support for blktap3. TODO When that
> patch is sent,
> > + * use the definition from there instead of hard-coding it here.
> > + */
> > +#define XENSTORE_BACKEND        "backend"
> > +#define BLKTAP3_BACKEND_NAME    "xenio"
> 
> tapback?

Xenio is the back-end's name (e.g "vbd" in blktap2), so it's shouldn't be strongly related to the daemon's name. In any case, at some point (hopefully ;)) blktap3 will become the default back-end and should use the "vbd" back-end name, so I don't think we should bother finding a pretty, temporary name for this.

> 
> > +#define BLKTAP3_BACKEND_PATH
> XENSTORE_BACKEND"/"BLKTAP3_BACKEND_NAME
> > +#define BLKTAP3_BACKEND_TOKEN   XENSTORE_BACKEND"-
> "BLKTAP3_BACKEND_NAME
> > +#define BLKTAP3_FRONTEND_TOKEN  "otherend-state"
> > +#define BLKTAP3_SERIAL          BLKTAP3_BACKEND_NAME"-serial"
> 
> I'm not sure all of these are worth a #define, but I'm not yet sure
> what
> they are used for.

They're used for querying and parsing XenStore paths. IMO it's better, from a defensive programming perspective, to avoid using the same string multiple times as it makes it harder to maintain the code.

> > +#define FEAT_PERSIST            "feature-persistent"
> > +/**
> > + * Iterates over all devices and returns the one for which the
> condition is
> > + * true.
> > + */
> > +#define tapback_backend_find_device(_device, _cond)		\
> > +do {
> 			\
> > +	vbd_t *__next;
> \
> > +	int found = 0;
> 		\
> > +	tapback_backend_for_each_device(_device, __next) {	\
> > +		if (_cond) {
> 		\
> > +			found = 1;
> 		\
> > +			break;
> 			\
> > +		}
> 			\
> > +	}
> 			\
> > +	if (!found)
> 		\
> > +		_device = NULL;
> 		\
> > +} while (0)
> 
> Whitespace...
> 
Fixed.

Re: [PATCH 2 of 7 v2] blktap3/tapback: Introduces functionality required to access XenStore

[Thanos Makatos]

> On Fri, 2013-01-04 at 12:14 +0000, Thanos Makatos wrote:
> > This patch introduces convenience functions that read/write values
> from/to
> > XenStore.
> >
> > diff -r 0777319f0a6e -r efd3f343f7c7 tools/blktap3/tapback/xenstore.c
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/tools/blktap3/tapback/xenstore.c	Fri Jan 04 11:54:51 2013
> +0000
> > @@ -0,0 +1,188 @@
> > +/*
> > + * Copyright (C) 2012      Citrix Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301,
> > + * USA.
> > + */
> > +
> > +#include <stdarg.h>
> > +#include <stdio.h>
> > +#include <xenstore.h>
> > +#include <assert.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +
> > +#include "blktap3.h"
> > +#include "tapback.h"
> > +#include "xenstore.h"
> > +
> > +/**
> > + * Prints the supplied arguments to a buffer and returns it. The
> buffer must
> > + * be deallocated by the user.
> 
> I'm not sure how much value this function has for callers over and
> above
> just using (v)asprintf directly, likewise for mprintf.

You're right they don't add anything, I'll remove them.

> 
> > + */
> > +static char *
> > +vmprintf(const char * const fmt, va_list ap)
> > +{
> > +    char *s = NULL;
> 
> No need to init s.

Regarding initialising variables, isn't it a good programming practice to initialise variables even if they'll be shortly assigned a value? Won't the compiler easily optimise such cases? Or does it make it too cumbersome to read? In any case, I checked the coding style document and it doesn't specify this, so I think it would be a meaningful addition.

> 
> > +
> > +    if (vasprintf(&s, fmt, ap) < 0)
> > +        s = NULL;
> > +
> > +    return s;
> > +}
> > +
> > +__printf(1, 2)
> > +char *
> > +mprintf(const char * const fmt, ...)
> > +{
> > +    va_list ap;
> > +    char *s = NULL;
> 
> No need to init s.
> 
> > +
> > +    va_start(ap, fmt);
> > +    s = vmprintf(fmt, ap);
> > +    va_end(ap);
> > +
> > +    return s;
> > +}
> > +
> > +char *
> > +tapback_xs_vread(struct xs_handle * const xs, xs_transaction_t xst,
> > +        const char * const fmt, va_list ap)
> 
> This might be an interesting/useful addition to libxenstore?

Sure.

> 
> > +{
> > +    char *path = NULL, *data = NULL;
> 
> No need to init path.
> 
> > +    unsigned int len = 0;
> > +
> > +    assert(xs);
> > +
> > +    path = vmprintf(fmt, ap);
> > +    data = xs_read(xs, xst, path, &len);
> 
> This uses path without checking if it is NULL.
> 
> I think you can pass len == NULL if you don't care about the length of
> the result.

Right.

> 
> However xenstore values generally can contain nulls and are not
> necessarily NULL terminated (see docs/misc/xenstore.txt), but perhaps
> the block protocol tightens this restriction such that you can rely on
> NULL termination (at least in practice)?

Hm.. I saw this in tools/xenstore/xenstore.h and I though xs_read does end the string with a NULL:

/* Get the value of a single file, nul terminated.
 * Returns a malloced value: call free() on it after use.
 * len indicates length in bytes, not including terminator.
 */
void *xs_read(struct xs_handle *h, xs_transaction_t t,
	      const char *path, unsigned int *len);

Am I missing something?

> 
> > +    free(path);
> > +
> > +    return data;
> > +}
> > +
> > +__printf(3, 4)
> > +char *
> > +tapback_xs_read(struct xs_handle * const xs, xs_transaction_t xst,
> > +        const char * const fmt, ...)
> > +{
> > +    va_list ap;
> > +    char *s = NULL;
> 
> No need to init s.
> 
> > +
> > +    assert(xs);
> > +
> > +    va_start(ap, fmt);
> > +    s = tapback_xs_vread(xs, xst, fmt, ap);
> > +    va_end(ap);
> > +
> > +    return s;
> > +}
> > +
> 
> > +__scanf(3, 4)
> > +int
> > +tapback_device_scanf_otherend(vbd_t * const device,
> > +        const char * const path, const char * const fmt, ...)
> > +{
> > +    va_list ap;
> > +    int n = 0;
> > +    char *s = NULL;
> > +
> > +    assert(device);
> > +    assert(path);
> > +
> > +    if (!(s = tapback_device_read_otherend(device, path)))
> > +        return -1;
> 
> Since s here is read from the frontend you probably can't trust it to
> be
> NULL terminated.
> 
> > +    va_start(ap, fmt);
> > +    n = vsscanf(s, fmt, ap);
> > +    free(s);
> > +    va_end(ap);
> > +
> > +    return n;
> > +}
> > +
> 
> Ian.

Re: [PATCH 2 of 7 v2] blktap3/tapback: Introduces functionality required to access XenStore

[Ian Campbell]

On Fri, 2013-01-25 at 14:03 +0000, Thanos Makatos wrote:

> > > + */
> > > +static char *
> > > +vmprintf(const char * const fmt, va_list ap)
> > > +{
> > > +    char *s = NULL;
> > 
> > No need to init s.
> 
> Regarding initialising variables, isn't it a good programming practice
> to initialise variables even if they'll be shortly assigned a value?
> Won't the compiler easily optimise such cases?

It should but that's not why we avoid them.

>  Or does it make it too cumbersome to read?

The problem with "unnecessary" initialisations is that the prevent the
compiler's checks for use of uninitialised variables from functioning as
intended and catching bugs.

This is obviously a very simple contrived case but e.g. if you start
with:
	int x = 0, y = 0;

	x = get_a_value();
	y = another_value();

	use_some_values(x, y)
and someone adds a new case:
	int x = 0, y = 0;

	if (some_condition)
		x = get_a_value();
	else
		y = get_an_alternative_value();
	y = another_value();

	use_some_values(x, y);

But they've accidentally used the wrong variable in the new assignment!
So x is actually uninitialised, although it has been set arbitrarily to
0. This is the sort of thing which is easily missed (especially in more
complex example with multiple code paths etc)

If you just had "int x, y;" this would have resulted in "Variable x used
without being initialised" from the compiler.

This doesn't apply in all cases, and obviously it something of a matter
of taste/judgement, but there we go ;-)

> > However xenstore values generally can contain nulls and are not
> > necessarily NULL terminated (see docs/misc/xenstore.txt), but perhaps
> > the block protocol tightens this restriction such that you can rely on
> > NULL termination (at least in practice)?
> 
> Hm.. I saw this in tools/xenstore/xenstore.h and I though xs_read does end the string with a NULL:
> 
> /* Get the value of a single file, nul terminated.
>  * Returns a malloced value: call free() on it after use.
>  * len indicates length in bytes, not including terminator.
>  */
> void *xs_read(struct xs_handle *h, xs_transaction_t t,
> 	      const char *path, unsigned int *len);
> 
> Am I missing something?

I'm not sure, this isn't consistent with the wire protocol doc (at least
AIUI), but perhaps the library is just hiding some complexity for you.
This is one for Ian J I think.

Ian.

Re: [PATCH 4 of 7 v2] blktap3/tapback: Introduce back-end XenStore path handler

[Thanos Makatos]

> >
> > * Function tapback_backend_handle_backend_watch calls
> tapback_backend_scan
> >   even when we know in which domain a device changed, wouldn't it
> make sense
> >   from a performance perspective to only probe the devices of that
> domain
> >   instead of probing ALL devices of ALL domains?
> 
> I suppose it would...

The question is, could we leave this for later as an optimisation or should it be done in this patch series?

> 
> > * Is there a race condition between the tapback daemon and the
> toolstack
> >   regarding partially brought up devices? I'm trying to figure out
> what the
> >   "serial" thing does (check functions tapback_backend_create_device
> and
> >   tapback_backend_probe_device) but I don't get it. Even if there is
> such a
> >   race condition, I'm not convinced that the way that "serial" thing
> is
> >   implemented fixes it.
> 
> There isn't a "serial thing" in the generic protocol so I don't know
> what that is but normally the toolstack will write the entire initial
> backend/frontend directories in xenstore in a single transaction.
> 
> Also I think it is customary for backends to ignore directories with no
> "state" node -- and toolstacks write that last.
> 
> (Having now seen the serial stuff you refer I've no idea WTF it is
> trying to do either ;-))
> 

Then I suppose it's ok to completely remove this thing.

Re: [PATCH 4 of 7 v2] blktap3/tapback: Introduce back-end XenStore path handler

[Ian Campbell]

On Fri, 2013-01-25 at 14:31 +0000, Thanos Makatos wrote:
> > >
> > > * Function tapback_backend_handle_backend_watch calls
> > tapback_backend_scan
> > >   even when we know in which domain a device changed, wouldn't it
> > make sense
> > >   from a performance perspective to only probe the devices of that
> > domain
> > >   instead of probing ALL devices of ALL domains?
> > 
> > I suppose it would...
> 
> The question is, could we leave this for later as an optimisation or
> should it be done in this patch series?

I'm happy to leave that up to you.

Ian.

Re: [PATCH 5 of 7 v2] blktap3/tapback: Introduce front-end XenStore path handler

[Thanos Makatos]

> On Fri, 2013-01-04 at 12:14 +0000, Thanos Makatos wrote:
> > +static inline int
> > +blkback_frontend_changed(vbd_t * const xbdev, const XenbusState
> state)
> > +{
> > +    /*
> > +     * XXX The size of the array (9) comes from the XenbusState
> enum.
> > +     *
> > +     * TODO Send a patch that adds XenbusStateMin, XenbusStateMax,
> > +     * XenbusStateInvalid and in the XenbusState enum (located in
> xenbus.h).
> > +     */
> > +    struct frontend_state_change {
> > +        int (*fn)(vbd_t * const, const XenbusState);
> > +        const XenbusState state;
> 
> Is this the next backend state or the current or?...
> 
> > +    } static const frontend_state_change_map[] = {
> > +        {NULL, 0},                                          /*
> Unknown */
> 
> 	[XenbusStateUnknown] = {NULL,0},
> 	[XenbusStateInitialising] = {tapback_device_switch_state,
> XenbusStateInitWait},
> 
> ? Bit less error prone.
> 
> > +        {tapback_device_switch_state, XenbusStateInitWait}, /*
> Initialising */
> > +        {NULL, 0},                                          /*
> InitWait */
> > +        {blkback_connect_tap, 0},                           /*
> Initialised */
> > +        {blkback_connect_tap, 0},                           /*
> Connected */
> > +        {tapback_device_switch_state, XenbusStateClosing},  /*
> Closing */
> > +        {backend_close, 0},                                 /*
> StateClosed */
> > +        {NULL, 0},                                          /*
> Reconfiguring */
> > +        {NULL, 0}                                           /*
> Reconfigured */
> > +    };

The position of each element in the frontend_state_change_map array reflects the state into which the front-end just switched. Each element contains a call-back function to be executed in response, and an optional state for the back-end to switch to. Does this make sense? Actually, there was a switch statement and I replaced it with this in order to make it less error-prone!

Re: [PATCH 5 of 7 v2] blktap3/tapback: Introduce front-end XenStore path handler

[Ian Campbell]

On Fri, 2013-01-25 at 15:09 +0000, Thanos Makatos wrote:
> > On Fri, 2013-01-04 at 12:14 +0000, Thanos Makatos wrote:
> > > +static inline int
> > > +blkback_frontend_changed(vbd_t * const xbdev, const XenbusState
> > state)
> > > +{
> > > +    /*
> > > +     * XXX The size of the array (9) comes from the XenbusState
> > enum.
> > > +     *
> > > +     * TODO Send a patch that adds XenbusStateMin, XenbusStateMax,
> > > +     * XenbusStateInvalid and in the XenbusState enum (located in
> > xenbus.h).
> > > +     */
> > > +    struct frontend_state_change {
> > > +        int (*fn)(vbd_t * const, const XenbusState);
> > > +        const XenbusState state;
> > 
> > Is this the next backend state or the current or?...
> > 
> > > +    } static const frontend_state_change_map[] = {
> > > +        {NULL, 0},                                          /*
> > Unknown */
> > 
> > 	[XenbusStateUnknown] = {NULL,0},
> > 	[XenbusStateInitialising] = {tapback_device_switch_state,
> > XenbusStateInitWait},
> > 
> > ? Bit less error prone.
> > 
> > > +        {tapback_device_switch_state, XenbusStateInitWait}, /*
> > Initialising */
> > > +        {NULL, 0},                                          /*
> > InitWait */
> > > +        {blkback_connect_tap, 0},                           /*
> > Initialised */
> > > +        {blkback_connect_tap, 0},                           /*
> > Connected */
> > > +        {tapback_device_switch_state, XenbusStateClosing},  /*
> > Closing */
> > > +        {backend_close, 0},                                 /*
> > StateClosed */
> > > +        {NULL, 0},                                          /*
> > Reconfiguring */
> > > +        {NULL, 0}                                           /*
> > Reconfigured */
> > > +    };
> 
> The position of each element in the frontend_state_change_map array
> reflects the state into which the front-end just switched. Each
> element contains a call-back function to be executed in response, and
> an optional state for the back-end to switch to. Does this make sense?

Absolutely.

>  Actually, there was a switch statement and I replaced it with this in
>  order to make it less error-prone!

The switch is a pretty common idiom in xenbus state machines but this
works too if you prefer it.

My suggestion is just to use:
	[XenbusStateInitialising] = {tapback_device_switch_state, XenbusStateInitWait},
(so, when f.e. goes to Initialising, call tapback_... and switch to InitWait).

instead of relying on the order of the array, otherwise you have to be
careful to get thing in the correct slot in the array.


Ian.