blktap3: Introduce the tapback daemon (most of blkback in user-space).
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.
PATCH v2 1/1 blktap3/tapback: Introduce core defines and structure definitions
tools/blktap3/tapback/tapback.h new file +253 -0
@@ -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__ */
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.hhere 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...
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()orif (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 bedone 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_PATHXENSTORE_BACKEND"/"BLKTAP3_BACKEND_NAME
Quoted 1 line
+#define
BLKTAP3_BACKEND_TOKENXENSTORE_BACKEND"-"
BLKTAP3_BACKEND_NAMEtools/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.
PATCH v2 1/1 blktap3/tapback: Introduces functionality required to access XenStore
In reply to Thanos MakatosThis patch introduces convenience functions that read/write values from/to XenStore.
tools/blktap3/tapback/xenstore.c new file +188 -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> +#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
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-refd3f343f7c7tools/blktap3/tapback/xenstore.ctools/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.
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-refd3f343f7c7tools/blktap3/tapback/xenstore.ctools/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.
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.hand I thoughxs_readdoes end the string with a NULL:
Quoted 8 lines
/* Get the value of a single file, nul terminated.
* Returns a malloced value: callfree()on it after use.
* len indicates length in bytes, not including terminator. */
void *xs_read(structxs_handle*h,xs_transaction_tt,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.
PATCH v2 1/1 blktap3/tapback: Logging for the tapback daemon and libxenio
In reply to Thanos Makatostools/blktap3/tapback/log.c new file +33 -0
@@ -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 v2 1/1 blktap3/tapback: Introduce back-end XenStore path handler
In reply to Thanos MakatosThis 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
@@ -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); +}
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_watchcallstapback_backend_scaneven 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_deviceandtapback_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 4 of 7 v2] blktap3/tapback: Introduce back-end XenStore path handler
Quoted 10 lines
Quoted 2 lines
* Function
tapback_backend_handle_backend_watchcalls
tapback_backend_scanQuoted 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_deviceand
Quoted 1 line
tapback_backend_probe_device) but I don't get it. Even if there issuch 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.
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_watchcalls
tapback_backend_scanQuoted 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.
PATCH v2 1/1 blktap3/tapback: Introduce front-end XenStore path handler
In reply to Thanos MakatosThis 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
@@ -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
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 */ + };
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 XenbusStatestate)
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!
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 XenbusStatestate)
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_maparray 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.
PATCH v2 1/1 blktap3/tapback: Introduce the tapback daemon
In reply to Thanos MakatosThis 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
@@ -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 v2 1/1 blktap3/tapback: Introduce tapback daemon Makefile
In reply to Thanos MakatosThis 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
@@ -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 7 of 7 v2] blktap3/tapback: Introduce tapback daemon Makefile
Quoted 2 lines · Thanos Makatos
This patch introduces the
Makefilethat builds the tapback daemon. ThisMakefileis 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-r84f9d3e16cf1tools/blktap3/tapback/Makefiletools/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.