Skip to contentSkip to conversation

blktap3: Introduce the tapback daemon (most of blkback in user-space).

PATCH Superseded Thanos Makatos 2 participants 20 messages Jan 4, 2013 – Jan 25, 2013

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 blktap3: Introduce the tapback daemon (most of blkback in user-space).

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.

#1 lore raw

PATCH v2 1/1 blktap3/tapback: Introduce core defines and structure definitions

tools/blktap3/tapback/tapback.h new file +253 -0
diff -r fbf47a5d98ff -r 0777319f0a6e tools/blktap3/tapback/tapback.h
@@ -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__ */
#2 lore raw

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

tools/blktap3/tapback/tapback.h line 53

Quoted 3 lines · Thanos Makatos
tools/blktap3/tapback/tapback.h new file +3 -0
+#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)

tools/blktap3/tapback/tapback.h line 65

Quoted 12 lines
tools/blktap3/tapback/tapback.h new file +12 -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"

tapback?

tools/blktap3/tapback/tapback.h line 69

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

Quoted 12 lines

+ * TODO Put the rest of the front-end nodes defined in blkif.h here and group
+ * them. e.g. FRONTEND_NODE_xxx.
+ */

tools/blktap3/tapback/tapback.h new file +9 -0
+#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;								        \

tools/blktap3/tapback/tapback.h line 223

Quoted 10 lines
tools/blktap3/tapback/tapback.h new file +10 -0
+	int found = 0;										\
+	tapback_backend_for_each_device(_device, __next) {	\
+		if (_cond) {									\
+			found = 1;									\
+			break;										\
+		}												\
+	}													\
+	if (!found)											\
+		_device = NULL;									\
+} while (0)

Whitespace...

#3 lore raw

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

Quoted 14 lines · Thanos Makatos

tools/blktap3/tapback/tapback.h line 53

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

Quoted 6 lines

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.

Quoted 10 lines

tools/blktap3/tapback/tapback.h

Quoted 7 lines
tools/blktap3/tapback/tapback.h new file +6 -0
+    }
+
+/*
+ * XenStore path components.
+ *
+ * To avoid confusion with blktap2, we'll use a new kind of device

for libxl

Quoted 1 line

+ * defining it in tools/libxl/libxl_types_internal.idl. This will be

done by

Quoted 8 lines
Quoted 1 line

+ * the patch that adds libxl support for blktap3. TODO When that

patch is sent,

tools/blktap3/tapback/tapback.h line 65

Quoted 4 lines

+ * use the definition from there instead of hard-coding it here.
+ */

tools/blktap3/tapback/tapback.h new file +2 -0
+#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.

Quoted 11 lines
Quoted 2 lines

+#define BLKTAP3_BACKEND_PATH

XENSTORE_BACKEND"/"BLKTAP3_BACKEND_NAME

Quoted 1 line

+#define BLKTAP3_BACKEND_TOKEN XENSTORE_BACKEND"-

"BLKTAP3_BACKEND_NAME

tools/blktap3/tapback/tapback.h line 69

Quoted 2 lines
tools/blktap3/tapback/tapback.h new file +2 -0
+#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.

Quoted 24 lines

tools/blktap3/tapback/tapback.h

Quoted 3 lines
tools/blktap3/tapback/tapback.h new file +3 -0
+#define FEAT_PERSIST            "feature-persistent"
+/**
+ * Iterates over all devices and returns the one for which the

condition is

Review

Quoted 4 lines

+ * true.
+ */

(quoted diff) +2 -0
+#define tapback_backend_find_device(_device, _cond)		\
+do {
			\

Review

Quoted 1 line
(quoted diff) +1 -0
+	vbd_t *__next;

\

Review

Quoted 1 line
(quoted diff) +1 -0
+	int found = 0;
		\

Review

Quoted 2 lines
(quoted diff) +2 -0
+	tapback_backend_for_each_device(_device, __next) {	\
+		if (_cond) {
		\

Review

Quoted 1 line
(quoted diff) +1 -0
+			found = 1;
		\

Review

Quoted 1 line
(quoted diff) +1 -0
+			break;
			\

Review

Quoted 1 line
(quoted diff) +1 -0
+		}
			\

Review

Quoted 1 line
(quoted diff) +1 -0
+	}
			\
Quoted 8 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+	if (!found)
		\

Review

Quoted 1 line
(quoted diff) +1 -0
+		_device = NULL;
		\
Quoted 1 line

+} while (0)

Whitespace...

Fixed.

#4 lore raw

PATCH v2 1/1 blktap3/tapback: Introduces functionality required to access XenStore

In reply to Thanos Makatos

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

tools/blktap3/tapback/xenstore.c new file +188 -0
diff -r 0777319f0a6e -r efd3f343f7c7 tools/blktap3/tapback/xenstore.c
@@ -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;
+}
tools/blktap3/tapback/xenstore.h new file +103 -0

103 lines not on this page · raw message

#5 lore raw

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

Quoted 30 lines · Thanos Makatos

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

diff -r 0777319f0a6e -r efd3f343f7c7 tools/blktap3/tapback/xenstore.c

tools/blktap3/tapback/xenstore.c new file +23 -0
@@ -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>

tools/blktap3/tapback/xenstore.c line 33

Quoted 10 lines
tools/blktap3/tapback/xenstore.c new file +10 -0
+#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.

tools/blktap3/tapback/xenstore.c line 38

Quoted 5 lines
tools/blktap3/tapback/xenstore.c new file +5 -0
+ */
+static char *
+vmprintf(const char * const fmt, va_list ap)
+{
+    char *s = NULL;

No need to init s.

tools/blktap3/tapback/xenstore.c line 51

Quoted 13 lines
tools/blktap3/tapback/xenstore.c new file +13 -0
+
+    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.

tools/blktap3/tapback/xenstore.c line 62

Quoted 11 lines
tools/blktap3/tapback/xenstore.c new file +11 -0
+
+    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?

tools/blktap3/tapback/xenstore.c line 64

Quoted 2 lines
tools/blktap3/tapback/xenstore.c new file +2 -0
+{
+    char *path = NULL, *data = NULL;

No need to init path.

tools/blktap3/tapback/xenstore.c line 70

Quoted 6 lines
tools/blktap3/tapback/xenstore.c new file +6 -0
+    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)?

tools/blktap3/tapback/xenstore.c line 82

Quoted 12 lines
tools/blktap3/tapback/xenstore.c new file +12 -0
+    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.

Quoted 15 lines
tools/blktap3/tapback/xenstore.c new file +14 -0
+
+    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, ...)

tools/blktap3/tapback/xenstore.c line 127

Quoted 10 lines
tools/blktap3/tapback/xenstore.c new file +10 -0
+{
+    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.

tools/blktap3/tapback/xenstore.c line 135

Quoted 8 lines
tools/blktap3/tapback/xenstore.c new file +8 -0
+    va_start(ap, fmt);
+    n = vsscanf(s, fmt, ap);
+    free(s);
+    va_end(ap);
+
+    return n;
+}
+

Ian.

#6 lore raw

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

Quoted 44 lines · Thanos Makatos
Quoted 1 line

This patch introduces convenience functions that read/write values

from/to

tools/blktap3/tapback/xenstore.c

Quoted 5 lines

XenStore.

diff -r 0777319f0a6e -r efd3f343f7c7 tools/blktap3/tapback/xenstore.c

tools/blktap3/tapback/xenstore.c new file +0 -0

+0000

Quoted 7 lines
tools/blktap3/tapback/xenstore.c new file +6 -0
@@ -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

tools/blktap3/tapback/xenstore.c line 16

Quoted 10 lines
tools/blktap3/tapback/xenstore.c new file +10 -0
+ * 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,

Quoted 6 lines
tools/blktap3/tapback/xenstore.c new file +6 -0
+ * USA.
+ */
+
+#include <stdarg.h>
+#include <stdio.h>
+#include <xenstore.h>

tools/blktap3/tapback/xenstore.c

Quoted 10 lines
tools/blktap3/tapback/xenstore.c new file +10 -0
+#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

Quoted 5 lines

tools/blktap3/tapback/xenstore.c line 33

Quoted 1 line
tools/blktap3/tapback/xenstore.c new file +1 -0
+ * 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.

Quoted 8 lines

tools/blktap3/tapback/xenstore.c line 38

Quoted 6 lines
tools/blktap3/tapback/xenstore.c new file +5 -0
+ */
+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.

Quoted 17 lines
Quoted 4 lines
tools/blktap3/tapback/xenstore.c new file +3 -0
+
+    if (vasprintf(&s, fmt, ap) < 0)
+        s = NULL;

tools/blktap3/tapback/xenstore.c line 51

