|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] RFC: blktap3
Thanks for your comments, Ian, I'll address them before reposting.
To start with, you say that I should replace LIBXL_DISK_BACKEND_TAP:
> > 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.
But in tools/libxl/libxl.c:1876, libxl__blktap_devpath is called which seems
blktap2 dependant, so we need a new backend type to be able to use blktap2
along with blktap3, no?
> -----Original Message-----
> From: Ian Campbell
> Sent: 16 August 2012 17:10
> To: Thanos Makatos
> Cc: xen-devel@xxxxxxxxxxxxx
> Subject: 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 |