[PATCH] libxl: blktap2 portiblity fixes

[PATCH] libxl: blktap2 portiblity fixes

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 481 bytes --]


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 <Christoph.Egger@amd.com>

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

[-- Attachment #2: xen_tools_libxl2.diff --]
[-- Type: text/x-diff, Size: 8558 bytes --]

diff -r 6d67482c3649 -r 63901ae6eb20 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c
+++ b/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;
diff -r 6d67482c3649 -r 63901ae6eb20 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c
+++ b/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)
diff -r 6d67482c3649 -r 63901ae6eb20 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h
+++ b/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,
diff -r 6d67482c3649 -r 63901ae6eb20 tools/libxl/libxl_linux.c
--- /dev/null
+++ b/tools/libxl/libxl_linux.c
@@ -0,0 +1,50 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <Christoph.Egger@amd.com>
+ *
+ * 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 "tap-ctl.h"
+
+int libxl_blktap_enabled(struct libxl_ctx *ctx)
+{
+    const char *msg;
+    return !tap_ctl_check(&msg);
+}
+
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype)
+{
+    const char *type, *devname;
+    char *params;
+    int minor, err;
+
+    type = device_disk_string_of_type(phystype);
+    minor = tap_ctl_find_minor(type, disk);
+    if (minor >= 0) {
+        devname = libxl_sprintf(ctx, "/dev/xen/blktap-2/tapdev%d", minor);
+        if (devname)
+            return devname;
+    }
+
+    params = libxl_sprintf(ctx, "%s:%s", type, disk);
+    err = tap_ctl_create(params, &devname);
+    if (!err) {
+        libxl_ptr_add(ctx, devname);
+        return devname;
+    }
+
+    return NULL;
+}
diff -r 6d67482c3649 -r 63901ae6eb20 tools/libxl/libxl_netbsd.c
--- /dev/null
+++ b/tools/libxl/libxl_netbsd.c
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <Christoph.Egger@amd.com>
+ *
+ * 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;
+}
diff -r 6d67482c3649 -r 63901ae6eb20 tools/libxl/libxl_osdeps.h
--- a/tools/libxl/libxl_osdeps.h
+++ b/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

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] libxl: blktap2 portiblity fixes

[Stefano Stabellini]

On Tue, 20 Jul 2010, Christoph Egger wrote:
> 
> 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 <Christoph.Egger@amd.com>
> 


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

Re: [PATCH] libxl: blktap2 portiblity fixes

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

On Tuesday 20 July 2010 18:46:30 Stefano Stabellini wrote:
> On Tue, 20 Jul 2010, Christoph Egger wrote:
> > 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 <Christoph.Egger@amd.com>
>
> 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 <Christoph.Egger@amd.com>


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

[-- Attachment #2: xen_tools_libxl.diff --]
[-- Type: text/x-diff, Size: 9530 bytes --]

diff -r c7a7179810b6 -r 784d73e565cc tools/libxl/Makefile
--- a/tools/libxl/Makefile
+++ b/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
diff -r c7a7179810b6 -r 784d73e565cc tools/libxl/libxl.c
--- a/tools/libxl/libxl.c
+++ b/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)
 
@@ -1369,27 +1368,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;
@@ -1438,14 +1416,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");
@@ -1466,7 +1443,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;
@@ -1530,7 +1507,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: {
@@ -1541,16 +1518,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;
diff -r c7a7179810b6 -r 784d73e565cc tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c
+++ b/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)
diff -r c7a7179810b6 -r 784d73e565cc tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h
+++ b/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,
diff -r c7a7179810b6 -r 784d73e565cc tools/libxl/libxl_linux.c
--- /dev/null
+++ b/tools/libxl/libxl_linux.c
@@ -0,0 +1,50 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <Christoph.Egger@amd.com>
+ *
+ * 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 "tap-ctl.h"
+
+int libxl_blktap_enabled(struct libxl_ctx *ctx)
+{
+    const char *msg;
+    return !tap_ctl_check(&msg);
+}
+
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype)
+{
+    const char *type, *devname;
+    char *params;
+    int minor, err;
+
+    type = device_disk_string_of_type(phystype);
+    minor = tap_ctl_find_minor(type, disk);
+    if (minor >= 0) {
+        devname = libxl_sprintf(ctx, "/dev/xen/blktap-2/tapdev%d", minor);
+        if (devname)
+            return devname;
+    }
+
+    params = libxl_sprintf(ctx, "%s:%s", type, disk);
+    err = tap_ctl_create(params, &devname);
+    if (!err) {
+        libxl_ptr_add(ctx, devname);
+        return devname;
+    }
+
+    return NULL;
+}
diff -r c7a7179810b6 -r 784d73e565cc tools/libxl/libxl_netbsd.c
--- /dev/null
+++ b/tools/libxl/libxl_netbsd.c
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <Christoph.Egger@amd.com>
+ *
+ * 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;
+}
diff -r c7a7179810b6 -r 784d73e565cc tools/libxl/libxl_osdeps.h
--- a/tools/libxl/libxl_osdeps.h
+++ b/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

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] libxl: blktap2 portiblity fixes

[Stefano Stabellini]

On Wed, 21 Jul 2010, Christoph Egger wrote:
> On Tuesday 20 July 2010 18:46:30 Stefano Stabellini wrote:
> > On Tue, 20 Jul 2010, Christoph Egger wrote:
> > > 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 <Christoph.Egger@amd.com>
> >
> > 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 <Christoph.Egger@amd.com>
> 

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

Re: [PATCH] libxl: blktap2 portiblity fixes

[Ian Jackson]

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity fixes"):
> 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 <Christoph.Egger@amd.com>

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.

Re: [PATCH] libxl: blktap2 portiblity fixes

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]

On Friday 23 July 2010 20:05:50 Ian Jackson wrote:
> Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity 
fixes"):
> > 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 <Christoph.Egger@amd.com>
>
> 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.

> 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 <Christoph.Egger@amd.com>

> that doesn't break the build 
   ... on Linux you mean ?

> Then I'll test and apply it.

Christoph


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

[-- Attachment #2: xen_tools_libxl.diff --]
[-- Type: text/x-diff, Size: 10888 bytes --]

diff -r 2b768d52bc7f tools/Rules.mk
--- a/tools/Rules.mk	Sun Jul 25 22:20:47 2010 +0100
+++ b/tools/Rules.mk	Mon Jul 26 12:41:02 2010 +0200
@@ -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)
 
diff -r 2b768d52bc7f tools/libxl/Makefile
--- a/tools/libxl/Makefile	Sun Jul 25 22:20:47 2010 +0100
+++ b/tools/libxl/Makefile	Mon Jul 26 12:41:02 2010 +0200
@@ -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
diff -r 2b768d52bc7f tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Sun Jul 25 22:20:47 2010 +0100
+++ b/tools/libxl/libxl.c	Mon Jul 26 12:41:02 2010 +0200
@@ -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 @@ 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;
@@ -1460,14 +1438,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");
@@ -1488,7 +1465,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;
@@ -1552,7 +1529,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: {
@@ -1563,16 +1540,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;
diff -r 2b768d52bc7f tools/libxl/libxl_bootloader.c
--- a/tools/libxl/libxl_bootloader.c	Sun Jul 25 22:20:47 2010 +0100
+++ b/tools/libxl/libxl_bootloader.c	Mon Jul 26 12:41:02 2010 +0200
@@ -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>
diff -r 2b768d52bc7f tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Sun Jul 25 22:20:47 2010 +0100
+++ b/tools/libxl/libxl_device.c	Mon Jul 26 12:41:02 2010 +0200
@@ -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)
diff -r 2b768d52bc7f tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Sun Jul 25 22:20:47 2010 +0100
+++ b/tools/libxl/libxl_internal.h	Mon Jul 26 12:41:02 2010 +0200
@@ -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,
diff -r 2b768d52bc7f tools/libxl/libxl_linux.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/libxl_linux.c	Mon Jul 26 12:41:02 2010 +0200
@@ -0,0 +1,50 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <Christoph.Egger@amd.com>
+ *
+ * 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 "tap-ctl.h"
+
+int libxl_blktap_enabled(struct libxl_ctx *ctx)
+{
+    const char *msg;
+    return !tap_ctl_check(&msg);
+}
+
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype)
+{
+    const char *type;
+    char *params, *devname;
+    int minor, err;
+
+    type = device_disk_string_of_phystype(phystype);
+    minor = tap_ctl_find_minor(type, disk);
+    if (minor >= 0) {
+        devname = libxl_sprintf(ctx, "/dev/xen/blktap-2/tapdev%d", minor);
+        if (devname)
+            return devname;
+    }
+
+    params = libxl_sprintf(ctx, "%s:%s", type, disk);
+    err = tap_ctl_create(params, &devname);
+    if (!err) {
+        libxl_ptr_add(ctx, devname);
+        return devname;
+    }
+
+    return NULL;
+}
diff -r 2b768d52bc7f tools/libxl/libxl_netbsd.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/libxl_netbsd.c	Mon Jul 26 12:41:02 2010 +0200
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <Christoph.Egger@amd.com>
+ *
+ * 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;
+}
diff -r 2b768d52bc7f tools/libxl/libxl_osdeps.h
--- a/tools/libxl/libxl_osdeps.h	Sun Jul 25 22:20:47 2010 +0100
+++ b/tools/libxl/libxl_osdeps.h	Mon Jul 26 12:41:02 2010 +0200
@@ -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

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] libxl: blktap2 portiblity fixes

[Ian Jackson]

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity fixes"):
> 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.

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

> diff -r 2b768d52bc7f tools/libxl/libxl_bootloader.c
> --- a/tools/libxl/libxl_bootloader.c	Sun Jul 25 22:20:47 2010 +0100
> +++ b/tools/libxl/libxl_bootloader.c	Mon Jul 26 12:41:02 2010 +0200
> @@ -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

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

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

Re: [PATCH] libxl: blktap2 portiblity fixes

[Ian Campbell]

On Mon, 2010-07-26 at 15:53 +0100, Ian Jackson wrote:
>   * 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.

Re: [PATCH] libxl: blktap2 portiblity fixes

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity fixes"):
> On Mon, 2010-07-26 at 15:53 +0100, Ian Jackson wrote:
> >   * 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.

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

Re: [PATCH] libxl: blktap2 portiblity fixes

[Christoph Egger]

On Monday 26 July 2010 17:12:34 Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity 
fixes"):
> > On Mon, 2010-07-26 at 15:53 +0100, Ian Jackson wrote:
> > >   * 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.
>
> > (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.


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

Re: [PATCH] libxl: blktap2 portiblity fixes

[Christoph Egger]

On Monday 26 July 2010 16:53:02 Ian Jackson wrote:
> Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity 
fixes"):
> > 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.
>
> > 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.

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

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.

> > diff -r 2b768d52bc7f tools/libxl/libxl_bootloader.c
> > --- a/tools/libxl/libxl_bootloader.c	Sun Jul 25 22:20:47 2010 +0100
> > +++ b/tools/libxl/libxl_bootloader.c	Mon Jul 26 12:41:02 2010 +0200
> > @@ -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
>
> 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 ?

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


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

Re: [PATCH] libxl: blktap2 portiblity fixes

[Ian Jackson]

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity fixes"):
> On Monday 26 July 2010 16:53:02 Ian Jackson wrote:
> > Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity 
> fixes"):
> > > 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.
> >
> > > 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.

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

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

Re: [PATCH] libxl: blktap2 portiblity fixes

[Christoph Egger]

On Monday 26 July 2010 18:36:56 Ian Jackson wrote:
> Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity 
fixes"):
> > On Monday 26 July 2010 16:53:02 Ian Jackson wrote:
> > > Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2
> > > portiblity
> >
> > fixes"):
> > > > 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.
> > >
> > > > 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


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

[PATCH 1/6] libxl: portiblity fixes

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 323 bytes --]


Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


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

[-- Attachment #2: xen_tools_libxl1.diff --]
[-- Type: text/x-diff, Size: 1310 bytes --]

diff -r 3a2899ebb8e2 -r 993ee4f54b2e tools/libxl/Makefile
--- a/tools/libxl/Makefile	Tue Jul 27 12:24:53 2010 +0200
+++ b/tools/libxl/Makefile	Tue Jul 27 13:48:38 2010 +0200
@@ -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)
diff -r 3a2899ebb8e2 -r 993ee4f54b2e tools/libxl/libxl_bootloader.c
--- a/tools/libxl/libxl_bootloader.c	Tue Jul 27 12:24:53 2010 +0200
+++ b/tools/libxl/libxl_bootloader.c	Tue Jul 27 13:48:38 2010 +0200
@@ -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>

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 2/6] libxl: portiblity fixes

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 323 bytes --]

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>



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

[-- Attachment #2: xen_tools_libxl2.diff --]
[-- Type: text/x-diff, Size: 1058 bytes --]

diff -r 993ee4f54b2e -r 1ca40043b632 tools/libxl/libxl_osdeps.h
--- a/tools/libxl/libxl_osdeps.h	Tue Jul 27 13:48:38 2010 +0200
+++ b/tools/libxl/libxl_osdeps.h	Tue Jul 27 13:53:12 2010 +0200
@@ -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

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 3/6] libxl: portiblity fixes

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 323 bytes --]

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>



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

[-- Attachment #2: xen_tools_libxl3.diff --]
[-- Type: text/x-diff, Size: 1076 bytes --]

diff -r 1ca40043b632 -r 8ccaa5608511 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Tue Jul 27 13:53:12 2010 +0200
+++ b/tools/libxl/libxl_device.c	Tue Jul 27 13:55:04 2010 +0200
@@ -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)
diff -r 1ca40043b632 -r 8ccaa5608511 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Tue Jul 27 13:53:12 2010 +0200
+++ b/tools/libxl/libxl_internal.h	Tue Jul 27 13:55:04 2010 +0200
@@ -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,

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 4/6] libxl: portiblity fixes

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 323 bytes --]

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>



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

[-- Attachment #2: xen_tools_libxl4.diff --]
[-- Type: text/x-diff, Size: 570 bytes --]

diff -r 8ccaa5608511 -r 73082ed01e88 tools/Rules.mk
--- a/tools/Rules.mk	Tue Jul 27 13:55:04 2010 +0200
+++ b/tools/Rules.mk	Tue Jul 27 13:56:32 2010 +0200
@@ -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)
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 5/6] libxl: portiblity fixes

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 321 bytes --]

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

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

[-- Attachment #2: xen_tools_libxl5.diff --]
[-- Type: text/x-diff, Size: 3418 bytes --]

diff -r 73082ed01e88 -r 82c7ff060fa3 tools/libxl/Makefile
--- a/tools/libxl/Makefile	Tue Jul 27 13:56:32 2010 +0200
+++ b/tools/libxl/Makefile	Tue Jul 27 13:58:00 2010 +0200
@@ -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
diff -r 73082ed01e88 -r 82c7ff060fa3 tools/libxl/libxl_linux.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/libxl_linux.c	Tue Jul 27 13:58:00 2010 +0200
@@ -0,0 +1,50 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <Christoph.Egger@amd.com>
+ *
+ * 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 "tap-ctl.h"
+
+int libxl_blktap_enabled(struct libxl_ctx *ctx)
+{
+    const char *msg;
+    return !tap_ctl_check(&msg);
+}
+
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype)
+{
+    const char *type;
+    char *params, *devname;
+    int minor, err;
+
+    type = device_disk_string_of_phystype(phystype);
+    minor = tap_ctl_find_minor(type, disk);
+    if (minor >= 0) {
+        devname = libxl_sprintf(ctx, "/dev/xen/blktap-2/tapdev%d", minor);
+        if (devname)
+            return devname;
+    }
+
+    params = libxl_sprintf(ctx, "%s:%s", type, disk);
+    err = tap_ctl_create(params, &devname);
+    if (!err) {
+        libxl_ptr_add(ctx, devname);
+        return devname;
+    }
+
+    return NULL;
+}
diff -r 73082ed01e88 -r 82c7ff060fa3 tools/libxl/libxl_netbsd.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/libxl_netbsd.c	Tue Jul 27 13:58:00 2010 +0200
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <Christoph.Egger@amd.com>
+ *
+ * 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;
+}

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 6/6] libxl: portiblity fixes

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 322 bytes --]

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


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

[-- Attachment #2: xen_tools_libxl6.diff --]
[-- Type: text/x-diff, Size: 3701 bytes --]

diff -r 82c7ff060fa3 -r 999c524ce64b tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Tue Jul 27 13:58:00 2010 +0200
+++ b/tools/libxl/libxl.c	Tue Jul 27 13:59:19 2010 +0200
@@ -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;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

[Ian Jackson]

Christoph Egger writes ("[Xen-devel] [PATCH 5/6] libxl: portiblity fixes"):
>  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

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

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

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

[Christoph Egger]

On Tuesday 27 July 2010 19:00:07 Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH 5/6] libxl: portiblity fixes"):
> >  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
>
> 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.

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


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

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

[Ian Jackson]

Christoph Egger writes ("[Xen-devel] [PATCH 4/6] libxl: portiblity fixes"):
> +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.

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

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

On Tuesday 27 July 2010 18:58:26 Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH 4/6] libxl: portiblity fixes"):
> > +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.