Quoted 10 lines
tools/blktap3/tapback/xenstore.c new file +10 -0
+
+    return s;
+}
+
+__printf(1, 2)
+char *
+mprintf(const char * const fmt, ...)
+{
+    va_list ap;
+    char *s = NULL;

No need to init s.

Quoted 13 lines

tools/blktap3/tapback/xenstore.c line 62

Quoted 11 lines
tools/blktap3/tapback/xenstore.c new file +11 -0
+
+    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.

Quoted 15 lines

tools/blktap3/tapback/xenstore.c line 64

Quoted 3 lines
tools/blktap3/tapback/xenstore.c new file +2 -0
+{
+    char *path = NULL, *data = NULL;

No need to init path.

tools/blktap3/tapback/xenstore.c line 70

Quoted 6 lines
tools/blktap3/tapback/xenstore.c new file +6 -0
+    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.

Quoted 2 lines

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

Right.

Quoted 5 lines

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?

Quoted 56 lines

tools/blktap3/tapback/xenstore.c line 82

Quoted 13 lines
tools/blktap3/tapback/xenstore.c new file +12 -0
+    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.

Quoted 15 lines
tools/blktap3/tapback/xenstore.c new file +14 -0
+
+    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, ...)

tools/blktap3/tapback/xenstore.c line 127

Quoted 10 lines
tools/blktap3/tapback/xenstore.c new file +10 -0
+{
+    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.

tools/blktap3/tapback/xenstore.c line 135

Quoted 8 lines
tools/blktap3/tapback/xenstore.c new file +8 -0
+    va_start(ap, fmt);
+    n = vsscanf(s, fmt, ap);
+    free(s);
+    va_end(ap);
+
+    return n;
+}
+

Ian.

#7 lore raw

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

Quoted 11 lines · Thanos Makatos
Quoted 7 lines

tools/blktap3/tapback/xenstore.c line 38

Quoted 5 lines

+ */

tools/blktap3/tapback/xenstore.c new file +4 -0
+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.

Quoted 1 line

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 ;-)

Quoted 7 lines
Quoted 4 lines

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:

Quoted 8 lines

/* 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.

#8 lore raw

PATCH v2 1/1 blktap3/tapback: Logging for the tapback daemon and libxenio

In reply to Thanos Makatos
tools/blktap3/tapback/log.c new file +33 -0
diff -r efd3f343f7c7 -r a9beb303b541 tools/blktap3/tapback/log.c
@@ -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);
+}
#9 lore raw

PATCH v2 1/1 blktap3/tapback: Introduce back-end XenStore path handler

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

tools/blktap3/tapback/backend.c new file +434 -0
diff -r a9beb303b541 -r 8094db37a249 tools/blktap3/tapback/backend.c
@@ -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);
+}
#10 lore raw

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

Quoted 13 lines · 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.

Quoted 6 lines

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

Quoted 6 lines

* 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 ;-))

#11 lore raw

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

Quoted 10 lines
Quoted 2 lines

* Function tapback_backend_handle_backend_watch calls

tapback_backend_scan

Quoted 1 line

even when we know in which domain a device changed, wouldn't it

make sense

Quoted 1 line

from a performance perspective to only probe the devices of that

domain

Quoted 1 line

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?

Quoted 17 lines
Quoted 2 lines

* Is there a race condition between the tapback daemon and the

toolstack

Quoted 1 line

regarding partially brought up devices? I'm trying to figure out

what the

Quoted 1 line

"serial" thing does (check functions tapback_backend_create_device

and

Quoted 1 line

tapback_backend_probe_device) but I don't get it. Even if there is

such a

Quoted 1 line

race condition, I'm not convinced that the way that "serial" thing

is

Quoted 1 line

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.

Quoted 6 lines

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.

#12 lore raw

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

Quoted 5 lines · Thanos Makatos
Quoted 5 lines
Quoted 2 lines

* Function tapback_backend_handle_backend_watch calls

tapback_backend_scan

Quoted 1 line

even when we know in which domain a device changed, wouldn't it

make sense

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

from a performance perspective to only probe the devices of that

domain

Quoted 1 line

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.

#13 lore raw

PATCH v2 1/1 blktap3/tapback: Introduce front-end XenStore path handler

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

tools/blktap3/include/blktap3.h +15 -0
diff -r 8094db37a249 -r d332adcebe40 tools/blktap3/include/blktap3.h
@@ -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__ */
tools/blktap3/tapback/frontend.c new file +396 -0

396 lines not on this page · raw message

#14 lore raw

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

tools/blktap3/tapback/frontend.c line 321

Quoted 12 lines · Thanos Makatos
tools/blktap3/tapback/frontend.c new file +12 -0
+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?...

tools/blktap3/tapback/frontend.c line 323

