libxl: blktap2 portiblity fixes

PATCH In review Christoph Egger 5 participants 44 messages Jul 20, 2010 – Jul 30, 2010 1 message not in the archive

lore

Share & tools

lore

Christoph Egger

author

2010-07-20 16:31 UTC #0 lore raw

PATCH 1/1 libxl: blktap2 portiblity fixes

Hi!

Attached patch moves blktap2 (which is Linux specific) into a linux specific file.
Combined with the previous libxl patch, libxl builds again on NetBSD.

Signed-off-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

tools/libxl/libxl.c +16 ~~-41~~

diff -r 6d67482c3649 -r 63901ae6eb20 tools/libxl/libxl.c
@@ -34,7 +34,6 @@
 #include "libxl_utils.h"
 #include "libxl_internal.h"
 #include "flexarray.h"
-#include "tap-ctl.h"
 
 #define PAGE_TO_MEMKB(pages) ((pages) * 4)

@@ -1306,27 +1305,6 @@ int libxl_confirm_device_model_startup(s
 
 /******************************************************************************/
 
-static char *get_blktap2_device(struct libxl_ctx *ctx,
-				const char *name, const char *type)
-{
-    int minor = tap_ctl_find_minor(type, name);
-    if (minor < 0)
-        return NULL;
-    return libxl_sprintf(ctx, "/dev/xen/blktap-2/tapdev%d", minor);
-}
-
-static char *make_blktap2_device(struct libxl_ctx *ctx,
-				 const char *name, const char *type)
-{
-    char *params, *devname = NULL;
-    int err;
-    params = libxl_sprintf(ctx, "%s:%s", type, name);
-    err = tap_ctl_create(params, &devname);
-    if (!err)
-        libxl_ptr_add(ctx, devname);
-    return err ? NULL : devname;
-}
-
 int libxl_device_disk_add(struct libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
 {
     flexarray_t *front;

@@ -1375,14 +1353,13 @@ int libxl_device_disk_add(struct libxl_c
         case PHYSTYPE_FILE:
             /* let's pretend is tap:aio for the moment */
             disk->phystype = PHYSTYPE_AIO;
-        case PHYSTYPE_AIO: case PHYSTYPE_QCOW: case PHYSTYPE_QCOW2: case PHYSTYPE_VHD: {
-            const char *msg;
-            if (!tap_ctl_check(&msg)) {
-                const char *type = device_disk_string_of_phystype(disk->phystype);
-                char *dev;
-                dev = get_blktap2_device(ctx, disk->physpath, type);
-                if (!dev)
-                    dev = make_blktap2_device(ctx, disk->physpath, type);
+        case PHYSTYPE_AIO:
+        case PHYSTYPE_QCOW:
+        case PHYSTYPE_QCOW2:
+        case PHYSTYPE_VHD:
+            if (libxl_blktap_enabled(ctx)) {
+                const char *dev = libxl_blktap_devpath(ctx,
+                                               disk->physpath, disk->phystype);
                 if (!dev)
                     return ERROR_FAIL;
                 flexarray_set(back, boffset++, "tapdisk-params");

@@ -1403,7 +1380,7 @@ int libxl_device_disk_add(struct libxl_c
 
             device.backend_kind = DEVICE_TAP;
             break;
-        }
+
         default:
             XL_LOG(ctx, XL_LOG_ERROR, "unrecognized disk physical type: %d\n", disk->phystype);
             return ERROR_INVAL;

@@ -1467,7 +1444,7 @@ int libxl_device_disk_del(struct libxl_c
 
 const char * libxl_device_disk_local_attach(struct libxl_ctx *ctx, libxl_device_disk *disk)
 {
-    char *dev = NULL;
+    const char *dev = NULL;
     int phystype = disk->phystype;
     switch (phystype) {
         case PHYSTYPE_PHY: {

@@ -1478,16 +1455,14 @@ const char * libxl_device_disk_local_att
         case PHYSTYPE_FILE:
             /* let's pretend is tap:aio for the moment */
             phystype = PHYSTYPE_AIO;
-        case PHYSTYPE_AIO: case PHYSTYPE_QCOW: case PHYSTYPE_QCOW2: case PHYSTYPE_VHD: {
-            const char *msg;
-            if (!tap_ctl_check(&msg)) {
-                const char *type = device_disk_string_of_phystype(phystype);
-                dev = get_blktap2_device(ctx, disk->physpath, type);
-                if (!dev)
-                    dev = make_blktap2_device(ctx, disk->physpath, type);
-            }
+        case PHYSTYPE_AIO:
+        case PHYSTYPE_QCOW:
+        case PHYSTYPE_QCOW2:
+        case PHYSTYPE_VHD:
+            if (libxl_blktap_enabled(ctx))
+                dev = libxl_blktap_devpath(ctx, disk->physpath, phystype);
             break;
-        }
+
         default:
             XL_LOG(ctx, XL_LOG_ERROR, "unrecognized disk physical type: %d\n", phystype);
             break;

tools/libxl/libxl_device.c +1 -1

diff -r 6d67482c3649 -r 63901ae6eb20 tools/libxl/libxl_device.c
@@ -132,7 +132,7 @@ char *device_disk_backend_type_of_physty
     }
 }
 
-int device_physdisk_major_minor(char *physpath, int *major, int *minor)
+int device_physdisk_major_minor(const char *physpath, int *major, int *minor)
 {
     struct stat buf;
     if (stat(physpath, &buf) < 0)

tools/libxl/libxl_internal.h +1 -1

diff -r 6d67482c3649 -r 63901ae6eb20 tools/libxl/libxl_internal.h
@@ -149,7 +149,7 @@ void libxl__userdata_destroyall(struct l
 char *device_disk_backend_type_of_phystype(libxl_disk_phystype phystype);
 char *device_disk_string_of_phystype(libxl_disk_phystype phystype);
 
-int device_physdisk_major_minor(char *physpath, int *major, int *minor);
+int device_physdisk_major_minor(const char *physpath, int *major, int *minor);
 int device_disk_dev_number(char *virtpath);
 
 int libxl_device_generic_add(struct libxl_ctx *ctx, libxl_device *device,

tools/libxl/libxl_linux.c new file +50 -0

50 lines not on this page · raw message

tools/libxl/libxl_netbsd.c new file +28 -0

diff -r 6d67482c3649 -r 63901ae6eb20 tools/libxl/libxl_netbsd.c
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only.
+ *
+ * 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 Lesser General Public License for more details.
+ */
+
+#include "libxl.h"
+#include "libxl_osdeps.h"
+
+int libxl_blktap_enabled(struct libxl_ctx *ctx)
+{
+    return 0;
+}
+
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype)
+{
+    return NULL;
+}

tools/libxl/libxl_osdeps.h +21 -0

diff -r 6d67482c3649 -r 63901ae6eb20 tools/libxl/libxl_osdeps.h
@@ -23,6 +23,8 @@
 
 #define _GNU_SOURCE
 
+#include <libxl_internal.h>
+
 #ifdef NEED_OWN_ASPRINTF
 #include <stdarg.h>

@@ -30,4 +32,23 @@ int asprintf(char **buffer, char *fmt, .
 int vasprintf(char **buffer, const char *fmt, va_list ap);
 #endif /*NEED_OWN_ASPRINTF*/
 
+/*
+ * blktap2 support
+ */
+
+/* libxl_blktap_enabled:
+ *    return true if blktap/blktap2 support is available.
+ */
+int libxl_blktap_enabled(struct libxl_ctx *ctx);
+
+/* libxl_blktap_devpath:
+ *    Argument: path and disk image as specified in config file.
+ *      The type specifies whether this is aio, qcow, qcow2, etc.
+ *    returns device path xenstore wants to have. returns NULL
+ *      if no device corresponds to the disk.
+ */
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype);
+
 #endif

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Stefano Stabellini

2010-07-20 16:46 UTC #1 lore raw

Quoted 9 lines · Christoph Egger

Hi!
Attached patch moves blktap2 (which is Linux specific) into a linux specific file.
Combined with the previous libxl patch, libxl builds again on NetBSD.
Signed-off-by Christoph Egger <[email protected]>

this patch breaks the build:

libxenlight.so: undefined reference to libxl_blktap_enabled' libxenlight.so: undefined reference to libxl_blktap_devpath'
collect2: ld returned 1 exit status

Christoph Egger

author

2010-07-21 08:32 UTC #2 lore raw

Quoted 7 lines · On Tuesday 20 July 2010 18:46:30 Stefano Stabellini wrote:

Quoted 6 lines · Christoph Egger
Hi!
Attached patch moves blktap2 (which is Linux specific) into a linux specific file.
Combined with the previous libxl patch, libxl builds again on NetBSD.

Quoted 7 lines

Quoted 1 line
Signed-off-by Christoph Egger <[email protected]>
this patch breaks the build:
libxenlight.so: undefined reference to libxl_blktap_enabled' libxenlight.so: undefined reference to libxl_blktap_devpath'
collect2: ld returned 1 exit status

Oh, I see the problem. The wrong lines are those:

LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o +LIBXL_OBJS-$(CONFIG_Linux) = libxl_linux.o +LIBXL_OBJS-$(CONFIG_NetBSD) = libxl_netbsd.o

New corrected version is attached. It also reverts c/s 21835 as this showed up due to this bug in my tree.

Signed-off-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

tools/libxl/Makefile +3 -1

diff -r c7a7179810b6 -r 784d73e565cc tools/libxl/Makefile
@@ -18,6 +18,8 @@ CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_
 LIBS = $(LDFLAGS_libxenctrl) $(LDFLAGS_libxenguest) $(LDFLAGS_libxenstore) $(LDFLAGS_libblktapctl) $(UTIL_LIBS)
 
 LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
+LIBXL_OBJS-$(CONFIG_Linux) += libxl_linux.o
+LIBXL_OBJS-$(CONFIG_NetBSD) += libxl_netbsd.o
 LIBXL_OBJS = flexarray.o libxl.o libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o libxl_internal.o xenguest.o libxl_utils.o $(LIBXL_OBJS-y)
 
 AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h

@@ -85,7 +87,7 @@ xl_cmdimpl.o: xl_cmdimpl.c
 xl_cmdtable.o: xl_cmdtable.c
 	$(CC) $(CFLAGS) -c xl_cmdtable.c
 
-$(CLIENTS): xl.o xl_cmdimpl.o xl_cmdtable.o libxl_paths.o libxl_bootloader.o libxlutil.so libxenlight.so
+$(CLIENTS): xl.o xl_cmdimpl.o xl_cmdtable.o libxlutil.so libxenlight.so
 	$(CC) $(LDFLAGS) -o $@ $^ $(LIBS)
 
 .PHONY: install

tools/libxl/libxl.c +16 ~~-41~~

94 lines not on this page · raw message

tools/libxl/libxl_device.c +1 -1

diff -r c7a7179810b6 -r 784d73e565cc tools/libxl/libxl_device.c
@@ -132,7 +132,7 @@ char *device_disk_backend_type_of_physty
     }
 }
 
-int device_physdisk_major_minor(char *physpath, int *major, int *minor)
+int device_physdisk_major_minor(const char *physpath, int *major, int *minor)
 {
     struct stat buf;
     if (stat(physpath, &buf) < 0)

tools/libxl/libxl_internal.h +1 -1

diff -r c7a7179810b6 -r 784d73e565cc tools/libxl/libxl_internal.h
@@ -149,7 +149,7 @@ void libxl__userdata_destroyall(struct l
 char *device_disk_backend_type_of_phystype(libxl_disk_phystype phystype);
 char *device_disk_string_of_phystype(libxl_disk_phystype phystype);
 
-int device_physdisk_major_minor(char *physpath, int *major, int *minor);
+int device_physdisk_major_minor(const char *physpath, int *major, int *minor);
 int device_disk_dev_number(char *virtpath);
 
 int libxl_device_generic_add(struct libxl_ctx *ctx, libxl_device *device,

tools/libxl/libxl_linux.c new file +50 -0

50 lines not on this page · raw message

tools/libxl/libxl_netbsd.c new file +28 -0

diff -r c7a7179810b6 -r 784d73e565cc tools/libxl/libxl_netbsd.c
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only.
+ *
+ * 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 Lesser General Public License for more details.
+ */
+
+#include "libxl.h"
+#include "libxl_osdeps.h"
+
+int libxl_blktap_enabled(struct libxl_ctx *ctx)
+{
+    return 0;
+}
+
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype)
+{
+    return NULL;
+}

tools/libxl/libxl_osdeps.h +21 -0

diff -r c7a7179810b6 -r 784d73e565cc tools/libxl/libxl_osdeps.h
@@ -23,6 +23,8 @@
 
 #define _GNU_SOURCE
 
+#include <libxl_internal.h>
+
 #ifdef NEED_OWN_ASPRINTF
 #include <stdarg.h>

@@ -30,4 +32,23 @@ int asprintf(char **buffer, char *fmt, .
 int vasprintf(char **buffer, const char *fmt, va_list ap);
 #endif /*NEED_OWN_ASPRINTF*/
 
+/*
+ * blktap2 support
+ */
+
+/* libxl_blktap_enabled:
+ *    return true if blktap/blktap2 support is available.
+ */
+int libxl_blktap_enabled(struct libxl_ctx *ctx);
+
+/* libxl_blktap_devpath:
+ *    Argument: path and disk image as specified in config file.
+ *      The type specifies whether this is aio, qcow, qcow2, etc.
+ *    returns device path xenstore wants to have. returns NULL
+ *      if no device corresponds to the disk.
+ */
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype);
+
 #endif

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Stefano Stabellini

2010-07-23 12:30 UTC #3 lore raw

Quoted 22 lines · Christoph Egger

Quoted 7 lines · On Tuesday 20 July 2010 18:46:30 Stefano Stabellini wrote:
Quoted 6 lines · Christoph Egger
Hi!
Attached patch moves blktap2 (which is Linux specific) into a linux specific file.
Combined with the previous libxl patch, libxl builds again on NetBSD.
Quoted 7 lines
Quoted 1 line
Signed-off-by Christoph Egger <[email protected]>
this patch breaks the build:
libxenlight.so: undefined reference to libxl_blktap_enabled' libxenlight.so: undefined reference to libxl_blktap_devpath'
collect2: ld returned 1 exit status
Oh, I see the problem. The wrong lines are those:
LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
(quoted diff) +2 -0
+LIBXL_OBJS-$(CONFIG_Linux) = libxl_linux.o
+LIBXL_OBJS-$(CONFIG_NetBSD) = libxl_netbsd.o

Quoted 5 lines

New corrected version is attached. It also reverts c/s 21835 as this showed up due to this bug in my tree.
Signed-off-by Christoph Egger <[email protected]>

This seems to be the right fix for the compiling issue. Acked by me.

Ian Jackson

2010-07-23 18:05 UTC #4 lore raw

In reply to Christoph Egger

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity fixes"):

Quoted 4 lines

New corrected version is attached. It also reverts c/s 21835 as this showed up due to this bug in my tree.
Signed-off-by Christoph Egger <[email protected]>

I applied this, but I found that it broke the build. I have reverted these three patches of yours:

df9d8319bd37 Fix blktap2 NetBSD build and also revert broken change e76befc7fe2d portability fixes from tools/console 24277e3237ca Fix linking error when creating the xl binary.

Please come back with a proper fix that doesn't break the build. Then I'll test and apply it.

Ian.

Christoph Egger

author

2010-07-26 10:56 UTC #5 lore raw

Quoted 1 line · On Friday 23 July 2010 20:05:50 Ian Jackson wrote:

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity

fixes"):

Quoted 11 lines

Quoted 4 lines
New corrected version is attached. It also reverts c/s 21835 as this showed up due to this bug in my tree.
Signed-off-by Christoph Egger <[email protected]>
I applied this, but I found that it broke the build. I have reverted these three patches of yours:
df9d8319bd37 Fix blktap2 NetBSD build and also revert broken change e76befc7fe2d portability fixes from tools/console 24277e3237ca Fix linking error when creating the xl binary.

Can you use the c/s numbers, please?

It was not necessary to backout c/s 21834 as this wasn't the root cause.

Quoted 1 line

Please come back with a proper fix.

Attached.

Re-include c/s 21834 which takes over portability fixes from tools/console to make libxl_bootloader.c build on NetBSD. Also use $(UTIL_LIBS) in the Makefile as done in tools/console/Makefile.

blktapctl is build on Linux only. Introduce two API functions (libxl_blktap_*) which I have discussed with Stefano in end of June/beginning of July.
Implement the Linux API by moving the
Linux specific code from libxl.c into libxl_linux.c.

Signed-off-by Christoph Egger <[email protected]>

Quoted 1 line

that doesn't break the build

... on Linux you mean ?

Quoted 1 line

Then I'll test and apply it.

Christoph

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

tools/Rules.mk +5 -0

diff -r 2b768d52bc7f tools/Rules.mk
@@ -26,8 +26,13 @@ LDFLAGS_libxenguest = -L$(XEN_LIBXC) -lx
 CFLAGS_libxenstore = -I$(XEN_XENSTORE) $(CFLAGS_include)
 LDFLAGS_libxenstore = -L$(XEN_XENSTORE) -lxenstore
 
+ifeq ($(CONFIG_Linux),y)
 CFLAGS_libblktapctl = -I$(XEN_BLKTAP2)/control -I$(XEN_BLKTAP2)/include $(CFLAGS_include)
 LDFLAGS_libblktapctl = -L$(XEN_BLKTAP2)/control -lblktapctl
+else
+CFLAGS_libblktapctl =
+LDFLAGS_libblktapctl =
+endif
 
 X11_LDPATH = -L/usr/X11R6/$(LIBLEAFDIR)

tools/libxl/Makefile +3 -1

diff -r 2b768d52bc7f tools/libxl/Makefile
@@ -15,9 +15,11 @@ CFLAGS += -Werror -Wno-format-zero-lengt
 CFLAGS += -I. -fPIC
 CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest) $(CFLAGS_libxenstore) $(CFLAGS_libblktapctl)
 
-LIBS = $(LDFLAGS_libxenctrl) $(LDFLAGS_libxenguest) $(LDFLAGS_libxenstore) $(LDFLAGS_libblktapctl) -lutil
+LIBS = $(LDFLAGS_libxenctrl) $(LDFLAGS_libxenguest) $(LDFLAGS_libxenstore) $(LDFLAGS_libblktapctl) $(UTIL_LIBS)
 
 LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
+LIBXL_OBJS-$(CONFIG_Linux) += libxl_linux.o
+LIBXL_OBJS-$(CONFIG_NetBSD) += libxl_netbsd.o
 LIBXL_OBJS = flexarray.o libxl.o libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o libxl_internal.o xenguest.o libxl_utils.o $(LIBXL_OBJS-y)
 
 AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h

tools/libxl/libxl.c +16 ~~-41~~

94 lines not on this page · raw message

tools/libxl/libxl_bootloader.c +8 -1

diff -r 2b768d52bc7f tools/libxl/libxl_bootloader.c
@@ -15,9 +15,16 @@
 #include "libxl_osdeps.h"
 
 #include <string.h>
-#include <pty.h>
 #include <unistd.h>
 #include <fcntl.h>
+#include <termios.h>
+#if defined(__NetBSD__) || defined(__OpenBSD__)
+#include <util.h>
+#elif defined(__linux__)
+#include <pty.h>
+#elif defined(__sun__)
+#include <stropts.h>
+#endif
 
 #include <sys/stat.h>
 #include <sys/types.h>

tools/libxl/libxl_device.c +1 -1

diff -r 2b768d52bc7f tools/libxl/libxl_device.c
@@ -123,7 +123,7 @@ char *device_disk_backend_type_of_physty
     }
 }
 
-int device_physdisk_major_minor(char *physpath, int *major, int *minor)
+int device_physdisk_major_minor(const char *physpath, int *major, int *minor)
 {
     struct stat buf;
     if (stat(physpath, &buf) < 0)

tools/libxl/libxl_internal.h +1 -1

diff -r 2b768d52bc7f tools/libxl/libxl_internal.h
@@ -149,7 +149,7 @@ void libxl__userdata_destroyall(struct l
 char *device_disk_backend_type_of_phystype(libxl_disk_phystype phystype);
 char *device_disk_string_of_phystype(libxl_disk_phystype phystype);
 
-int device_physdisk_major_minor(char *physpath, int *major, int *minor);
+int device_physdisk_major_minor(const char *physpath, int *major, int *minor);
 int device_disk_dev_number(char *virtpath);
 
 int libxl_device_generic_add(struct libxl_ctx *ctx, libxl_device *device,

tools/libxl/libxl_linux.c new file +50 -0

50 lines not on this page · raw message

tools/libxl/libxl_netbsd.c new file +28 -0

diff -r 2b768d52bc7f tools/libxl/libxl_netbsd.c
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only.
+ *
+ * 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 Lesser General Public License for more details.
+ */
+
+#include "libxl.h"
+#include "libxl_osdeps.h"
+
+int libxl_blktap_enabled(struct libxl_ctx *ctx)
+{
+    return 0;
+}
+
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype)
+{
+    return NULL;
+}

tools/libxl/libxl_osdeps.h +21 -0

diff -r 2b768d52bc7f tools/libxl/libxl_osdeps.h
@@ -23,6 +23,8 @@
 
 #define _GNU_SOURCE
 
+#include <libxl_internal.h>
+
 #ifdef NEED_OWN_ASPRINTF
 #include <stdarg.h>

@@ -30,4 +32,23 @@ int asprintf(char **buffer, char *fmt, .
 int vasprintf(char **buffer, const char *fmt, va_list ap);
 #endif /*NEED_OWN_ASPRINTF*/
 
+/*
+ * blktap2 support
+ */
+
+/* libxl_blktap_enabled:
+ *    return true if blktap/blktap2 support is available.
+ */
+int libxl_blktap_enabled(struct libxl_ctx *ctx);
+
+/* libxl_blktap_devpath:
+ *    Argument: path and disk image as specified in config file.
+ *      The type specifies whether this is aio, qcow, qcow2, etc.
+ *    returns device path xenstore wants to have. returns NULL
+ *      if no device corresponds to the disk.
+ */
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype);
+
 #endif

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Ian Jackson

2010-07-26 14:53 UTC #6 lore raw

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity fixes"):

Quoted 1 line

Can you use the c/s numbers, please?

Changeset numbers are not guaranteed to be meaningful outside a particular tree, particularly in the presence of merges.

Quoted 1 line

It was not necessary to backout c/s 21834 as this wasn't the root cause.

I think by 21834 you mean 24277e3237ca. That change was entirely wrong and you even retracted it yourself! I see that your patch doesn't actually reinstate it.

Looking at this patch it's difficult to review and test and there are some things I would like to see improved. Can I ask you to try to split the patch up into separate pieces ?

At the very least, please separate out the following:
* Adding new interfaces, ie introucing new functions and putting code into those functions and replacing it at the original site with a call;
* Const-correctness
* Code motion between files, and creation of libxl_linux.c;
* Provide libxl_netbsd.c and associated Makefile changes to disable blktap2 on netbsd
* #include portability fixes for openpty, pty.h etc.

Quoted 8 lines

diff -r 2b768d52bc7f tools/libxl/libxl_bootloader.c
tools/libxl/libxl_bootloader.c +0 -1
@@ -15,9 +15,16 @@
 #include "libxl_osdeps.h"
 
 #include <string.h>
-#include <pty.h>

tools/libxl/libxl_bootloader.c line 27

Quoted 10 lines

tools/libxl/libxl_bootloader.c +8 -0

 #include <unistd.h>
 #include <fcntl.h>
+#include <termios.h>
+#if defined(__NetBSD__) || defined(__OpenBSD__)
+#include <util.h>
+#elif defined(__linux__)
+#include <pty.h>
+#elif defined(__sun__)
+#include <stropts.h>
+#endif

This should be done by moving the relevant #includes to osdep.h, where all this kind of thing should be done.

tools/libxl/libxl_bootloader.c line 28

Quoted 2 lines

tools/libxl/libxl_bootloader.c +1 -1

-int device_physdisk_major_minor(char *physpath, int *major, int *minor)
+int device_physdisk_major_minor(const char *physpath, int *major, int *minor)

This change should be separated out.

Thanks,
Ian.

Ian Campbell

2010-07-26 15:00 UTC #7 lore raw

Quoted 3 lines · Ian Jackson

* Code motion between files, and creation of libxl_linux.c;
* Provide libxl_netbsd.c and associated Makefile changes to disable blktap2 on netbsd

I wonder if this might be better as libxl_blk_blktap.c and libxl_blk_none.c or something to correspond to the presence of the specific feature rather than an OS which happens to implement the feature? (I guess in this specific instance the chances of blktap cropping up on another OS is pretty slim).

Ian.

Ian Jackson

2010-07-26 15:12 UTC #8 lore raw

Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity fixes"):

Quoted 9 lines · Ian Jackson

Quoted 3 lines
* Code motion between files, and creation of libxl_linux.c;
* Provide libxl_netbsd.c and associated Makefile changes to disable blktap2 on netbsd
I wonder if this might be better as libxl_blk_blktap.c and libxl_blk_none.c or something to correspond to the presence of the specific feature rather than an OS which happens to implement the feature?

Good point.

Quoted 2 lines

(I guess in this specific instance the chances of blktap cropping up on another OS is pretty slim).

Well, but there's no harm to naming the feature rather than the platform. It's good practice.

Ian.

Christoph Egger

author

2010-07-26 16:01 UTC #9 lore raw

Quoted 1 line · On Monday 26 July 2010 17:12:34 Ian Jackson wrote:

Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity

fixes"):

Quoted 10 lines

Quoted 9 lines · Ian Jackson
Quoted 3 lines
* Code motion between files, and creation of libxl_linux.c;
* Provide libxl_netbsd.c and associated Makefile changes to disable blktap2 on netbsd
I wonder if this might be better as libxl_blk_blktap.c and libxl_blk_none.c or something to correspond to the presence of the specific feature rather than an OS which happens to implement the feature?

Quoted 7 lines

Good point.
Quoted 2 lines
(I guess in this specific instance the chances of blktap cropping up on another OS is pretty slim).
Well, but there's no harm to naming the feature rather than the platform. It's good practice.

It's bad practise to have OS specific code in common code. There's a lot of sysfs code in libxl.c - currently only used for pci passthrough.

In June/July I discussed three other API functions with Stephano (which are not part of this series):

- libxl_pciback_flr
performs FLR on the device specified by BFD

- libxl_pciback_bind
unhooks pci device from dom0 and bind to pciback

- libxl_pciback_unbind
unbind pci device from pciback and give back to dom0

These should allow to move all sysfs code in libxl.c into a linux specific file.

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Christoph Egger

author

2010-07-26 16:05 UTC #10 lore raw

In reply to Ian Jackson

Quoted 1 line · On Monday 26 July 2010 16:53:02 Ian Jackson wrote:

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity

fixes"):

Quoted 8 lines

Quoted 1 line
Can you use the c/s numbers, please?
Changeset numbers are not guaranteed to be meaningful outside a particular tree, particularly in the presence of merges.
Quoted 1 line
It was not necessary to backout c/s 21834 as this wasn't the root cause.
I think by 21834 you mean 24277e3237ca.

No, c/s 21834 has the hash e76befc7fe2d.

Quoted 4 lines

Looking at this patch it's difficult to review and test and there are some things I would like to see improved. Can I ask you to try to split the patch up into separate pieces ?

Quoted 9 lines

At the very least, please separate out the following:
* Adding new interfaces, ie introucing new functions and putting code into those functions and replacing it at the original site with a call;
* Const-correctness
* Code motion between files, and creation of libxl_linux.c;
* Provide libxl_netbsd.c and associated Makefile changes to disable blktap2 on netbsd
* #include portability fixes for openpty, pty.h etc.

That's doable but I don't see if a standalone const-correctness patch makes sense just to make gcc happy with an other patch.

Quoted 6 lines

Quoted 6 lines
diff -r 2b768d52bc7f tools/libxl/libxl_bootloader.c
tools/libxl/libxl_bootloader.c +0 -0
@@ -15,9 +15,16 @@
 #include "libxl_osdeps.h"
 

Quoted 15 lines

tools/libxl/libxl_bootloader.c line 27
Quoted 12 lines
tools/libxl/libxl_bootloader.c +8 -1
 #include <string.h>
-#include <pty.h>
 #include <unistd.h>
 #include <fcntl.h>
+#include <termios.h>
+#if defined(__NetBSD__) || defined(__OpenBSD__)
+#include <util.h>
+#elif defined(__linux__)
+#include <pty.h>
+#elif defined(__sun__)
+#include <stropts.h>
+#endif
This should be done by moving the relevant #includes to osdep.h, where all this kind of thing should be done.

Should this header be re-used by tools/console/daemon/io.c ? If yes, where is the best place to put osdep.h ?

Quoted 6 lines

tools/libxl/libxl_bootloader.c line 29
Quoted 4 lines
tools/libxl/libxl_bootloader.c +1 -1
-int device_physdisk_major_minor(char *physpath, int *major, int *minor)
+int device_physdisk_major_minor(const char *physpath, int *major, int
 *minor)
This change should be separated out.

hg record is your friend.

Christoph

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Ian Jackson

2010-07-26 16:36 UTC #11 lore raw

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity fixes"):

Quoted 5 lines · On Monday 26 July 2010 16:53:02 Ian Jackson wrote:

Quoted 1 line
Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity
fixes"):
Quoted 2 lines
Quoted 1 line
Can you use the c/s numbers, please?

Quoted 8 lines

Quoted 6 lines
Changeset numbers are not guaranteed to be meaningful outside a particular tree, particularly in the presence of merges.
Quoted 1 line
It was not necessary to backout c/s 21834 as this wasn't the root cause.
I think by 21834 you mean 24277e3237ca.
No, c/s 21834 has the hash e76befc7fe2d.

Thus proving my point. In my tree 21834 is 24277e3237ca :-). I'm sorry that I may have reverted more than was necessary. I just wanted to get everything back to a known good state.

Quoted 2 lines

That's doable but I don't see if a standalone const-correctness patch makes sense just to make gcc happy with an other patch.

The reason is that const-correctness changes shouldn't be checked in as "portability to netbsd". It also means that if for any reason there is some argument or rework needed for other parts of your patchset, it's possible to apply the ones which are clearly right. And it simplifies the review if you can explain for each patch what one thing it does.

Quoted 5 lines

Quoted 2 lines
This should be done by moving the relevant #includes to osdep.h, where all this kind of thing should be done.
Should this header be re-used by tools/console/daemon/io.c ? If yes, where is the best place to put osdep.h ?

Perhaps. That would make it a public header rather than a private one. On the other hand xenconsoled is not a libxl caller so it's not clear where that public header would live. I don't think that we need to cross this bridge right away. Regardless, finding an example elsewhere in the tree, in directory which doesn't have an osdep.h, isn't a good reason not to put #ifdef #include stuff in the header we do have for it in libxl.

Ian.

Christoph Egger

author

2010-07-27 12:07 UTC #12 lore raw

Quoted 1 line · On Monday 26 July 2010 18:36:56 Ian Jackson wrote:

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity

fixes"):

Quoted 10 lines

Quoted 10 lines · On Monday 26 July 2010 16:53:02 Ian Jackson wrote:
Quoted 2 lines
Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity
fixes"):
Quoted 5 lines
Quoted 1 line
Can you use the c/s numbers, please?
Changeset numbers are not guaranteed to be meaningful outside a particular tree, particularly in the presence of merges.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
It was not necessary to backout c/s 21834 as this wasn't the root cause.
I think by 21834 you mean 24277e3237ca.
No, c/s 21834 has the hash e76befc7fe2d.
Thus proving my point. In my tree 21834 is 24277e3237ca :-).

I am not refering the c/s to my local tree. I'm refering the c/s to
http://xenbits.xensource.com/staging/xen-unstable.hg/

Christoph

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Christoph Egger

author

2010-07-27 12:15 UTC #13 lore raw

PATCH 1/6 libxl: portiblity fixes

In reply to Ian Jackson

Signed-off-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

tools/libxl/Makefile +1 -1

diff -r 3a2899ebb8e2 -r 993ee4f54b2e tools/libxl/Makefile
@@ -15,7 +15,7 @@
 CFLAGS += -I. -fPIC
 CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest) $(CFLAGS_libxenstore) $(CFLAGS_libblktapctl)
 
-LIBS = $(LDFLAGS_libxenctrl) $(LDFLAGS_libxenguest) $(LDFLAGS_libxenstore) $(LDFLAGS_libblktapctl) -lutil
+LIBS = $(LDFLAGS_libxenctrl) $(LDFLAGS_libxenguest) $(LDFLAGS_libxenstore) $(LDFLAGS_libblktapctl) $(UTIL_LIBS)
 
 LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
 LIBXL_OBJS = flexarray.o libxl.o libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o libxl_internal.o xenguest.o libxl_utils.o $(LIBXL_OBJS-y)

tools/libxl/libxl_bootloader.c +8 -1

diff -r 3a2899ebb8e2 -r 993ee4f54b2e tools/libxl/libxl_bootloader.c
@@ -15,9 +15,16 @@
 #include "libxl_osdeps.h"
 
 #include <string.h>
-#include <pty.h>
 #include <unistd.h>
 #include <fcntl.h>
+#include <termios.h>
+#if defined(__NetBSD__) || defined(__OpenBSD__)
+#include <util.h>
+#elif defined(__linux__)
+#include <pty.h>
+#elif defined(__sun__)
+#include <stropts.h>
+#endif
 
 #include <sys/stat.h>
 #include <sys/types.h>

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Christoph Egger

author

2010-07-27 12:16 UTC #14 lore raw

PATCH 2/6 libxl: portiblity fixes

Signed-off-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

tools/libxl/libxl_osdeps.h +21 -0

diff -r 993ee4f54b2e -r 1ca40043b632 tools/libxl/libxl_osdeps.h
@@ -23,6 +23,8 @@
 
 #define _GNU_SOURCE
 
+#include <libxl_internal.h>
+
 #ifdef NEED_OWN_ASPRINTF
 #include <stdarg.h>

@@ -30,4 +32,23 @@
 int vasprintf(char **buffer, const char *fmt, va_list ap);
 #endif /*NEED_OWN_ASPRINTF*/
 
+/*
+ * blktap2 support
+ */
+
+/* libxl_blktap_enabled:
+ *    return true if blktap/blktap2 support is available.
+ */
+int libxl_blktap_enabled(struct libxl_ctx *ctx);
+
+/* libxl_blktap_devpath:
+ *    Argument: path and disk image as specified in config file.
+ *      The type specifies whether this is aio, qcow, qcow2, etc.
+ *    returns device path xenstore wants to have. returns NULL
+ *      if no device corresponds to the disk.
+ */
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype);
+
 #endif

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Christoph Egger

author

2010-07-27 12:17 UTC #15 lore raw

PATCH 3/6 libxl: portiblity fixes

Signed-off-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

tools/libxl/libxl_device.c +1 -1

diff -r 1ca40043b632 -r 8ccaa5608511 tools/libxl/libxl_device.c
@@ -123,7 +123,7 @@
     }
 }
 
-int device_physdisk_major_minor(char *physpath, int *major, int *minor)
+int device_physdisk_major_minor(const char *physpath, int *major, int *minor)
 {
     struct stat buf;
     if (stat(physpath, &buf) < 0)

tools/libxl/libxl_internal.h +1 -1

diff -r 1ca40043b632 -r 8ccaa5608511 tools/libxl/libxl_internal.h
@@ -149,7 +149,7 @@
 char *device_disk_backend_type_of_phystype(libxl_disk_phystype phystype);
 char *device_disk_string_of_phystype(libxl_disk_phystype phystype);
 
-int device_physdisk_major_minor(char *physpath, int *major, int *minor);
+int device_physdisk_major_minor(const char *physpath, int *major, int *minor);
 int device_disk_dev_number(char *virtpath);
 
 int libxl_device_generic_add(struct libxl_ctx *ctx, libxl_device *device,

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Christoph Egger

author

2010-07-27 12:18 UTC #16 lore raw

PATCH 4/6 libxl: portiblity fixes

Signed-off-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

tools/Rules.mk +5 -0

diff -r 8ccaa5608511 -r 73082ed01e88 tools/Rules.mk
@@ -26,8 +26,13 @@
 CFLAGS_libxenstore = -I$(XEN_XENSTORE) $(CFLAGS_include)
 LDFLAGS_libxenstore = -L$(XEN_XENSTORE) -lxenstore
 
+ifeq ($(CONFIG_Linux),y)
 CFLAGS_libblktapctl = -I$(XEN_BLKTAP2)/control -I$(XEN_BLKTAP2)/include $(CFLAGS_include)
 LDFLAGS_libblktapctl = -L$(XEN_BLKTAP2)/control -lblktapctl
+else
+CFLAGS_libblktapctl =
+LDFLAGS_libblktapctl =
+endif
 
 X11_LDPATH = -L/usr/X11R6/$(LIBLEAFDIR)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Christoph Egger

author

2010-07-27 12:20 UTC #17 lore raw

PATCH 5/6 libxl: portiblity fixes

Signed-off-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

tools/libxl/Makefile +2 -0

diff -r 73082ed01e88 -r 82c7ff060fa3 tools/libxl/Makefile
@@ -18,6 +18,8 @@
 LIBS = $(LDFLAGS_libxenctrl) $(LDFLAGS_libxenguest) $(LDFLAGS_libxenstore) $(LDFLAGS_libblktapctl) $(UTIL_LIBS)
 
 LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
+LIBXL_OBJS-$(CONFIG_Linux) += libxl_linux.o
+LIBXL_OBJS-$(CONFIG_NetBSD) += libxl_netbsd.o
 LIBXL_OBJS = flexarray.o libxl.o libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o libxl_internal.o xenguest.o libxl_utils.o $(LIBXL_OBJS-y)
 
 AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h

tools/libxl/libxl_linux.c new file +50 -0

50 lines not on this page · raw message

tools/libxl/libxl_netbsd.c new file +28 -0

diff -r 73082ed01e88 -r 82c7ff060fa3 tools/libxl/libxl_netbsd.c
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only.
+ *
+ * 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 Lesser General Public License for more details.
+ */
+
+#include "libxl.h"
+#include "libxl_osdeps.h"
+
+int libxl_blktap_enabled(struct libxl_ctx *ctx)
+{
+    return 0;
+}
+
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype)
+{
+    return NULL;
+}

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Christoph Egger

author

2010-07-27 12:21 UTC #18 lore raw

PATCH 6/6 libxl: portiblity fixes

Signed-off-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

tools/libxl/libxl.c +16 ~~-41~~

diff -r 82c7ff060fa3 -r 999c524ce64b tools/libxl/libxl.c
@@ -34,7 +34,6 @@
 #include "libxl_utils.h"
 #include "libxl_internal.h"
 #include "flexarray.h"
-#include "tap-ctl.h"
 
 #define PAGE_TO_MEMKB(pages) ((pages) * 4)

@@ -1391,27 +1390,6 @@
 
 /******************************************************************************/
 
-static char *get_blktap2_device(struct libxl_ctx *ctx,
-				const char *name, const char *type)
-{
-    int minor = tap_ctl_find_minor(type, name);
-    if (minor < 0)
-        return NULL;
-    return libxl_sprintf(ctx, "/dev/xen/blktap-2/tapdev%d", minor);
-}
-
-static char *make_blktap2_device(struct libxl_ctx *ctx,
-				 const char *name, const char *type)
-{
-    char *params, *devname = NULL;
-    int err;
-    params = libxl_sprintf(ctx, "%s:%s", type, name);
-    err = tap_ctl_create(params, &devname);
-    if (!err)
-        libxl_ptr_add(ctx, devname);
-    return err ? NULL : devname;
-}
-
 int libxl_device_disk_add(struct libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
 {
     flexarray_t *front;

@@ -1460,14 +1438,13 @@
         case PHYSTYPE_FILE:
             /* let's pretend is tap:aio for the moment */
             disk->phystype = PHYSTYPE_AIO;
-        case PHYSTYPE_AIO: case PHYSTYPE_QCOW: case PHYSTYPE_QCOW2: case PHYSTYPE_VHD: {
-            const char *msg;
-            if (!tap_ctl_check(&msg)) {
-                const char *type = device_disk_string_of_phystype(disk->phystype);
-                char *dev;
-                dev = get_blktap2_device(ctx, disk->physpath, type);
-                if (!dev)
-                    dev = make_blktap2_device(ctx, disk->physpath, type);
+        case PHYSTYPE_AIO:
+        case PHYSTYPE_QCOW:
+        case PHYSTYPE_QCOW2:
+        case PHYSTYPE_VHD:
+            if (libxl_blktap_enabled(ctx)) {
+                const char *dev = libxl_blktap_devpath(ctx,
+                                               disk->physpath, disk->phystype);
                 if (!dev)
                     return ERROR_FAIL;
                 flexarray_set(back, boffset++, "tapdisk-params");

@@ -1488,7 +1465,7 @@
 
             device.backend_kind = DEVICE_TAP;
             break;
-        }
+
         default:
             XL_LOG(ctx, XL_LOG_ERROR, "unrecognized disk physical type: %d\n", disk->phystype);
             return ERROR_INVAL;

@@ -1552,7 +1529,7 @@
 
 const char * libxl_device_disk_local_attach(struct libxl_ctx *ctx, libxl_device_disk *disk)
 {
-    char *dev = NULL;
+    const char *dev = NULL;
     int phystype = disk->phystype;
     switch (phystype) {
         case PHYSTYPE_PHY: {

@@ -1563,16 +1540,14 @@
         case PHYSTYPE_FILE:
             /* let's pretend is tap:aio for the moment */
             phystype = PHYSTYPE_AIO;
-        case PHYSTYPE_AIO: case PHYSTYPE_QCOW: case PHYSTYPE_QCOW2: case PHYSTYPE_VHD: {
-            const char *msg;
-            if (!tap_ctl_check(&msg)) {
-                const char *type = device_disk_string_of_phystype(phystype);
-                dev = get_blktap2_device(ctx, disk->physpath, type);
-                if (!dev)
-                    dev = make_blktap2_device(ctx, disk->physpath, type);
-            }
+        case PHYSTYPE_AIO:
+        case PHYSTYPE_QCOW:
+        case PHYSTYPE_QCOW2:
+        case PHYSTYPE_VHD:
+            if (libxl_blktap_enabled(ctx))
+                dev = libxl_blktap_devpath(ctx, disk->physpath, phystype);
             break;
-        }
+
         default:
             XL_LOG(ctx, XL_LOG_ERROR, "unrecognized disk physical type: %d\n", phystype);
             break;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Ian Jackson

2010-07-27 17:00 UTC #19 lore raw

Re: [PATCH 5/6] libxl: portiblity fixes

In reply to Christoph Egger

Christoph Egger writes ("[Xen-devel] [PATCH 5/6] libxl: portiblity fixes"):

tools/libxl/Makefile line 22

Quoted 3 lines

LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
tools/libxl/Makefile +2 -0
+LIBXL_OBJS-$(CONFIG_Linux) += libxl_linux.o
+LIBXL_OBJS-$(CONFIG_NetBSD) += libxl_netbsd.o

As previously discussed, these files should probably be called libxl_blktap2.c and libxl_noblktap2.c or some such.

tools/libxl/libxl_linux.c line 28

Quoted 3 lines

tools/libxl/libxl_linux.c new file +3 -0

+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype)

You seem to be moving code about by adding it in one patch and deleting it in another; as I mentioned before, that's not the right thing to do.

When submitting a patch series, every individual patch should make sense by itself and leave a sane and compilable tree.

Ian.

Christoph Egger

author

2010-07-28 09:17 UTC #20 lore raw

Re: [PATCH 5/6] libxl: portiblity fixes

Quoted 7 lines · On Tuesday 27 July 2010 19:00:07 Ian Jackson wrote:

Christoph Egger writes ("[Xen-devel] [PATCH 5/6] libxl: portiblity fixes"):
tools/libxl/Makefile line 22
Quoted 3 lines
LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
tools/libxl/Makefile +2 -0
+LIBXL_OBJS-$(CONFIG_Linux) += libxl_linux.o
+LIBXL_OBJS-$(CONFIG_NetBSD) += libxl_netbsd.o
As previously discussed, these files should probably be called libxl_blktap2.c and libxl_noblktap2.c or some such.

Discussed between you and Ian Campell. Both of you believe that blktap support on NetBSD will never exist, right ?

blktap support for NetBSD is under development and will be very different in its implementation.

Quoted 6 lines

tools/libxl/libxl_linux.c line 28
Quoted 3 lines
tools/libxl/libxl_linux.c new file +3 -0
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype)
You seem to be moving code about by adding it in one patch and deleting it in another;

In this case I call this 'factor out OS dependent code'.

Christoph

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Ian Jackson

2010-07-27 16:58 UTC #21 lore raw

Re: [PATCH 4/6] libxl: portiblity fixes

In reply to Christoph Egger

Christoph Egger writes ("[Xen-devel] [PATCH 4/6] libxl: portiblity fixes"):

tools/Rules.mk line 35

Quoted 7 lines

tools/Rules.mk +5 -0

+ifeq ($(CONFIG_Linux),y)
 CFLAGS_libblktapctl = -I$(XEN_BLKTAP2)/control -I$(XEN_BLKTAP2)/include $(CFLAGS_include)
 LDFLAGS_libblktapctl = -L$(XEN_BLKTAP2)/control -lblktapctl
+else
+CFLAGS_libblktapctl =
+LDFLAGS_libblktapctl =
+endif

I think this should be in the same patch as provides the stub implementation of the blktap functions for non-blktap systems.

Ian.

Christoph Egger

author

2010-07-28 11:50 UTC #22 lore raw

Re: [PATCH 4/6] libxl: portiblity fixes

Quoted 12 lines · On Tuesday 27 July 2010 18:58:26 Ian Jackson wrote:

Christoph Egger writes ("[Xen-devel] [PATCH 4/6] libxl: portiblity fixes"):
Review
Quoted 8 lines
+ifeq ($(CONFIG_Linux),y)
CFLAGS_libblktapctl = -I$(XEN_BLKTAP2)/control -I$(XEN_BLKTAP2)/include
$(CFLAGS_include) LDFLAGS_libblktapctl = -L$(XEN_BLKTAP2)/control
(quoted diff) +4 -1
-lblktapctl
+else
+CFLAGS_libblktapctl =
+LDFLAGS_libblktapctl =
+endif
I think this should be in the same patch as provides the stub implementation of the blktap functions for non-blktap systems.

Done.

Attached patch adds the stub implementations for non-blktap systems.

Signed-off-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

tools/Rules.mk +9 -0

diff -r 9ea1cf03ef55 tools/Rules.mk
@@ -26,10 +26,19 @@ LDFLAGS_libxenguest = -L$(XEN_LIBXC) -lx
 CFLAGS_libxenstore = -I$(XEN_XENSTORE) $(CFLAGS_include)
 LDFLAGS_libxenstore = -L$(XEN_XENSTORE) -lxenstore
 
+ifeq ($(CONFIG_Linux),y)
 LIBXL_BLKTAP = y
+else
+LIBXL_BLKTAP = n
+endif
 
+ifeq ($(LIBXL_BLKTAP),y)
 CFLAGS_libblktapctl = -I$(XEN_BLKTAP2)/control -I$(XEN_BLKTAP2)/include $(CFLAGS_include)
 LDFLAGS_libblktapctl = -L$(XEN_BLKTAP2)/control -lblktapctl
+else
+CFLAGS_libblktapctl =
+LDFLAGS_libblktapctl =
+endif
 
 X11_LDPATH = -L/usr/X11R6/$(LIBLEAFDIR)

tools/libxl/Makefile +2 -0

diff -r 9ea1cf03ef55 tools/libxl/Makefile
@@ -20,6 +20,8 @@ LIBS = $(LDFLAGS_libxenctrl) $(LDFLAGS_l
 LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
 ifeq ($(LIBXL_BLKTAP),y)
 LIBXL_OBJS-y += libxl_blktap2.c
+else
+LIBXL_OBJS-y += libxl_noblktap2.c
 endif
 
 LIBXL_OBJS = flexarray.o libxl.o libxl_pci.o libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o libxl_internal.o xenguest.o libxl_utils.o $(LIBXL_OBJS-y)

tools/libxl/libxl_noblktap2.c new file +29 -0

diff -r 9ea1cf03ef55 tools/libxl/libxl_noblktap2.c
@@ -0,0 +1,29 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only.
+ *
+ * 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 Lesser General Public License for more details.
+ */
+
+#include "libxl.h"
+#include "libxl_osdeps.h"
+#include "libxl_internal.h"
+
+int libxl_blktap_enabled(struct libxl_ctx *ctx)
+{
+    return 0;
+}
+
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype)
+{
+    return NULL;
+}

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

A message that is not in the archive (sent off-list or not yet received): [email protected]

Christoph Egger

author

2010-07-28 11:42 UTC #24 lore raw

PATCH 1/1 libxl: compile fix

Hi!

Attached patch fixes this compile error:

xl_cmdimpl.c: In function 'create_domain':
xl_cmdimpl.c:1099: warning: 'action' may be used uninitialized in this function

Signed-off-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

tools/libxl/xl_cmdimpl.c +1 -1

diff -r 9ea1cf03ef55 tools/libxl/xl_cmdimpl.c
@@ -1096,7 +1096,7 @@ static int handle_domain_death(struct li
                                struct domain_config *d_config, struct libxl_dominfo *info)
 {
     int restart = 0;
-    enum action_on_shutdown action;
+    enum action_on_shutdown action = ACTION_DESTROY;
 
     switch (info->shutdown_reason) {
     case SHUTDOWN_poweroff:

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Ian Campbell

2010-07-28 12:22 UTC #25 lore raw

Re: [PATCH] libxl: compile fix

Quoted 7 lines · Christoph Egger

Hi!
Attached patch fixes this compile error:
xl_cmdimpl.c: In function 'create_domain':
xl_cmdimpl.c:1099: warning: 'action' may be used uninitialized in this function

Thanks.

This can't actually happen in practice today since the switch statement covers all of the possible values of info->shutdown_reason. (in a previous version of the series which introduced this code shutdown_reason was an enum so the compiler knew this).

However to be robust it is probably worth adding a default: case to the switch and logging the unknown shutdown code.

Ian.

Subject: xl: log unknown domain shutdown reason and default to destroy

Signed-off-by Ian Campbell <[email protected]>

tools/libxl/xl_cmdimpl.c +3 -0

diff -r 479d042f25e4 tools/libxl/xl_cmdimpl.c
@@ -1113,6 +1113,9 @@ static int handle_domain_death(libxl_ctx
     case SHUTDOWN_watchdog:
         action = d_config->on_watchdog;
         break;
+    default:
+        LOG("Unknown shutdown reason code %s. Destroying domain.", info->shutdown_reason);
+        action = ACTION_DESTROY;
     }
 
     LOG("Action for shutdown reason code %d is %s", info->shutdown_reason, action_on_shutdown_names[action]);

Quoted 4 lines

Signed-off-by Christoph Egger <[email protected]>

Christoph Egger

author Acked-by

2010-07-28 13:03 UTC #26 lore raw

Re: [PATCH] libxl: compile fix

Quoted 39 lines · On Wednesday 28 July 2010 14:22:37 Ian Campbell wrote:

Quoted 7 lines · Christoph Egger
Hi!
Attached patch fixes this compile error:
xl_cmdimpl.c: In function 'create_domain':
xl_cmdimpl.c:1099: warning: 'action' may be used uninitialized in this function
Thanks.
This can't actually happen in practice today since the switch statement covers all of the possible values of info->shutdown_reason. (in a previous version of the series which introduced this code shutdown_reason was an enum so the compiler knew this).
However to be robust it is probably worth adding a default: case to the switch and logging the unknown shutdown code.
Ian.
Subject: xl: log unknown domain shutdown reason and default to destroy
Signed-off-by Ian Campbell <[email protected]>
diff -r 479d042f25e4 tools/libxl/xl_cmdimpl.c
tools/libxl/xl_cmdimpl.c +2 -0
@@ -1113,6 +1113,9 @@ static int handle_domain_death(libxl_ctx
     case SHUTDOWN_watchdog:
         action = d_config->on_watchdog;
         break;
+    default:
+        LOG("Unknown shutdown reason code %s. Destroying domain.",
 info->shutdown_reason); +        action = ACTION_DESTROY;
     }
 
     LOG("Action for shutdown reason code %d is %s", info->shutdown_reason,

Quoted 2 lines

action_on_shutdown_names[action]);

Acked-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Ian Jackson

2010-07-27 16:34 UTC #27 lore raw

Re: [PATCH 2/6] libxl: portiblity fixes

In reply to Christoph Egger

Christoph Egger writes ("[Xen-devel] [PATCH 2/6] libxl: portiblity fixes"):

tools/libxl/libxl_osdeps.h line 27

Quoted 8 lines

tools/libxl/libxl_osdeps.h +2 -0

@@ -23,6 +23,8 @@
 
 #define _GNU_SOURCE
 
+#include <libxl_internal.h>
+

This is wrong. libxl_osdeps.h should not include libxl_internal.h.

tools/libxl/libxl_osdeps.h line 42

Quoted 4 lines

tools/libxl/libxl_osdeps.h +4 -0

+/* libxl_blktap_enabled:
+ *    return true if blktap/blktap2 support is available.
+ */
+int libxl_blktap_enabled(struct libxl_ctx *ctx);

This is not what libxl_osdeps.h is for. These kind of functions can be declared in a new section in libxl_internal.h.

Also, you should divide your patches conceptually, rather than according to which files they touch.

This patch is wrong because it introduces a couple of function declarations but it does not introduce the definitions; your later patch which introduces the definitions is wrong because it introduces some functions which are intended to replace existing code, but the patch does not replace the existing code and the new functions are not called anywhere in that patch.

Ian.

Christoph Egger

author

2010-07-28 09:06 UTC #28 lore raw

Re: [PATCH 2/6] libxl: portiblity fixes

Quoted 23 lines · On Tuesday 27 July 2010 18:34:21 Ian Jackson wrote:

Christoph Egger writes ("[Xen-devel] [PATCH 2/6] libxl: portiblity fixes"):
tools/libxl/libxl_osdeps.h line 27
Quoted 8 lines
tools/libxl/libxl_osdeps.h +2 -0
@@ -23,6 +23,8 @@
 
 #define _GNU_SOURCE
 
+#include <libxl_internal.h>
+
This is wrong. libxl_osdeps.h should not include libxl_internal.h.
tools/libxl/libxl_osdeps.h line 42
Quoted 4 lines
tools/libxl/libxl_osdeps.h +4 -0
+/* libxl_blktap_enabled:
+ *    return true if blktap/blktap2 support is available.
+ */
+int libxl_blktap_enabled(struct libxl_ctx *ctx);
This is not what libxl_osdeps.h is for. These kind of functions can be declared in a new section in libxl_internal.h.
Also, you should divide your patches conceptually, rather than according to which files they touch.

Quoted 6 lines

This patch is wrong because it introduces a couple of function declarations but it does not introduce the definitions; your later patch which introduces the definitions is wrong because it introduces some functions which are intended to replace existing code, but the patch does not replace the existing code and the new functions are not called anywhere in that patch.

The function declarations are the API and the function defintions are the OS dependent implementations of the API. Implementations and use of the API is used in different patches. This is my understanding of defining and implementing an API in C.
blktap support for linux and netbsd are very different in their implementation.
In netbsd, blktap will be implemented using puffs
(http://netbsd.gw.com/cgi-bin/man-cgi?puffs+3+NetBSD-current)

Christoph

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Keir Fraser

2010-07-28 09:21 UTC #29 lore raw

Re: [PATCH 2/6] libxl: portiblity fixes

Quoted 7 lines · Christoph Egger

Quoted 6 lines
This patch is wrong because it introduces a couple of function declarations but it does not introduce the definitions; your later patch which introduces the definitions is wrong because it introduces some functions which are intended to replace existing code, but the patch does not replace the existing code and the new functions are not called anywhere in that patch.

Quoted 5 lines

The function declarations are the API and the function defintions are the OS dependent implementations of the API. Implementations and use of the API is used in different patches. This is my understanding of defining and implementing an API in C.

I find that kind of way of splitting up a patch series annoying as well. As Ian said, we want each patch to be a logical and separate whole. That means providing an interface *and* its implementation. Possibly its users as well, depending on how complicated that bit is -- it's certainly arguable they belong in a separate patch, at least.

Quoted 4 lines

blktap support for linux and netbsd are very different in their implementation.
In netbsd, blktap will be implemented using puffs
(http://netbsd.gw.com/cgi-bin/man-cgi?puffs+3+NetBSD-current)

A bit of a sidestep I know, but: shouldn't the blktap library be hiding this osdep stuff?

-- Keir

Christoph Egger

author

2010-07-28 09:30 UTC #30 lore raw

Re: [PATCH 2/6] libxl: portiblity fixes

Quoted 20 lines · On Wednesday 28 July 2010 11:21:54 Keir Fraser wrote:

Quoted 7 lines · Christoph Egger
Quoted 6 lines
This patch is wrong because it introduces a couple of function declarations but it does not introduce the definitions; your later patch which introduces the definitions is wrong because it introduces some functions which are intended to replace existing code, but the patch does not replace the existing code and the new functions are not called anywhere in that patch.
Quoted 5 lines
The function declarations are the API and the function defintions are the OS dependent implementations of the API. Implementations and use of the API is used in different patches. This is my understanding of defining and implementing an API in C.
I find that kind of way of splitting up a patch series annoying as well. As Ian said, we want each patch to be a logical and separate whole. That means providing an interface *and* its implementation. Possibly its users as well, depending on how complicated that bit is -- it's certainly arguable they belong in a separate patch, at least.

Quoted 7 lines

Quoted 4 lines
blktap support for linux and netbsd are very different in their implementation.
In netbsd, blktap will be implemented using puffs
(http://netbsd.gw.com/cgi-bin/man-cgi?puffs+3+NetBSD-current)
A bit of a sidestep I know, but: shouldn't the blktap library be hiding this osdep stuff?

That's a good point. With this in mind, having libxl_blktap.c and libxl_noblktap.c as the Ian's suggested makes perfect sense to me.

Christoph

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Christoph Egger

author

2010-07-28 11:39 UTC #31 lore raw

PATCH 1/1 libxl: move blktap specific code into libxl_blktap.c

In reply to Ian Jackson

Move blktap specific code into libxl_blktap.c

Signed-off-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

tools/Rules.mk +2 -0

diff -r 4f39505426cb tools/Rules.mk
@@ -26,6 +26,8 @@ LDFLAGS_libxenguest = -L$(XEN_LIBXC) -lx
 CFLAGS_libxenstore = -I$(XEN_XENSTORE) $(CFLAGS_include)
 LDFLAGS_libxenstore = -L$(XEN_XENSTORE) -lxenstore
 
+LIBXL_BLKTAP = y
+
 CFLAGS_libblktapctl = -I$(XEN_BLKTAP2)/control -I$(XEN_BLKTAP2)/include $(CFLAGS_include)
 LDFLAGS_libblktapctl = -L$(XEN_BLKTAP2)/control -lblktapctl

tools/libxl/Makefile +4 -0

diff -r 4f39505426cb tools/libxl/Makefile
@@ -18,6 +18,10 @@ CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_
 LIBS = $(LDFLAGS_libxenctrl) $(LDFLAGS_libxenguest) $(LDFLAGS_libxenstore) $(LDFLAGS_libblktapctl) $(UTIL_LIBS)
 
 LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
+ifeq ($(LIBXL_BLKTAP),y)
+LIBXL_OBJS-y += libxl_blktap2.c
+endif
+
 LIBXL_OBJS = flexarray.o libxl.o libxl_pci.o libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o libxl_internal.o xenguest.o libxl_utils.o $(LIBXL_OBJS-y)
 
 AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h

tools/libxl/libxl.c +16 ~~-41~~

94 lines not on this page · raw message

tools/libxl/libxl_blktap2.c new file +51 -0

51 lines not on this page · raw message

tools/libxl/libxl_internal.h +19 -0

diff -r 4f39505426cb tools/libxl/libxl_internal.h
@@ -225,5 +225,24 @@ char *libxl_abs_path(struct libxl_ctx *c
 /* Error handling */
 int libxl_xc_error(int xc_err);
 
+/*
+ * blktap2 support
+ */
+
+/* libxl_blktap_enabled:
+ *    return true if blktap/blktap2 support is available.
+ */
+int libxl_blktap_enabled(struct libxl_ctx *ctx);
+
+/* libxl_blktap_devpath:
+ *    Argument: path and disk image as specified in config file.
+ *      The type specifies whether this is aio, qcow, qcow2, etc.
+ *    returns device path xenstore wants to have. returns NULL
+ *      if no device corresponds to the disk.
+ */
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype);
+
 #endif

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Ian Jackson

2010-07-29 15:02 UTC #32 lore raw

Re: [PATCH] libxl: move blktap specific code into libxl_blktap.c

Christoph Egger writes ("[Xen-devel] [PATCH] libxl: move blktap specific code into libxl_blktap.c"):

Quoted 1 line

Move blktap specific code into libxl_blktap.c

Thanks, this is going in the right direction. But can you please split up the moving code into a different file, from the changes to that code ?

As it is it is almost impossible to see what changes you have made to the code you are moving, as we have

diff old-file
stuff
- old
- code
more stuff
diff new-file
+ newly
+ reorganised
+ code

If the code needs to be reorganised so that it can be moved, you should do this in two patches, so we end up with:

[PATCH 1/2] reorganise preparatory to moving

diff old-file
stuff
- old
+ newly
+ reorganised
code
more stuff

[PATCH 2/2] move blktap-specific code to libxl_blktap.c Purely moving code about, no changes.

diff old-file
stuff
- newly
- reorganised
- code
old stuff
diff new-file
+ newly
+ reorganised
+ code

Ian.

Stefano Stabellini

2010-07-29 15:08 UTC #33 lore raw

Re: [PATCH] libxl: move blktap specific code into libxl_blktap.c

Quoted 37 lines · Ian Jackson

Christoph Egger writes ("[Xen-devel] [PATCH] libxl: move blktap specific code into libxl_blktap.c"):
Quoted 1 line
Move blktap specific code into libxl_blktap.c
Thanks, this is going in the right direction. But can you please split up the moving code into a different file, from the changes to that code ?
As it is it is almost impossible to see what changes you have made to the code you are moving, as we have
diff old-file
stuff
- old
- code
more stuff
diff new-file
+ newly
+ reorganised
+ code
If the code needs to be reorganised so that it can be moved, you should do this in two patches, so we end up with:
[PATCH 1/2] reorganise preparatory to moving
diff old-file
stuff
- old
+ newly
+ reorganised
code
more stuff
[PATCH 2/2] move blktap-specific code to libxl_blktap.c Purely moving code about, no changes.

Quoted 11 lines

diff old-file
stuff
- newly
- reorganised
- code
old stuff
diff new-file
+ newly
+ reorganised
+ code

I take this chance to say that I greatly prefer inline patches to attachments.

Christoph Egger

author

2010-07-29 15:41 UTC #34 lore raw

Re: [PATCH] libxl: move blktap specific code into libxl_blktap.c

In reply to Ian Jackson

Quoted 1 line · On Thursday 29 July 2010 17:02:25 Ian Jackson wrote:

Christoph Egger writes ("[Xen-devel] [PATCH] libxl: move blktap specific

code into libxl_blktap.c"):

Quoted 8 lines

Quoted 1 line
Move blktap specific code into libxl_blktap.c
Thanks, this is going in the right direction. But can you please split up the moving code into a different file, from the changes to that code ?
As it is it is almost impossible to see what changes you have made to the code you are moving, as we have

Is it enough when I explain the change and adjust the commit message accordingly ?

The libxl_blktap_devpath implementation merges
get_blktap2_devices() and make_blktap2_devices().

Quoted 27 lines

diff old-file
stuff
- old
- code
more stuff
diff new-file
+ newly
+ reorganised
+ code
If the code needs to be reorganised so that it can be moved, you should do this in two patches, so we end up with:
[PATCH 1/2] reorganise preparatory to moving
diff old-file
stuff
- old
+ newly
+ reorganised
code
more stuff
[PATCH 2/2] move blktap-specific code to libxl_blktap.c Purely moving code about, no changes.

Quoted 10 lines

diff old-file
stuff
- newly
- reorganised
- code
old stuff
diff new-file
+ newly
+ reorganised
+ code

hmm... doing it vice versa is easier to do because the new code merges to functions (see above).

First patch contains the move and the second patch contains the merge.

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Ian Jackson

2010-07-29 15:46 UTC #35 lore raw

Re: [PATCH] libxl: move blktap specific code into libxl_blktap.c

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: move blktap specific code into libxl_blktap.c"):

Quoted 2 lines

Is it enough when I explain the change and adjust the commit message accordingly ?

No, because we want to be able to review the changes, and people who look at the repo history in the future will want to be able to see what was done and why.

Ian.

Ian Jackson

2010-07-29 15:48 UTC #36 lore raw

Re: [PATCH] libxl: move blktap specific code into libxl_blktap.c

In reply to Christoph Egger

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: move blktap specific code into libxl_blktap.c"):

Quoted 2 lines · On Thursday 29 July 2010 17:02:25 Ian Jackson wrote:

Quoted 1 line
[PATCH 1/2] reorganise preparatory to moving

...

Quoted 2 lines

Quoted 2 lines
[PATCH 2/2] move blktap-specific code to libxl_blktap.c Purely moving code about, no changes.

...

Quoted 2 lines

hmm... doing it vice versa is easier to do because the new code merges to functions (see above).

Sorry, I didn't see that comment (because you didn't trim the quoted text (-:.)

Quoted 2 lines

First patch contains the move and the second patch contains the merge.

That would be fine. The point is not to mix moving code about with anything else.

Ian.

Christoph Egger

author

2010-07-29 16:46 UTC #37 lore raw

PATCH 1/1 libxl: move blktap-specific code into libxl_blktap.c

libxl: move blktap-specific code into libxl_blktap.c

Signed-off-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

tools/Rules.mk +2 -0

diff -r bb230a0aa455 tools/Rules.mk
@@ -26,6 +26,8 @@ LDFLAGS_libxenguest = -L$(XEN_LIBXC) -lx
 CFLAGS_libxenstore = -I$(XEN_XENSTORE) $(CFLAGS_include)
 LDFLAGS_libxenstore = -L$(XEN_XENSTORE) -lxenstore
 
+LIBXL_BLKTAP = y
+
 CFLAGS_libblktapctl = -I$(XEN_BLKTAP2)/control -I$(XEN_BLKTAP2)/include $(CFLAGS_include)
 LDFLAGS_libblktapctl = -L$(XEN_BLKTAP2)/control -lblktapctl

tools/libxl/Makefile +4 -0

diff -r bb230a0aa455 tools/libxl/Makefile
@@ -18,6 +18,10 @@ CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_
 LIBS = $(LDFLAGS_libxenctrl) $(LDFLAGS_libxenguest) $(LDFLAGS_libxenstore) $(LDFLAGS_libblktapctl) $(UTIL_LIBS)
 
 LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
+ifeq ($(LIBXL_BLKTAP),y)
+LIBXL_OBJS-y += libxl_blktap2.c
+endif
+
 LIBXL_OBJS = flexarray.o libxl.o libxl_pci.o libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o libxl_internal.o xenguest.o libxl_utils.o $(LIBXL_OBJS-y)
 
 AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h

tools/libxl/libxl.c +16 ~~-41~~

94 lines not on this page · raw message

tools/libxl/libxl_blktap2.c new file +60 -0

60 lines not on this page · raw message

tools/libxl/libxl_internal.h +19 -0

diff -r bb230a0aa455 tools/libxl/libxl_internal.h
@@ -225,5 +225,24 @@ char *libxl_abs_path(libxl_ctx *ctx, cha
 /* Error handling */
 int libxl_xc_error(int xc_err);
 
+/*
+ * blktap2 support
+ */
+
+/* libxl_blktap_enabled:
+ *    return true if blktap/blktap2 support is available.
+ */
+int libxl_blktap_enabled(libxl_ctx *ctx);
+
+/* libxl_blktap_devpath:
+ *    Argument: path and disk image as specified in config file.
+ *      The type specifies whether this is aio, qcow, qcow2, etc.
+ *    returns device path xenstore wants to have. returns NULL
+ *      if no device corresponds to the disk.
+ */
+const char *libxl_blktap_devpath(libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype);
+
 #endif

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Ian Jackson

2010-07-29 18:02 UTC #38 lore raw

Re: [PATCH] libxl: move blktap-specific code into libxl_blktap.c

Christoph Egger writes ("[PATCH] libxl: move blktap-specific code into libxl_blktap.c"):

Quoted 1 line

libxl: move blktap-specific code into libxl_blktap.c

Except, once again, this is not _pure_ motion. You need to split the patch up so that, in the code motion patch,

diff -u <(hg diff tools/libxl/libxl.c | perl -pe 's/^[-+]/ $& eq "-" ? "+" : "-" /e' ) <(hg diff tools/libxl/libxl_blktap.c)

doesn't produce any substantive output.

Ian.

Christoph Egger

author

2010-07-30 08:40 UTC #39 lore raw

Re: [PATCH] libxl: move blktap-specific code into libxl_blktap.c

Quoted 1 line · On Thursday 29 July 2010 20:02:09 Ian Jackson wrote:

Christoph Egger writes ("[PATCH] libxl: move blktap-specific code into

libxl_blktap.c"):

Quoted 9 lines

Quoted 1 line
libxl: move blktap-specific code into libxl_blktap.c
Except, once again, this is not _pure_ motion. You need to split the patch up so that, in the code motion patch,
diff -u <(hg diff tools/libxl/libxl.c | perl -pe 's/^[-+]/ $& eq "-" ?
"+" : "-" /e' ) <(hg diff tools/libxl/libxl_blktap.c)
doesn't produce any substantive output.

I'm sorry, I don't know perl. And this command fails with syntax error: '(' unexpected.

I haven't send the patch yesterday. It was late for me. You seem to respond always in the late afternoon (CEST). In which timezone are you ?

Christoph

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Christoph Egger

author

2010-07-30 08:45 UTC #40 lore raw

Re: Re: [PATCH] libxl: move blktap-specific code into libxl_blktap.c

Quoted 12 lines · On Friday 30 July 2010 10:40:27 Christoph Egger wrote:

Quoted 1 line · On Thursday 29 July 2010 20:02:09 Ian Jackson wrote:
Christoph Egger writes ("[PATCH] libxl: move blktap-specific code into
libxl_blktap.c"):
Quoted 8 lines
Quoted 1 line
libxl: move blktap-specific code into libxl_blktap.c
Except, once again, this is not _pure_ motion. You need to split the patch up so that, in the code motion patch,
diff -u <(hg diff tools/libxl/libxl.c | perl -pe 's/^[-+]/ $& eq "-" ?
"+" : "-" /e' ) <(hg diff tools/libxl/libxl_blktap.c)

Quoted 6 lines

Quoted 1 line
doesn't produce any substantive output.
I'm sorry, I don't know perl. And this command fails with syntax error: '(' unexpected.
I haven't send the patch yesterday.

Wanted to say: I haven't send the patch with the change yesterday.

Quoted 5 lines

It was late for me.
You seem to respond always in the late afternoon (CEST). In which timezone are you ?
Christoph

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Ian Jackson

2010-07-27 16:31 UTC #41 lore raw

Re: [PATCH 1/6] libxl: portiblity fixes

In reply to Christoph Egger

Christoph Egger writes ("[Xen-devel] [PATCH 1/6] libxl: portiblity fixes"):

tools/libxl/libxl_bootloader.c line 27

Quoted 12 lines

#include <string.h>

tools/libxl/libxl_bootloader.c +8 -1

-#include <pty.h>
 #include <unistd.h>
 #include <fcntl.h>
+#include <termios.h>
+#if defined(__NetBSD__) || defined(__OpenBSD__)
+#include <util.h>
+#elif defined(__linux__)
+#include <pty.h>
+#elif defined(__sun__)
+#include <stropts.h>
+#endif

This should be in libxl_osdeps.h as previously discussed.

Ian.

Christoph Egger

author

2010-07-28 09:49 UTC #42 lore raw

Re: [PATCH 1/6] libxl: portiblity fixes

Quoted 15 lines · On Tuesday 27 July 2010 18:31:24 Ian Jackson wrote:

Christoph Egger writes ("[Xen-devel] [PATCH 1/6] libxl: portiblity fixes"):
Review
Quoted 12 lines
#include <string.h>
(quoted diff) +8 -1
-#include <pty.h>
 #include <unistd.h>
 #include <fcntl.h>
+#include <termios.h>
+#if defined(__NetBSD__) || defined(__OpenBSD__)
+#include <util.h>
+#elif defined(__linux__)
+#include <pty.h>
+#elif defined(__sun__)
+#include <stropts.h>
+#endif
This should be in libxl_osdeps.h as previously discussed.

Done.

Take over portability fixes from tools/console:
include proper headers for openpty(3) and use $(UTIL_LIBS) include termios.h for tcgetattr & co.

Signed-off-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

tools/libxl/Makefile +1 -1

diff -r c04dad02ffa3 tools/libxl/Makefile
@@ -15,7 +15,7 @@ CFLAGS += -Werror -Wno-format-zero-lengt
 CFLAGS += -I. -fPIC
 CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest) $(CFLAGS_libxenstore) $(CFLAGS_libblktapctl)
 
-LIBS = $(LDFLAGS_libxenctrl) $(LDFLAGS_libxenguest) $(LDFLAGS_libxenstore) $(LDFLAGS_libblktapctl) -lutil
+LIBS = $(LDFLAGS_libxenctrl) $(LDFLAGS_libxenguest) $(LDFLAGS_libxenstore) $(LDFLAGS_libblktapctl) $(UTIL_LIBS)
 
 LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
 LIBXL_OBJS = flexarray.o libxl.o libxl_pci.o libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o libxl_internal.o xenguest.o libxl_utils.o $(LIBXL_OBJS-y)

tools/libxl/libxl_bootloader.c +1 -1

diff -r c04dad02ffa3 tools/libxl/libxl_bootloader.c
@@ -15,9 +15,9 @@
 #include "libxl_osdeps.h"
 
 #include <string.h>
-#include <pty.h>
 #include <unistd.h>
 #include <fcntl.h>
+#include <termios.h>
 
 #include <sys/stat.h>
 #include <sys/types.h>

tools/libxl/libxl_osdeps.h +8 -0

diff -r c04dad02ffa3 tools/libxl/libxl_osdeps.h
@@ -23,6 +23,14 @@
 
 #define _GNU_SOURCE
 
+#if defined(__NetBSD__) || defined(__OpenBSD__)
+#include <util.h>
+#elif defined(__linux__)
+#include <pty.h>
+#elif defined(__sun__)
+#include <stropts.h>
+#endif
+
 #ifdef NEED_OWN_ASPRINTF
 #include <stdarg.h>

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Christoph Egger

author

2010-07-27 12:17 UTC #43 lore raw

cover libxl: portiblity fixes

In reply to Ian Jackson

patch 1:
Take over portability fixes from tools/console:
include proper headers for openpty(3) and use $(UTIL_LIBS) include termios.h for tcgetattr & co.

patch 2:
add generic libxl_blktap interface

patch 3:
device_physdisk_major_minor does not intend to modify physpath. Also needed to make gcc happy with linux blktap implemenation (required to make patch 5 compile).

patch 4:
blktapctl is only build on Linux. Disable use on non-Linux.

patch 5:
add bltap implementations for linux. netbsd implementation is under development so just add a stub for now to allow build.

patch 6:
Make use of libxl_blktap interface introduced in patch 2. This removes code from libxl.c which uses blktapctl and therefore broke build on non-Linux.

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Ian Jackson

2010-07-27 16:30 UTC #44 lore raw

Re: [PATCH 0/6] libxl: portiblity fixes

Christoph Egger writes ("[Xen-devel] [PATCH 0/6] libxl: portiblity fixes"):

Quoted 4 lines

patch 1:
Take over portability fixes from tools/console:
include proper headers for openpty(3) and use $(UTIL_LIBS) include termios.h for tcgetattr & co.

Thanks, this is better. It is more usual to submit the commit message for each patch in the same email as each patch. I'll reply to each patch separately.

Ian.