Done.

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

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


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

[-- Attachment #2: xen_tools_libxl4.diff --]
[-- Type: text/x-diff, Size: 2328 bytes --]

diff -r 9ea1cf03ef55 tools/Rules.mk
--- a/tools/Rules.mk	Wed Jul 28 13:35:25 2010 +0200
+++ b/tools/Rules.mk	Wed Jul 28 13:48:07 2010 +0200
@@ -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)
 
diff -r 9ea1cf03ef55 tools/libxl/Makefile
--- a/tools/libxl/Makefile	Wed Jul 28 13:35:25 2010 +0200
+++ b/tools/libxl/Makefile	Wed Jul 28 13:48:07 2010 +0200
@@ -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)
diff -r 9ea1cf03ef55 tools/libxl/libxl_noblktap2.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/libxl_noblktap2.c	Wed Jul 28 13:48:07 2010 +0200
@@ -0,0 +1,29 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <Christoph.Egger@amd.com>
+ *
+ * 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;
+}

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH] libxl: compile fix

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 495 bytes --]


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 <Christoph.Egger@amd.com>


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

[-- Attachment #2: xen_tools_libxl3.diff --]
[-- Type: text/x-diff, Size: 510 bytes --]

diff -r 9ea1cf03ef55 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Wed Jul 28 13:35:25 2010 +0200
+++ b/tools/libxl/xl_cmdimpl.c	Wed Jul 28 13:41:00 2010 +0200
@@ -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:

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] libxl: compile fix

[Ian Campbell]

On Wed, 2010-07-28 at 12:42 +0100, Christoph Egger wrote:
> 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 <ian.campbell@citrix.com>

diff -r 479d042f25e4 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Wed Jul 28 12:07:44 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Wed Jul 28 13:21:16 2010 +0100
@@ -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]);


> 
> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> 
> 

Re: [PATCH] libxl: compile fix

[Christoph Egger]

On Wednesday 28 July 2010 14:22:37 Ian Campbell wrote:
> On Wed, 2010-07-28 at 12:42 +0100, Christoph Egger wrote:
> > 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 <ian.campbell@citrix.com>
>
> diff -r 479d042f25e4 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Wed Jul 28 12:07:44 2010 +0100
> +++ b/tools/libxl/xl_cmdimpl.c	Wed Jul 28 13:21:16 2010 +0100
> @@ -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]);
>

Acked-by: Christoph Egger <Christoph.Egger@amd.com>


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

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

[Ian Jackson]

Christoph Egger writes ("[Xen-devel] [PATCH 2/6] libxl: portiblity fixes"):
> --- a/tools/libxl/libxl_osdeps.h	Tue Jul 27 13:48:38 2010 +0200
> +++ b/tools/libxl/libxl_osdeps.h	Tue Jul 27 13:53:12 2010 +0200
> @@ -23,6 +23,8 @@
>  
>  #define _GNU_SOURCE
>  
> +#include <libxl_internal.h>
> +

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

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

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

[Christoph Egger]

On Tuesday 27 July 2010 18:34:21 Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH 2/6] libxl: portiblity fixes"):
> > --- a/tools/libxl/libxl_osdeps.h	Tue Jul 27 13:48:38 2010 +0200
> > +++ b/tools/libxl/libxl_osdeps.h	Tue Jul 27 13:53:12 2010 +0200
> > @@ -23,6 +23,8 @@
> >
> >  #define _GNU_SOURCE
> >
> > +#include <libxl_internal.h>
> > +
>
> This is wrong.  libxl_osdeps.h should not include libxl_internal.h.
>
> > +/* 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.

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

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

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

[Keir Fraser]

On 28/07/2010 10:06, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

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

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.

> 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

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

[Christoph Egger]

On Wednesday 28 July 2010 11:21:54 Keir Fraser wrote:
> On 28/07/2010 10:06, "Christoph Egger" <Christoph.Egger@amd.com> wrote:
> >> 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.
>
> 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.
>
> > 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

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

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

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 369 bytes --]


Move blktap specific code into libxl_blktap.c

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

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

[-- Attachment #2: xen_tools_libxl2.diff --]
[-- Type: text/x-diff, Size: 7666 bytes --]

diff -r 4f39505426cb tools/Rules.mk
--- a/tools/Rules.mk	Wed Jul 28 11:53:38 2010 +0200
+++ b/tools/Rules.mk	Wed Jul 28 13:25:50 2010 +0200
@@ -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
 
diff -r 4f39505426cb tools/libxl/Makefile
--- a/tools/libxl/Makefile	Wed Jul 28 11:53:38 2010 +0200
+++ b/tools/libxl/Makefile	Wed Jul 28 13:25:50 2010 +0200
@@ -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
diff -r 4f39505426cb tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Wed Jul 28 11:53:38 2010 +0200
+++ b/tools/libxl/libxl.c	Wed Jul 28 13:25:50 2010 +0200
@@ -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)
 
@@ -1430,27 +1429,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;
@@ -1499,14 +1477,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");
@@ -1527,7 +1504,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;
@@ -1591,7 +1568,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: {
@@ -1602,16 +1579,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;
diff -r 4f39505426cb tools/libxl/libxl_blktap2.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/libxl_blktap2.c	Wed Jul 28 13:25:50 2010 +0200
@@ -0,0 +1,51 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <Christoph.Egger@amd.com>
+ *
+ * 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"
+
+#include "tap-ctl.h"
+
+int libxl_blktap_enabled(struct libxl_ctx *ctx)
+{
+    const char *msg;
+    return !tap_ctl_check(&msg);
+}
+
+const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype)
+{
+    const char *type;
+    char *params, *devname;
+    int minor, err;
+
+    type = device_disk_string_of_phystype(phystype);
+    minor = tap_ctl_find_minor(type, disk);
+    if (minor >= 0) {
+        devname = libxl_sprintf(ctx, "/dev/xen/blktap-2/tapdev%d", minor);
+        if (devname)
+            return devname;
+    }
+
+    params = libxl_sprintf(ctx, "%s:%s", type, disk);
+    err = tap_ctl_create(params, &devname);
+    if (!err) {
+        libxl_ptr_add(ctx, devname);
+        return devname;
+    }
+
+    return NULL;
+}
diff -r 4f39505426cb tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Wed Jul 28 11:53:38 2010 +0200
+++ b/tools/libxl/libxl_internal.h	Wed Jul 28 13:25:50 2010 +0200
@@ -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
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

[Ian Jackson]

Christoph Egger writes ("[Xen-devel] [PATCH] libxl: move blktap specific code into libxl_blktap.c"):
> 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.

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

[Stefano Stabellini]

On Thu, 29 Jul 2010, Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH] libxl: move blktap specific code into libxl_blktap.c"):
> > 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
> 
 
I take this chance to say that I greatly prefer inline patches to
attachments.

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

[Christoph Egger]

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"):
> > 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().

>  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

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.


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

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

[Ian Jackson]

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: move blktap specific code into libxl_blktap.c"):
> 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.

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

[Ian Jackson]

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: move blktap specific code into libxl_blktap.c"):
> On Thursday 29 July 2010 17:02:25 Ian Jackson wrote:
> >  [PATCH 1/2] reorganise preparatory to moving
...
> >  [PATCH 2/2] move blktap-specific code to libxl_blktap.c
> >  Purely moving code about, no changes.
...
> 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 (-:.)

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

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

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 377 bytes --]


libxl: move blktap-specific code into libxl_blktap.c

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


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

[-- Attachment #2: xen_tools_libxl1.diff --]
[-- Type: text/x-diff, Size: 8170 bytes --]

diff -r bb230a0aa455 tools/Rules.mk
--- a/tools/Rules.mk	Thu Jul 29 18:28:03 2010 +0200
+++ b/tools/Rules.mk	Thu Jul 29 18:43:22 2010 +0200
@@ -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
 
diff -r bb230a0aa455 tools/libxl/Makefile
--- a/tools/libxl/Makefile	Thu Jul 29 18:28:03 2010 +0200
+++ b/tools/libxl/Makefile	Thu Jul 29 18:43:22 2010 +0200
@@ -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
diff -r bb230a0aa455 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Thu Jul 29 18:28:03 2010 +0200
+++ b/tools/libxl/libxl.c	Thu Jul 29 18:43:22 2010 +0200
@@ -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)
 
@@ -1430,27 +1429,6 @@ int libxl_confirm_device_model_startup(l
 
 /******************************************************************************/
 
-static char *get_blktap2_device(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(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(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
 {
     flexarray_t *front;
@@ -1499,14 +1477,13 @@ int libxl_device_disk_add(libxl_ctx *ctx
         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");
@@ -1527,7 +1504,7 @@ int libxl_device_disk_add(libxl_ctx *ctx
 
             device.backend_kind = DEVICE_TAP;
             break;
-        }
+
         default:
             XL_LOG(ctx, XL_LOG_ERROR, "unrecognized disk physical type: %d\n", disk->phystype);
             return ERROR_INVAL;
@@ -1591,7 +1568,7 @@ int libxl_device_disk_del(libxl_ctx *ctx
 
 const char * libxl_device_disk_local_attach(libxl_ctx *ctx, libxl_device_disk *disk)
 {
-    char *dev = NULL;
+    const char *dev = NULL;
     int phystype = disk->phystype;
     switch (phystype) {
         case PHYSTYPE_PHY: {
@@ -1602,16 +1579,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;
diff -r bb230a0aa455 tools/libxl/libxl_blktap2.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/libxl_blktap2.c	Thu Jul 29 18:43:22 2010 +0200
@@ -0,0 +1,60 @@
+/*
+ * Copyright (C) 2010      Advanced Micro Devices
+ * Author Christoph Egger <Christoph.Egger@amd.com>
+ *
+ * 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"
+
+#include "tap-ctl.h"
+
+int libxl_blktap_enabled(libxl_ctx *ctx)
+{
+    const char *msg;
+    return !tap_ctl_check(&msg);
+}
+
+static char *get_blktap2_device(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(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;
+}
+
+const char *libxl_blktap_devpath(libxl_ctx *ctx,
+                                 const char *disk,
+                                 libxl_disk_phystype phystype)
+{
+    const char *type;
+    char *dev;
+
+    type = device_disk_string_of_phystype(phystype);
+    dev = get_blktap2_device(ctx, disk, type);
+    if (!dev)
+        dev = make_blktap2_device(ctx, disk, type);
+    return dev;
+}
diff -r bb230a0aa455 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Thu Jul 29 18:28:03 2010 +0200
+++ b/tools/libxl/libxl_internal.h	Thu Jul 29 18:43:22 2010 +0200
@@ -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
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

[Ian Jackson]

Christoph Egger writes ("[PATCH] libxl: move blktap-specific code into libxl_blktap.c"):
> 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.

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

[Christoph Egger]

On Thursday 29 July 2010 20:02:09 Ian Jackson wrote:
> Christoph Egger writes ("[PATCH] libxl: move blktap-specific code into 
libxl_blktap.c"):
> > 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


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

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

[Christoph Egger]

On Friday 30 July 2010 10:40:27 Christoph Egger wrote:
> On Thursday 29 July 2010 20:02:09 Ian Jackson wrote:
> > Christoph Egger writes ("[PATCH] libxl: move blktap-specific code into
>
> libxl_blktap.c"):
> > > 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.

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

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



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

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

[Ian Jackson]

Christoph Egger writes ("[Xen-devel] [PATCH 1/6] libxl: portiblity fixes"):
>  #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 in libxl_osdeps.h as previously discussed.

Ian.

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

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 983 bytes --]

On Tuesday 27 July 2010 18:31:24 Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH 1/6] libxl: portiblity fixes"):
> >  #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 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 <Christoph.Egger@amd.com>

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

[-- Attachment #2: xen_tools_libxl1.diff --]
[-- Type: text/x-diff, Size: 1601 bytes --]

diff -r c04dad02ffa3 tools/libxl/Makefile
--- a/tools/libxl/Makefile	Wed Jul 28 11:37:51 2010 +0200
+++ b/tools/libxl/Makefile	Wed Jul 28 11:46:52 2010 +0200
@@ -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)
diff -r c04dad02ffa3 tools/libxl/libxl_bootloader.c
--- a/tools/libxl/libxl_bootloader.c	Wed Jul 28 11:37:51 2010 +0200
+++ b/tools/libxl/libxl_bootloader.c	Wed Jul 28 11:46:52 2010 +0200
@@ -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>
diff -r c04dad02ffa3 tools/libxl/libxl_osdeps.h
--- a/tools/libxl/libxl_osdeps.h	Wed Jul 28 11:37:51 2010 +0200
+++ b/tools/libxl/libxl_osdeps.h	Wed Jul 28 11:46:52 2010 +0200
@@ -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>
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 0/6] libxl: portiblity fixes

[Christoph Egger]

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.


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

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

[Ian Jackson]

Christoph Egger writes ("[Xen-devel] [PATCH 0/6] libxl: portiblity fixes"):
> 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.