Quoted 2 lines
tools/blktap3/tapback/frontend.c new file +2 -0
+    } static const frontend_state_change_map[] = {
+        {NULL, 0},                                          /* Unknown */

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

? Bit less error prone.

Quoted 9 lines
tools/blktap3/tapback/frontend.c new file +9 -0
+        {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 */
+    }; 
#15 lore raw

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

Quoted 37 lines · Thanos Makatos

Review

Quoted 2 lines
(quoted diff) +2 -0
+static inline int
+blkback_frontend_changed(vbd_t * const xbdev, const XenbusState

state)

tools/blktap3/tapback/frontend.c

Quoted 3 lines
tools/blktap3/tapback/frontend.c new file +3 -0
+{
+    /*
+     * XXX The size of the array (9) comes from the XenbusState

enum.

tools/blktap3/tapback/frontend.c

Quoted 3 lines
tools/blktap3/tapback/frontend.c new file +3 -0
+     *
+     * TODO Send a patch that adds XenbusStateMin, XenbusStateMax,
+     * XenbusStateInvalid and in the XenbusState enum (located in

xenbus.h).

tools/blktap3/tapback/frontend.c line 321

Quoted 4 lines
tools/blktap3/tapback/frontend.c new file +4 -0
+     */
+    struct frontend_state_change {
+        int (*fn)(vbd_t * const, const XenbusState);
+        const XenbusState state;

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

Review

Quoted 2 lines
(quoted diff) +2 -0
+    } static const frontend_state_change_map[] = {
+        {NULL, 0},                                          /*

Unknown */

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

? Bit less error prone.

Quoted 1 line

+ {tapback_device_switch_state, XenbusStateInitWait}, /*

Initialising */

Quoted 1 line

+ {NULL, 0}, /*

InitWait */

Quoted 1 line

+ {blkback_connect_tap, 0}, /*

Initialised */

Quoted 1 line

+ {blkback_connect_tap, 0}, /*

Connected */

Quoted 9 lines
Quoted 1 line

+ {tapback_device_switch_state, XenbusStateClosing}, /*

Closing */

Quoted 1 line

+ {backend_close, 0}, /*

StateClosed */

Quoted 1 line

+ {NULL, 0}, /*

Reconfiguring */

Quoted 1 line

+ {NULL, 0} /*

Reconfigured */

Quoted 1 line

+ };

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!

#16 lore raw

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

Quoted 43 lines · Thanos Makatos
Quoted 43 lines · Thanos Makatos

Review

Quoted 2 lines
(quoted diff) +2 -0
+static inline int
+blkback_frontend_changed(vbd_t * const xbdev, const XenbusState

state)

tools/blktap3/tapback/frontend.c

Quoted 3 lines
tools/blktap3/tapback/frontend.c new file +3 -0
+{
+    /*
+     * XXX The size of the array (9) comes from the XenbusState

enum.

tools/blktap3/tapback/frontend.c

Quoted 3 lines
tools/blktap3/tapback/frontend.c new file +3 -0
+     *
+     * TODO Send a patch that adds XenbusStateMin, XenbusStateMax,
+     * XenbusStateInvalid and in the XenbusState enum (located in

xenbus.h).

tools/blktap3/tapback/frontend.c line 321

Quoted 4 lines
tools/blktap3/tapback/frontend.c new file +4 -0
+     */
+    struct frontend_state_change {
+        int (*fn)(vbd_t * const, const XenbusState);
+        const XenbusState state;

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

Review

Quoted 2 lines
(quoted diff) +2 -0
+    } static const frontend_state_change_map[] = {
+        {NULL, 0},                                          /*

Unknown */

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

? Bit less error prone.

Quoted 1 line

+ {tapback_device_switch_state, XenbusStateInitWait}, /*

Initialising */

Quoted 1 line

+ {NULL, 0}, /*

InitWait */

Quoted 1 line

+ {blkback_connect_tap, 0}, /*

Initialised */

Quoted 1 line

+ {blkback_connect_tap, 0}, /*

Connected */

Quoted 1 line

+ {tapback_device_switch_state, XenbusStateClosing}, /*

Closing */

Quoted 1 line

+ {backend_close, 0}, /*

StateClosed */

Quoted 1 line

+ {NULL, 0}, /*

Reconfiguring */

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

+ {NULL, 0} /*

Reconfigured */

Quoted 1 line

+ };

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.

Quoted 2 lines

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.

#17 lore raw

PATCH v2 1/1 blktap3/tapback: Introduce the tapback daemon

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

tools/blktap3/tapback/tapback.c new file +286 -0
diff -r d332adcebe40 -r 407ebde55295 tools/blktap3/tapback/tapback.c
@@ -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;
+}
#18 lore raw

PATCH v2 1/1 blktap3/tapback: Introduce tapback daemon Makefile

In reply to Thanos Makatos

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

tools/blktap3/tapback/Makefile new file +48 -0
diff -r 407ebde55295 -r 84f9d3e16cf1 tools/blktap3/tapback/Makefile
@@ -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
#19 lore raw

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

Quoted 2 lines · Thanos Makatos

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

tools/blktap3/tapback/Makefile line 6

Quoted 10 lines

diff -r 407ebde55295 -r 84f9d3e16cf1 tools/blktap3/tapback/Makefile

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

Quoted 4 lines
tools/blktap3/tapback/Makefile new file +4 -0
+
+IBIN = tapback
+
+# -D_GNU_SOURCE is required by vasprintf.

tools/blktap3/tapback/Makefile line 20

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