|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] RFC: blktap3
On Thu, 2012-08-09 at 15:03 +0100, Thanos Makatos wrote:
> Iâd like to introduce blktap3: essentially blktap2 without the need of
> blkback. This has been developed by Santosh Jodh, and Iâll maintain
> it.
I think you are working on reposting in a more manageable form but
here's a few things which I noticed on a top level scroll though. (I
might be repeating myself occasionally from the quick comments I made
earlier, sorry)
> diff --git a/tools/Makefile b/tools/Makefile
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -201,3 +203,20 @@
>
> subdir-distclean-firmware: .phony
> $(MAKE) -C firmware distclean
> +
> +subdir-all-blktap3 subdir-install-blktap3: .phony
> + source=.; \
> + cd blktap3; \
> + ./autogen.sh; \
If anything this should be called from the top-level ./autogen.sh and
not here. We shouldn't expect end users to have autoconf available.
> + ./configure \
I think autoconf has a construct which can cause configure to call other
sub-configures in subdirs. If I'm right then it would be better to use
this instead of calling it here.
However I think that the real correct answer is that blktap3 shouldn't
have it's own configure anyway but should simply add the tests which it
needs to the global tools level one and use the result like everyone
else.
> + CFLAGS="-I$(XEN_ROOT)/tools/include \
> + -I$(XEN_ROOT)/tools/libxc \
> + -I$(XEN_ROOT)/tools/xenstore" \
> + LDFLAGS="-L$(XEN_ROOT)/tools/xenstore \
> + -L$(XEN_ROOT)/tools/libxc"; \
Your Makefiles should start with
XEN_ROOT = $(CURDIR)/../..
include $(XEN_ROOT)/tools/Rules.mk
And then make use of the variables defined in Rules.mk. e.g.
CFLAGS_libxenctrl, LIBS_libxenctrl etc rather than doing this.
I suppose blktap3 once lived outside of the xen tree and this (and the
configurey) is a hangover from that. But we should clean it up on its
way into the tree
> diff --git a/tools/blktap2/drivers/Makefile b/tools/blktap2/drivers/Makefile
> --- a/tools/blktap2/drivers/Makefile
> +++ b/tools/blktap2/drivers/Makefile
> @@ -4,9 +4,9 @@
>
> LIBVHDDIR = $(BLKTAP_ROOT)/vhd/lib
>
> -IBIN = tapdisk2 td-util tapdisk-client tapdisk-stream tapdisk-diff
> -QCOW_UTIL = img2qcow qcow-create qcow2raw
> -LOCK_UTIL = lock-util
> +IBIN = tapdisk2 td-util2 tapdisk-client2 tapdisk-stream2 tapdisk-diff2
> +QCOW_UTIL = img2qcow2 qcow-create2 qcow2raw2
> +LOCK_UTIL = lock-util2
This series shouldn't be renaming bits of blktap2. In fact I think as a
general rule it should not be touching tools/blktap2 at all. If it does
it should be in a separate patch I think.
> diff --git a/tools/blktap3/Makefile.am b/tools/blktap3/Makefile.am
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/Makefile.am
This is adding a new dependency on automake which is something we'll
have to discuss.
As part of the initial push I think it would be less controversial to
simply use the existing Xen tools build infrastructure (such as it is).
I think the majority of this could be cribbed petty directly from
blktap2 and other parts of the tools tree.
> diff --git a/tools/blktap3/README b/tools/blktap3/README
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/README
I think I mentioned this before but it looks like this document could do
with a pretty hefty update.
> diff --git a/tools/blktap3/control/tap-ctl-attach.c
> b/tools/blktap3/control/tap-ctl-attach.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/control/tap-ctl-attach.c
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright (c) 2008, XenSource Inc.
You probably want to do an update of all these copyright headers.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> met:
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * * Neither the name of XenSource Inc. nor the names of its contributors
And I suppose this ought to be updated too.
> + * may be used to endorse or promote products derived from this
> software
> + * without specific prior written permission.
The actual three clause BSD says "The name of the author may not be used
to endorse or promote products derived from this software without
specific prior written permission.
This weird variant of the 3-clause BSD is something you might want to
discuss with your management to see if it can't be rationalised.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER
> + * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
> + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
I think it would be worthwhile to have a tools/blktap3/COPYING file to
clarify the licesing terms of blktap3 as a whole.
[...] I didn't look at the majority of the actual tools/blktap3 code.
There's quite a lot of it. I mentioned earlier that you might want to
consider dropping some of the optional components for the time being to
keep the initial upstreaming more manageable.
> diff --git a/tools/blktap3/drivers/td-rated.1.txt
> b/tools/blktap3/drivers/td-rated.1.txt
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/drivers/td-rated.1.txt
Is this a generated file? I didn't see the source but it'd be nice to
have e.g. the actual man page etc.
This made me grep for "doc", "man" and "txt" in the patch, which only
found this one file. Hopefully I just missed it all, or at least can we
expect that additional docs will be forthcoming in the future?
> diff --git a/tools/blktap3/include/blktap2.h b/tools/blktap3/include/blktap2.h
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/include/blktap2.h
s/2/3/ Or does this file belong at all? It seems to mostly relate to the
blktap2 kernel driver ioctl interface. Please can you kill all this
cruft before reposting.
> diff --git a/tools/blktap3/include/list.h b/tools/blktap3/include/list.h
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/include/list.h
> @@ -0,0 +1,149 @@
> +/*
> + * list.h
> + *
> + * This is a subset of linux's list.h intended to be used in user-space.
> + *
> + */
If this came from Linux then it is GPL licensed and must have a GPL
header on it.
The intention seems to be that blktap3 is BSD but this would make it
overall GPL. You could either relicense the whole thing as (L)GPL or
perhaps reimplement using the BSD licensed list macros (see
tools/include/xen-external for the BSD macros which libxl and mini-os
use)
> diff --git a/tools/blktap3/xenio/blkif.h b/tools/blktap3/xenio/blkif.h
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/xenio/blkif.h
Given that this is in-tree you might perhaps want to use the in-three
interface declarations from tools/include.
> diff --git a/tools/blktap3/xenio/list.h b/tools/blktap3/xenio/list.h
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/xenio/list.h
> @@ -0,0 +1,134 @@
> +/*
> + * list.h
> + *
> + * This is a subset of linux's list.h intended to be used in user-space.
> + *
> + */
Another duplicated copy of some GPL code.
Apart from the licensing things perhaps you could rationalise the number
of copies of things like this which you are introducing?
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1171,6 +1171,8 @@
Can you add the following to your ~/.hgrc please:
[diff]
showfunc = True
This will inject the current function name into the hunk header which
makes review much easier.
> disk->backend = LIBXL_DISK_BACKEND_TAP;
> } else if (!strcmp(backend_type, "qdisk")) {
> disk->backend = LIBXL_DISK_BACKEND_QDISK;
> + } else if (!strcmp(backend_type, "xenio")) {
> + disk->backend = LIBXL_DISK_BACKEND_XENIO;
I think you want to replace LIBXL_DISK_BACKEND_TAP rather than add a new
one. You could also steal the name if you like I reckon.
>
> } else {
> disk->backend = LIBXL_DISK_BACKEND_UNKNOWN;
> }
> @@ -1961,6 +1981,7 @@
> }
>
> static void libxl__device_disk_from_xs_be(libxl__gc *gc,
> + xs_transaction_t t,
> const char *be_path,
> libxl_device_disk *disk)
> {
This sort of thing should be done as a separate pre-cursor patch.
> diff --git a/tools/libxl/libxl_tapdisk.c b/tools/libxl/libxl_tapdisk.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/libxl/libxl_tapdisk.c
Is this actually a move of of the existing ibxl_blktap? I think "hg diff
-g" will cause it to use git style patches which make this clearer.
Although I don't see libxl_blktap getting removed, so perhaps not? I
thought I saw you changing the Makefile as if you were renamng as well.
Renaming should generally be done as a standalone patch with no
non-related changes in them, to make them eaiser to review.
> @@ -0,0 +1,162 @@
[...]
> + struct list_head list;
> + tap_list_t *entry, *next_t;
Something odd with whitespace here.
> + int ret = -ENOENT, err;
> +
> + fprintf(stderr, "blktap_find(%s:%s)\n", type, path);
Please drop this sort of debug.
> + INIT_LIST_HEAD(&list);
> + err = tap_ctl_list(&list);
> + if (err < 0)
> + return err;
> [...]
> +// tap_ctl_list_free(&list);
Leak?
> char *libxl__blktap_devpath(libxl__gc *gc,
> + const char *disk,
> + libxl_disk_format format)
> +{
> + const char *type;
> + char *params, *devname = NULL;
> + tap_list_t tap;
> + int err;
> +
> + type = libxl__device_disk_string_of_format(format);
> + fprintf(stderr, "libxl__blktap_devpath(%s:%s)\n", disk, type);
> + err = blktap_find(type, disk, &tap);
> + if (err == 0) {
> + devname = libxl__sprintf(gc, "/dev/xen/blktap-2/tapdev%d",
> tap.minor);
Surely not any more?
> + if (devname)
> + return devname;
> + }
> +
> + params = libxl__sprintf(gc, "%s:%s", type, disk);
> + err = tap_ctl_create(params, &devname, 0, -1, NULL);
> + if (!err) {
> + libxl__ptr_add(gc, devname);
> + return devname;
> + }
> +
> + return NULL;
> +}
[...]
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1862,7 +1862,7 @@
>
> child1 = xl_fork(child_waitdaemon);
> if (child1) {
> - printf("Daemon running with PID %d\n", child1);
> + printf("Daemon running with PID %d for domain %d\n", child1,
> domid);
This is probably a useful change but it has nothing at all to do with
blktap3, please separate all this sort of stuff out.
>
> for (;;) {
> got_child = xl_waitpid(child_waitdaemon, &status, 0);
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |