* [RFC] xl disk configuration handling
@ 2011-01-30 17:46 Kamala Narasimhan
2011-01-31 17:18 ` Stefano Stabellini
0 siblings, 1 reply; 7+ messages in thread
From: Kamala Narasimhan @ 2011-01-30 17:46 UTC (permalink / raw)
To: xen-devel
[-- Attachment #1: Type: text/plain, Size: 2531 bytes --]
Based on the discussion we have had so far on xl disk configuration handling, I have made a fair amount of implementation changes. As most of you might have suspected the changes are vast and has ripple effect all over the place due to interface changes. But I think I got the go ahead from the maintainers to make these changes for 4.1 and I also believe we are better off making these changes for 4.1. The plan is to send the patches in following order -
1) Patch 1/3 - Interface changes, parsing changes, frontend/endback specific disk parameters setup and other ripple effects caused by interface changes. Ideally, I would like to keep this patch to just the interface and parsing changes but by doing so I might break the rest of the code from even compiling. So, this change is going to be huge.
2) Patch 2/3 - Disk validation changes
3) Patch 3/3 - "xl Disk Configuration option" documentation
Let me know if you would prefer a different order for convenience or for some other reason.
I have attached the changes I have made so far for you to get an idea of the level of changes coming. Note that this is NOT a patch submission.
Here are the parts of the patch I would like to get reviewed -
1) Interface changes - libxl.idl, libxl.h
2) Parsing changes - xl_cmdimpl.c. I have basically divided each disk configuration information into three chunks - pdev, vdev and attribute and parsed it based on our discussion/documentation. In specific, parse_disk_config, parse_disk_attrib, parse_disk_vdev_info and parse_disk_pdev_info implementation is the core of parsing code and that is what I would like to get reviewed to be sure I am not way off from what you would be willing to accept.
What not to review in this patch -
Anything other than interface and parsing changes, I would request that you ignore for now. I am sending it only for the following reasons -
1) Since we are very close to 4.1, I would like you to get an idea of all the places this change touches.
2) I would want the patch to build!
Other than that, there are parts of the patch outside interface/parsing changes that might be incomplete or even incorrect! I will be sending out follow up emails to better understand certain parts of disk hotplug code and disk frontend/backend parameters setup code and will make changes based on the input I get and the patch as a whole should be ready for review after that. If you would rather I resend just the interface/parsing changes for review in that case, let me know. Thanks.
Kamala
[-- Attachment #2: xl-disk-config-handling.diff --]
[-- Type: text/x-diff, Size: 37106 bytes --]
diff -r 52e928af3637 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Fri Jan 28 19:37:49 2011 +0000
+++ b/tools/libxl/libxl.c Sun Jan 30 11:47:22 2011 -0500
@@ -588,7 +588,7 @@ int libxl_wait_for_disk_ejects(libxl_ctx
for (i = 0; i < num_disks; i++) {
if (asprintf(&(waiter[i].path), "%s/device/vbd/%d/eject",
libxl__xs_get_dompath(&gc, domid),
- libxl__device_disk_dev_number(disks[i].virtpath)) < 0)
+ libxl__device_disk_dev_number(disks[i].vdev_path)) < 0)
goto out;
if (asprintf(&(waiter[i].token), "%d", LIBXL_EVENT_DISK_EJECT) < 0)
goto out;
@@ -668,10 +668,9 @@ int libxl_event_get_disk_eject_info(libx
disk->backend_domid = 0;
disk->domid = domid;
- disk->physpath = strdup("");
- disk->phystype = PHYSTYPE_EMPTY;
+ disk->pdev_path = strdup("");
/* this value is returned to the user: do not free right away */
- disk->virtpath = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", backend));
+ disk->vdev_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", backend));
disk->unpluggable = 1;
disk->readwrite = 0;
disk->is_cdrom = 1;
@@ -855,18 +854,19 @@ skip_autopass:
/******************************************************************************/
-static int validate_virtual_disk(libxl_ctx *ctx, char *file_name, libxl_disk_phystype disk_type)
+static int validate_virtual_disk(libxl_ctx *ctx, char *file_name,
+ libxl_disk_pdev_type pdev_type, int is_cdrom)
{
struct stat stat_buf;
- if ( (file_name[0] == '\0') && (disk_type == PHYSTYPE_EMPTY) )
+ if ( (file_name[0] == '\0') && (is_cdrom == 1) )
return 0;
if ( stat(file_name, &stat_buf) != 0 ) {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to stat %s", file_name);
return ERROR_INVAL;
}
- if ( disk_type == PHYSTYPE_PHY ) {
+ if ( pdev_type == DISK_PDEV_TYPE_PHY ) {
if ( !(S_ISBLK(stat_buf.st_mode)) ) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s is not a block device!\n",
file_name);
@@ -890,7 +890,7 @@ int libxl_device_disk_add(libxl_ctx *ctx
libxl__device device;
int major, minor, rc;
- rc = validate_virtual_disk(ctx, disk->physpath, disk->phystype);
+ rc = validate_virtual_disk(ctx, disk->pdev_path, disk->pdev_type, disk->is_cdrom);
if (rc)
return rc;
@@ -905,11 +905,11 @@ int libxl_device_disk_add(libxl_ctx *ctx
goto out_free;
}
- backend_type = libxl__device_disk_backend_type_of_phystype(disk->phystype);
- devid = libxl__device_disk_dev_number(disk->virtpath);
+ backend_type = libxl__device_disk_backend_type_of_phystype(disk->pdev_type);
+ devid = libxl__device_disk_dev_number(disk->vdev_path);
if (devid==-1) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Invalid or unsupported"
- " virtual disk identifier %s", disk->virtpath);
+ " virtual disk identifier %s", disk->vdev_path);
rc = ERROR_INVAL;
goto out_free;
}
@@ -920,37 +920,31 @@ int libxl_device_disk_add(libxl_ctx *ctx
device.domid = disk->domid;
device.kind = DEVICE_VBD;
- switch (disk->phystype) {
- case PHYSTYPE_PHY: {
+ switch (disk->pdev_type) {
+ case DISK_PDEV_TYPE_PHY: {
- libxl__device_physdisk_major_minor(disk->physpath, &major, &minor);
+ libxl__device_physdisk_major_minor(disk->pdev_path, &major, &minor);
flexarray_append(back, "physical-device");
flexarray_append(back, libxl__sprintf(&gc, "%x:%x", major, minor));
flexarray_append(back, "params");
- flexarray_append(back, disk->physpath);
+ flexarray_append(back, disk->pdev_path);
device.backend_kind = DEVICE_VBD;
break;
}
- case PHYSTYPE_EMPTY:
- break;
- 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:
+ case DISK_PDEV_TYPE_FILE:
+ case DISK_PDEV_TYPE_TAP:
+ case DISK_PDEV_TYPE_TAP2:
if (libxl__blktap_enabled(&gc)) {
const char *dev = libxl__blktap_devpath(&gc,
- disk->physpath, disk->phystype);
+ disk->pdev_path, disk->pdev_type);
if (!dev) {
rc = ERROR_FAIL;
goto out_free;
}
flexarray_append(back, "tapdisk-params");
- flexarray_append(back, libxl__sprintf(&gc, "%s:%s", libxl__device_disk_string_of_phystype(disk->phystype), disk->physpath));
+ flexarray_append(back, libxl__sprintf(&gc, "%s:%s", libxl__device_disk_string_of_phystype(disk->pdev_type), disk->pdev_path));
flexarray_append(back, "params");
flexarray_append(back, libxl__strdup(&gc, dev));
backend_type = "phy";
@@ -963,7 +957,7 @@ int libxl_device_disk_add(libxl_ctx *ctx
}
flexarray_append(back, "params");
flexarray_append(back, libxl__sprintf(&gc, "%s:%s",
- libxl__device_disk_string_of_phystype(disk->phystype), disk->physpath));
+ libxl__device_disk_string_of_phystype(disk->pdev_type), disk->pdev_path));
if (libxl__blktap_enabled(&gc))
device.backend_kind = DEVICE_TAP;
@@ -972,7 +966,7 @@ int libxl_device_disk_add(libxl_ctx *ctx
break;
default:
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk physical type: %d\n", disk->phystype);
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk physical type: %d\n", disk->pdev_type);
rc = ERROR_INVAL;
goto out_free;
}
@@ -988,7 +982,7 @@ int libxl_device_disk_add(libxl_ctx *ctx
flexarray_append(back, "state");
flexarray_append(back, libxl__sprintf(&gc, "%d", 1));
flexarray_append(back, "dev");
- flexarray_append(back, disk->virtpath);
+ flexarray_append(back, disk->vdev_path);
flexarray_append(back, "type");
flexarray_append(back, backend_type);
flexarray_append(back, "mode");
@@ -1028,11 +1022,11 @@ int libxl_device_disk_del(libxl_ctx *ctx
libxl__device device;
int devid;
- devid = libxl__device_disk_dev_number(disk->virtpath);
+ devid = libxl__device_disk_dev_number(disk->vdev_path);
device.backend_domid = disk->backend_domid;
device.backend_devid = devid;
device.backend_kind =
- (disk->phystype == PHYSTYPE_PHY) ? DEVICE_VBD : DEVICE_TAP;
+ (disk->pdev_type == DISK_PDEV_TYPE_PHY) ? DEVICE_VBD : DEVICE_TAP;
device.domid = disk->domid;
device.devid = devid;
device.kind = DEVICE_VBD;
@@ -1044,32 +1038,33 @@ char * libxl_device_disk_local_attach(li
libxl__gc gc = LIBXL_INIT_GC(ctx);
const char *dev = NULL;
char *ret = NULL;
- int phystype = disk->phystype;
+ int phystype = disk->pdev_type;
switch (phystype) {
- case PHYSTYPE_PHY: {
- fprintf(stderr, "attaching PHY disk %s to domain 0\n", disk->physpath);
- dev = disk->physpath;
+ case DISK_PDEV_TYPE_PHY: {
+ fprintf(stderr, "attaching PHY disk %s to domain 0\n", disk->pdev_path);
+ dev = disk->pdev_path;
break;
}
- case PHYSTYPE_FILE:
- /* let's pretend is tap:aio for the moment */
- phystype = PHYSTYPE_AIO;
- case PHYSTYPE_AIO:
- if (!libxl__blktap_enabled(&gc)) {
- dev = disk->physpath;
+ case DISK_PDEV_TYPE_FILE:
+ case DISK_PDEV_TYPE_TAP:
+ case DISK_PDEV_TYPE_TAP2:
+ if ( disk->impl_type == DISK_IMPL_TYPE_AIO ) {
+ if (!libxl__blktap_enabled(&gc)) {
+ dev = disk->pdev_path;
+ break;
+ }
+ }
+ if ( disk->format == DISK_FORMAT_VHD ) {
+ if (libxl__blktap_enabled(&gc))
+ dev = libxl__blktap_devpath(&gc, disk->pdev_path, phystype);
+ else
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "tapdisk2 is required to open a vhd disk\n");
+ break;
+ } else if ( disk->format == DISK_FORMAT_QCOW ||
+ disk->format == DISK_FORMAT_QCOW2 ) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally attach a qcow or qcow2 disk image\n");
break;
}
- case PHYSTYPE_VHD:
- if (libxl__blktap_enabled(&gc))
- dev = libxl__blktap_devpath(&gc, disk->physpath, phystype);
- else
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "tapdisk2 is required to open a vhd disk\n");
- break;
- case PHYSTYPE_QCOW:
- case PHYSTYPE_QCOW2:
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally attach a qcow or qcow2 disk image\n");
- break;
-
default:
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk physical type: %d\n", phystype);
break;
@@ -1082,7 +1077,7 @@ char * libxl_device_disk_local_attach(li
int libxl_device_disk_local_detach(libxl_ctx *ctx, libxl_device_disk *disk)
{
- /* Nothing to do for PHYSTYPE_PHY. */
+ /* Nothing to do for DISK_PDEV_TYPE_PHY. */
/*
* For other device types assume that the blktap2 process is
@@ -1669,13 +1664,13 @@ static unsigned int libxl_append_disk_li
pdisk->domid = domid;
physpath_tmp = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "%s/%s/params", be_path, *dir), &len);
if (physpath_tmp && strchr(physpath_tmp, ':')) {
- pdisk->physpath = strdup(strchr(physpath_tmp, ':') + 1);
+ pdisk->pdev_path = strdup(strchr(physpath_tmp, ':') + 1);
free(physpath_tmp);
} else {
- pdisk->physpath = physpath_tmp;
+ pdisk->pdev_path = physpath_tmp;
}
- libxl_string_to_phystype(ctx, libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/%s/type", be_path, *dir)), &(pdisk->phystype));
- pdisk->virtpath = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "%s/%s/dev", be_path, *dir), &len);
+ libxl_string_to_phystype(ctx, libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/%s/type", be_path, *dir)), &(pdisk->pdev_type));
+ pdisk->vdev_path = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "%s/%s/dev", be_path, *dir), &len);
pdisk->unpluggable = atoi(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/%s/removable", be_path, *dir)));
if (!strcmp(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/%s/mode", be_path, *dir)), "w"))
pdisk->readwrite = 1;
@@ -1710,7 +1705,7 @@ int libxl_device_disk_getinfo(libxl_ctx
char *val;
dompath = libxl__xs_get_dompath(&gc, domid);
- diskinfo->devid = libxl__device_disk_dev_number(disk->virtpath);
+ diskinfo->devid = libxl__device_disk_dev_number(disk->vdev_path);
/* tap devices entries in xenstore are written as vbd devices. */
diskpath = libxl__sprintf(&gc, "%s/device/vbd/%d", dompath, diskinfo->devid);
@@ -1744,13 +1739,11 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
libxl_device_disk *disks;
int ret = ERROR_FAIL;
- if (!disk->physpath) {
- disk->physpath = strdup("");
- disk->phystype = PHYSTYPE_EMPTY;
- }
+ if (!disk->pdev_path)
+ disk->pdev_path = strdup("");
disks = libxl_device_disk_list(ctx, domid, &num);
for (i = 0; i < num; i++) {
- if (disks[i].is_cdrom && !strcmp(disk->virtpath, disks[i].virtpath))
+ if (disks[i].is_cdrom && !strcmp(disk->vdev_path, disks[i].vdev_path))
/* found */
break;
}
diff -r 52e928af3637 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h Fri Jan 28 19:37:49 2011 +0000
+++ b/tools/libxl/libxl.h Sun Jan 30 11:47:22 2011 -0500
@@ -172,14 +172,27 @@ typedef enum {
} libxl_console_consback;
typedef enum {
- PHYSTYPE_QCOW = 1,
- PHYSTYPE_QCOW2,
- PHYSTYPE_VHD,
- PHYSTYPE_AIO,
- PHYSTYPE_FILE,
- PHYSTYPE_PHY,
- PHYSTYPE_EMPTY,
-} libxl_disk_phystype;
+ DISK_FORMAT_UNKNOWN = 0,
+ DISK_FORMAT_QCOW,
+ DISK_FORMAT_QCOW2,
+ DISK_FORMAT_VHD,
+ DISK_FORMAT_RAW,
+} libxl_disk_format;
+
+typedef enum {
+ DISK_IMPL_TYPE_UNKNOWN = 0,
+ DISK_IMPL_TYPE_AIO,
+ DISK_IMPL_TYPE_TAPDISK,
+ DISK_IMPL_TYPE_IOEMU,
+} libxl_disk_impl_type;
+
+typedef enum {
+ DISK_PDEV_TYPE_UNKNOWN = 0,
+ DISK_PDEV_TYPE_PHY,
+ DISK_PDEV_TYPE_FILE,
+ DISK_PDEV_TYPE_TAP,
+ DISK_PDEV_TYPE_TAP2,
+} libxl_disk_pdev_type;
typedef enum {
NICTYPE_IOEMU = 1,
diff -r 52e928af3637 tools/libxl/libxl.idl
--- a/tools/libxl/libxl.idl Fri Jan 28 19:37:49 2011 +0000
+++ b/tools/libxl/libxl.idl Sun Jan 30 11:47:22 2011 -0500
@@ -11,7 +11,9 @@ libxl_qemu_machine_type = Number("qemu_m
libxl_qemu_machine_type = Number("qemu_machine_type", namespace="libxl_")
libxl_console_consback = Number("console_consback", namespace="libxl_")
libxl_console_constype = Number("console_constype", namespace="libxl_")
-libxl_disk_phystype = Number("disk_phystype", namespace="libxl_")
+libxl_disk_format = Number("disk_format", namespace="libxl_")
+libxl_disk_impl_type = Number("disk_impl_type", namespace="libxl_")
+libxl_disk_pdev_type = Number("disk_pdev_type", namespace="libxl_")
libxl_nic_type = Number("nic_type", namespace="libxl_")
libxl_cpuid_policy_list = Builtin("cpuid_policy_list", destructor_fn="libxl_cpuid_destroy", passby=PASS_BY_REFERENCE)
@@ -203,9 +205,11 @@ libxl_device_disk = Struct("device_disk"
libxl_device_disk = Struct("device_disk", [
("backend_domid", uint32),
("domid", domid),
- ("physpath", string),
- ("phystype", libxl_disk_phystype),
- ("virtpath", string),
+ ("pdev_path", string),
+ ("vdev_path", string),
+ ("pdev_type", libxl_disk_pdev_type),
+ ("impl_type", libxl_disk_impl_type),
+ ("format", libxl_disk_format),
("unpluggable", integer),
("readwrite", integer),
("is_cdrom", integer),
diff -r 52e928af3637 tools/libxl/libxl_blktap2.c
--- a/tools/libxl/libxl_blktap2.c Fri Jan 28 19:37:49 2011 +0000
+++ b/tools/libxl/libxl_blktap2.c Sun Jan 30 11:47:22 2011 -0500
@@ -26,13 +26,13 @@ int libxl__blktap_enabled(libxl__gc *gc)
const char *libxl__blktap_devpath(libxl__gc *gc,
const char *disk,
- libxl_disk_phystype phystype)
+ libxl_disk_pdev_type pdev_type)
{
const char *type;
char *params, *devname = NULL;
int minor, err;
- type = libxl__device_disk_string_of_phystype(phystype);
+ type = libxl__device_disk_string_of_phystype(pdev_type);
minor = tap_ctl_find_minor(type, disk);
if (minor >= 0) {
devname = libxl__sprintf(gc, "/dev/xen/blktap-2/tapdev%d", minor);
diff -r 52e928af3637 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c Fri Jan 28 19:37:49 2011 +0000
+++ b/tools/libxl/libxl_device.c Sun Jan 30 11:47:22 2011 -0500
@@ -121,31 +121,23 @@ out:
return rc;
}
-char *libxl__device_disk_string_of_phystype(libxl_disk_phystype phystype)
+char *libxl__device_disk_string_of_phystype(libxl_disk_pdev_type pdev_type)
{
- switch (phystype) {
- case PHYSTYPE_QCOW: return "qcow";
- case PHYSTYPE_QCOW2: return "qcow2";
- case PHYSTYPE_VHD: return "vhd";
- case PHYSTYPE_AIO: return "aio";
- case PHYSTYPE_FILE: return "file";
- case PHYSTYPE_PHY: return "phy";
- case PHYSTYPE_EMPTY: return "file";
+ switch (pdev_type) {
+ case DISK_PDEV_TYPE_FILE: return "file";
+ case DISK_PDEV_TYPE_PHY: return "phy";
+ case DISK_PDEV_TYPE_TAP: return "tap";
+ case DISK_PDEV_TYPE_TAP2: return "tap2";
default: return NULL;
}
}
-char *libxl__device_disk_backend_type_of_phystype(libxl_disk_phystype phystype)
+char *libxl__device_disk_backend_type_of_phystype(libxl_disk_pdev_type pdev_type)
{
- switch (phystype) {
- case PHYSTYPE_QCOW: return "tap";
- case PHYSTYPE_QCOW2: return "tap";
- case PHYSTYPE_VHD: return "tap";
- case PHYSTYPE_AIO: return "tap";
+ switch (pdev_type) {
/* let's pretend file is tap:aio */
- case PHYSTYPE_FILE: return "tap";
- case PHYSTYPE_EMPTY: return "tap";
- case PHYSTYPE_PHY: return "phy";
+ case DISK_PDEV_TYPE_FILE: return "tap";
+ case DISK_PDEV_TYPE_PHY: return "phy";
default: return NULL;
}
}
diff -r 52e928af3637 tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c Fri Jan 28 19:37:49 2011 +0000
+++ b/tools/libxl/libxl_dm.c Sun Jan 30 11:47:22 2011 -0500
@@ -303,10 +303,10 @@ static char ** libxl_build_device_model_
for (i; i < nb; i++) {
if (disks[i].is_cdrom) {
flexarray_append(dm_args, "-cdrom");
- flexarray_append(dm_args, libxl__strdup(gc, disks[i].physpath));
+ flexarray_append(dm_args, libxl__strdup(gc, disks[i].pdev_path));
} else {
- flexarray_append(dm_args, libxl__sprintf(gc, "-%s", disks[i].virtpath));
- flexarray_append(dm_args, libxl__strdup(gc, disks[i].physpath));
+ flexarray_append(dm_args, libxl__sprintf(gc, "-%s", disks[i].vdev_path));
+ flexarray_append(dm_args, libxl__strdup(gc, disks[i].pdev_path));
}
libxl_device_disk_destroy(&disks[i]);
}
diff -r 52e928af3637 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h Fri Jan 28 19:37:49 2011 +0000
+++ b/tools/libxl/libxl_internal.h Sun Jan 30 11:47:22 2011 -0500
@@ -178,8 +178,8 @@ _hidden void libxl__userdata_destroyall(
_hidden void libxl__userdata_destroyall(libxl_ctx *ctx, uint32_t domid);
/* from xl_device */
-_hidden char *libxl__device_disk_backend_type_of_phystype(libxl_disk_phystype phystype);
-_hidden char *libxl__device_disk_string_of_phystype(libxl_disk_phystype phystype);
+_hidden char *libxl__device_disk_backend_type_of_phystype(libxl_disk_pdev_type pdev_type);
+_hidden char *libxl__device_disk_string_of_phystype(libxl_disk_pdev_type pdev_type);
_hidden int libxl__device_physdisk_major_minor(const char *physpath, int *major, int *minor);
_hidden int libxl__device_disk_dev_number(char *virtpath);
@@ -306,7 +306,7 @@ _hidden int libxl__blktap_enabled(libxl_
*/
_hidden const char *libxl__blktap_devpath(libxl__gc *gc,
const char *disk,
- libxl_disk_phystype phystype);
+ libxl_disk_pdev_type pdev_type);
_hidden char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid);
diff -r 52e928af3637 tools/libxl/libxl_utils.c
--- a/tools/libxl/libxl_utils.c Fri Jan 28 19:37:49 2011 +0000
+++ b/tools/libxl/libxl_utils.c Sun Jan 30 11:47:22 2011 -0500
@@ -275,34 +275,20 @@ out:
return rc;
}
-int libxl_string_to_phystype(libxl_ctx *ctx, char *s, libxl_disk_phystype *phystype)
+int libxl_string_to_phystype(libxl_ctx *ctx, char *s, libxl_disk_pdev_type *pdev_type)
{
- char *p;
- int rc = 0;
+ *pdev_type = DISK_PDEV_TYPE_UNKNOWN;
- if (!strcmp(s, "phy")) {
- *phystype = PHYSTYPE_PHY;
- } else if (!strcmp(s, "file")) {
- *phystype = PHYSTYPE_FILE;
- } else if (!strcmp(s, "tap")) {
- p = strchr(s, ':');
- if (!p) {
- rc = ERROR_INVAL;
- goto out;
- }
- p++;
- if (!strcmp(p, "aio")) {
- *phystype = PHYSTYPE_AIO;
- } else if (!strcmp(p, "vhd")) {
- *phystype = PHYSTYPE_VHD;
- } else if (!strcmp(p, "qcow")) {
- *phystype = PHYSTYPE_QCOW;
- } else if (!strcmp(p, "qcow2")) {
- *phystype = PHYSTYPE_QCOW2;
- }
- }
-out:
- return rc;
+ if ( !strncmp(s, "phy", sizeof("phy")-1) )
+ *pdev_type = DISK_PDEV_TYPE_PHY;
+ else if ( !strncmp(s, "file", sizeof("file")-1) )
+ *pdev_type = DISK_PDEV_TYPE_FILE;
+ else if ( !strncmp(s, "tap", sizeof("tap")-1) )
+ *pdev_type = DISK_PDEV_TYPE_TAP;
+ else if ( !strncmp(s, "tap2", sizeof("tap2")-1) )
+ *pdev_type = DISK_PDEV_TYPE_TAP2;
+
+ return 0;
}
int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
@@ -553,10 +539,10 @@ int libxl_devid_to_device_disk(libxl_ctx
disk->backend_domid = strtoul(val, NULL, 10);
disk->domid = domid;
be_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/backend", diskpath));
- disk->physpath = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path));
+ disk->pdev_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path));
val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/type", be_path));
- libxl_string_to_phystype(ctx, val, &(disk->phystype));
- disk->virtpath = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path));
+ libxl_string_to_phystype(ctx, val, &(disk->pdev_type));
+ disk->vdev_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path));
val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/removable", be_path));
disk->unpluggable = !strcmp(val, "1");
val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/mode", be_path));
diff -r 52e928af3637 tools/libxl/libxl_utils.h
--- a/tools/libxl/libxl_utils.h Fri Jan 28 19:37:49 2011 +0000
+++ b/tools/libxl/libxl_utils.h Sun Jan 30 11:47:22 2011 -0500
@@ -29,7 +29,7 @@ int libxl_get_stubdom_id(libxl_ctx *ctx,
int libxl_get_stubdom_id(libxl_ctx *ctx, int guest_domid);
int libxl_is_stubdom(libxl_ctx *ctx, uint32_t domid, uint32_t *target_domid);
int libxl_create_logfile(libxl_ctx *ctx, char *name, char **full_name);
-int libxl_string_to_phystype(libxl_ctx *ctx, char *s, libxl_disk_phystype *phystype);
+int libxl_string_to_phystype(libxl_ctx *ctx, char *s, libxl_disk_pdev_type *pdev_type);
int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
void **data_r, int *datalen_r);
diff -r 52e928af3637 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Fri Jan 28 19:37:49 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c Sun Jan 30 11:47:22 2011 -0500
@@ -361,9 +361,9 @@ static void printf_info(int domid,
printf("\t\t(tap\n");
printf("\t\t\t(backend_domid %d)\n", d_config->disks[i].backend_domid);
printf("\t\t\t(domid %d)\n", d_config->disks[i].domid);
- printf("\t\t\t(physpath %s)\n", d_config->disks[i].physpath);
- printf("\t\t\t(phystype %d)\n", d_config->disks[i].phystype);
- printf("\t\t\t(virtpath %s)\n", d_config->disks[i].virtpath);
+ printf("\t\t\t(physpath %s)\n", d_config->disks[i].pdev_path);
+ printf("\t\t\t(phystype %d)\n", d_config->disks[i].pdev_type);
+ printf("\t\t\t(virtpath %s)\n", d_config->disks[i].vdev_path);
printf("\t\t\t(unpluggable %d)\n", d_config->disks[i].unpluggable);
printf("\t\t\t(readwrite %d)\n", d_config->disks[i].readwrite);
printf("\t\t\t(is_cdrom %d)\n", d_config->disks[i].is_cdrom);
@@ -438,137 +438,164 @@ static int parse_action_on_shutdown(cons
return 0;
}
-#define DSTATE_INITIAL 0
-#define DSTATE_TAP 1
-#define DSTATE_PHYSPATH 2
-#define DSTATE_VIRTPATH 3
-#define DSTATE_VIRTTYPE 4
-#define DSTATE_RW 5
-#define DSTATE_TERMINAL 6
-
+static int parse_disk_attrib(libxl_device_disk *disk, char *buf2)
+{
+ char *attrib = strrchr(buf2, ',');
+ if ( !attrib ) {
+ LOG("Invalid disk configuration option %s. Refer to the xl disk "
+ "configuration document for further information", buf2);
+ return ERROR_INVAL;
+ }
+
+ *attrib = '\0';
+ attrib++;
+ if ( attrib[0] == 'w' )
+ disk->readwrite = 1;
+ else if ( attrib[0] != 'r' ) {
+ LOG("Required access rights attribute is missing or incorrect in "
+ "disk configuration option %s", buf2);
+ return ERROR_INVAL;
+ }
+
+ return 0;
+}
+
+static int parse_disk_vdev_info(libxl_device_disk *disk, char *buf2)
+{
+ char *vdev_info, *vdev, *type;
+
+ vdev_info = strrchr(buf2, ',');
+ if ( !vdev_info ) {
+ LOG("Required vdev info missing in disk configuration option");
+ return ERROR_INVAL;
+ }
+
+ *vdev_info = '\0';
+ vdev_info++;
+
+ type = strchr(vdev_info, ':');
+ if ( type ) {
+ *type = '\0';
+ type++;
+ if ( !strncmp(type, "cdrom", sizeof("cdrom")-1) ) {
+ disk->is_cdrom = 1;
+ disk->unpluggable = 1;
+ }
+ else {
+ LOG("Invalid vdev type %s specified in disk configuration option",
+ type);
+ return ERROR_INVAL;
+ }
+ }
+
+ vdev = vdev_info;
+ if ( vdev[0] == '\0' ) {
+ LOG("Mandatory attribute vdev not specified");
+ return ERROR_INVAL;
+ }
+
+ disk->vdev_path = strdup(vdev);
+ return 0;
+}
+
+static int parse_disk_pdev_info(libxl_device_disk *disk, char *buf2)
+{
+ char *pdev_info, *pdev_type, *pdev_impl, *pdev_format, *pdev_path;
+
+ pdev_info = buf2;
+ if ( pdev_info[0] == '\0' && !disk->is_cdrom ) {
+ /* pdev can be empty string only for cdrom drive with no media inserted */
+ LOG("Required vdev info missing in disk configuration option");
+ return ERROR_INVAL;
+ }
+
+ pdev_path = strrchr(pdev_info, ':');
+ if ( !pdev_path ) {
+ disk->pdev_path = strdup(pdev_info);
+ return 0;
+ }
+
+ *pdev_path = '\0';
+ disk->pdev_path = strdup(++pdev_path);
+
+ pdev_format = strrchr(pdev_info, ':');
+ if ( !pdev_format )
+ pdev_format = pdev_info;
+ else {
+ pdev_format = '\0';
+ pdev_format++;
+ }
+
+ if ( !strncmp(pdev_format, "raw", sizeof("raw")-1) )
+ disk->format = DISK_FORMAT_RAW;
+ else if ( !strncmp(pdev_format, "qcow", sizeof("qcow")-1) )
+ disk->format = DISK_FORMAT_QCOW;
+ else if ( !strncmp(pdev_format, "qcow2", sizeof("qcow2")-1) )
+ disk->format = DISK_FORMAT_QCOW2;
+ else if ( !strncmp(pdev_format, "vhd", sizeof("vhd")-1) )
+ disk->format = DISK_FORMAT_VHD;
+
+ if ( disk->format == DISK_FORMAT_UNKNOWN )
+ pdev_impl = pdev_format;
+ else {
+ pdev_impl = strrchr(pdev_info, ':');
+ if ( !pdev_impl )
+ return 0;
+ *pdev_impl = '\0';
+ pdev_impl++;
+ }
+
+ if ( !strncmp(pdev_impl, "aio", sizeof("aio")-1) )
+ disk->impl_type = DISK_IMPL_TYPE_AIO;
+ else if ( !strncmp(pdev_impl, "tapdisk", sizeof("tapdisk")-1) )
+ disk->impl_type = DISK_IMPL_TYPE_TAPDISK;
+ else if ( !strncmp(pdev_impl, "ioemu", sizeof("ioemu")-1) )
+ disk->impl_type = DISK_IMPL_TYPE_IOEMU;
+
+ if ( disk->impl_type == DISK_IMPL_TYPE_UNKNOWN )
+ pdev_type = pdev_impl;
+ else {
+ pdev_type = pdev_info;
+ if ( pdev_type[0] == '\0' )
+ return 0;
+ }
+
+ if ( !strncmp(pdev_type, "phy", sizeof("phy")-1) )
+ disk->pdev_type = DISK_PDEV_TYPE_PHY;
+ else if ( !strncmp(pdev_type, "file", sizeof("file")-1) )
+ disk->pdev_type = DISK_PDEV_TYPE_FILE;
+ else if ( !strncmp(pdev_type, "tap", sizeof("tap")-1) )
+ disk->pdev_type = DISK_PDEV_TYPE_TAP;
+ else if ( !strncmp(pdev_type, "tap2", sizeof("tap2")-1) )
+ disk->pdev_type = DISK_PDEV_TYPE_TAP2;
+
+ return 0;
+}
+
+/*
+ * Note: Following code calls methods each of which will parse
+ * specific chunks of the disk configuration option, the specific
+ * chunks being attribute, vdev and pdev. As with any parsing code
+ * it makes assumption about the order in which specific chunks appear
+ * in the disk configuration option string. So, please use
+ * care while modifying the code below, esp. when it comes to
+ * the order of calls.
+ */
static int parse_disk_config(libxl_device_disk *disk, char *buf2)
{
- int state = DSTATE_INITIAL;
- char *p, *end, *tok;
+ int ret_val;
memset(disk, 0, sizeof(*disk));
- for(tok = p = buf2, end = buf2 + strlen(buf2) + 1; p < end; p++) {
- switch(state){
- case DSTATE_INITIAL:
- if ( *p == ':' ) {
- *p = '\0';
- if ( !strcmp(tok, "phy") ) {
- state = DSTATE_PHYSPATH;
- disk->phystype = PHYSTYPE_PHY;
- }else if ( !strcmp(tok, "file") ) {
- state = DSTATE_PHYSPATH;
- disk->phystype = PHYSTYPE_FILE;
- }else if ( !strcmp(tok, "tap") ) {
- state = DSTATE_TAP;
- }else{
- fprintf(stderr, "Unknown disk type: %s\n", tok);
- return 0;
- }
- tok = p + 1;
- } else if (*p == ',') {
- state = DSTATE_VIRTPATH;
- disk->phystype = PHYSTYPE_EMPTY;
- disk->physpath = strdup("");
- tok = p + 1;
- }
- break;
- case DSTATE_TAP:
- if ( *p == ':' ) {
- *p = '\0';
- if ( !strcmp(tok, "aio") ) {
- disk->phystype = PHYSTYPE_AIO;
- }else if ( !strcmp(tok, "vhd") ) {
- disk->phystype = PHYSTYPE_VHD;
- }else if ( !strcmp(tok, "qcow") ) {
- disk->phystype = PHYSTYPE_QCOW;
- }else if ( !strcmp(tok, "qcow2") ) {
- disk->phystype = PHYSTYPE_QCOW2;
- }else {
- fprintf(stderr, "Unknown tapdisk type: %s\n", tok);
- return 0;
- }
-
- tok = p + 1;
- state = DSTATE_PHYSPATH;
- }
- break;
- case DSTATE_PHYSPATH:
- if ( *p == ',' ) {
- int ioemu_len;
-
- *p = '\0';
- disk->physpath = (*tok) ? strdup(tok) : NULL;
- tok = p + 1;
-
- /* hack for ioemu disk spec */
- ioemu_len = strlen("ioemu:");
- state = DSTATE_VIRTPATH;
- if ( tok + ioemu_len < end &&
- !strncmp(tok, "ioemu:", ioemu_len)) {
- tok += ioemu_len;
- p += ioemu_len;
- }
- }
- break;
- case DSTATE_VIRTPATH:
- if ( *p == ',' || *p == ':' || *p == '\0' ) {
- switch(*p) {
- case ':':
- state = DSTATE_VIRTTYPE;
- break;
- case ',':
- state = DSTATE_RW;
- break;
- case '\0':
- state = DSTATE_TERMINAL;
- break;
- }
- if ( tok == p )
- goto out;
- *p = '\0';
- disk->virtpath = (*tok) ? strdup(tok) : NULL;
- tok = p + 1;
- }
- break;
- case DSTATE_VIRTTYPE:
- if ( *p == ',' || *p == '\0' ) {
- *p = '\0';
- if ( !strcmp(tok, "cdrom") ) {
- disk->is_cdrom = 1;
- disk->unpluggable = 1;
- }else{
- fprintf(stderr, "Unknown virtual disk type: %s\n", tok);
- return 0;
- }
- tok = p + 1;
- state = (*p == ',') ? DSTATE_RW : DSTATE_TERMINAL;
- }
- break;
- case DSTATE_RW:
- if ( *p == '\0' ) {
- disk->readwrite = (tok[0] == 'w');
- tok = p + 1;
- state = DSTATE_TERMINAL;
- }
- break;
- case DSTATE_TERMINAL:
- goto out;
- }
- }
-
-out:
- if ( tok != p || state != DSTATE_TERMINAL ) {
- fprintf(stderr, "parse error in disk config near '%s'\n", tok);
- return 0;
- }
-
- return 1;
+ ret_val = parse_disk_attrib(disk, buf2);
+ if ( ret_val )
+ return ret_val;
+
+ ret_val = parse_disk_vdev_info(disk, buf2);
+ if ( ret_val )
+ return ret_val;
+
+ return parse_disk_pdev_info(disk, buf2);
}
static void parse_config_data(const char *configfile_filename_report,
@@ -762,7 +789,7 @@ static void parse_config_data(const char
d_config->disks = (libxl_device_disk *) realloc(d_config->disks, sizeof (libxl_device_disk) * (d_config->num_disks + 1));
disk = d_config->disks + d_config->num_disks;
- if ( !parse_disk_config(disk, buf2) ) {
+ if ( parse_disk_config(disk, buf2) ) {
exit(1);
}
@@ -1838,25 +1865,24 @@ static void cd_insert(const char *dom, c
p = strchr(phys, ':');
if (!p) {
fprintf(stderr, "No type specified, ");
- disk.physpath = phys;
+ disk.pdev_path = phys;
if (!strncmp(phys, "/dev", 4)) {
fprintf(stderr, "assuming phy:\n");
- disk.phystype = PHYSTYPE_PHY;
+ disk.pdev_type = DISK_PDEV_TYPE_PHY;
} else {
fprintf(stderr, "assuming file:\n");
- disk.phystype = PHYSTYPE_FILE;
+ disk.pdev_type = DISK_PDEV_TYPE_FILE;
}
} else {
*p = '\0';
p++;
- disk.physpath = p;
- libxl_string_to_phystype(&ctx, phys, &disk.phystype);
- }
- } else {
- disk.physpath = strdup("");
- disk.phystype = PHYSTYPE_EMPTY;
- }
- disk.virtpath = (char*)virtdev;
+ disk.pdev_path = p;
+ libxl_string_to_phystype(&ctx, phys, &disk.pdev_type);
+ }
+ } else
+ disk.pdev_path = strdup("");
+
+ disk.vdev_path = (char*)virtdev;
disk.unpluggable = 1;
disk.readwrite = 0;
disk.is_cdrom = 1;
@@ -4375,19 +4401,19 @@ int main_blockattach(int argc, char **ar
tok = strtok(argv[optind+1], ":");
if (!strcmp(tok, "phy")) {
- disk.phystype = PHYSTYPE_PHY;
+ disk.pdev_type = DISK_PDEV_TYPE_PHY;
} else if (!strcmp(tok, "file")) {
- disk.phystype = PHYSTYPE_FILE;
+ disk.pdev_type = DISK_PDEV_TYPE_FILE;
} else if (!strcmp(tok, "tap")) {
tok = strtok(NULL, ":");
if (!strcmp(tok, "aio")) {
- disk.phystype = PHYSTYPE_AIO;
+ disk.impl_type = DISK_IMPL_TYPE_AIO;
} else if (!strcmp(tok, "vhd")) {
- disk.phystype = PHYSTYPE_VHD;
+ disk.format = DISK_FORMAT_VHD;
} else if (!strcmp(tok, "qcow")) {
- disk.phystype = PHYSTYPE_QCOW;
+ disk.format = DISK_FORMAT_QCOW;
} else if (!strcmp(tok, "qcow2")) {
- disk.phystype = PHYSTYPE_QCOW2;
+ disk.format = DISK_FORMAT_QCOW2;
} else {
fprintf(stderr, "Error: `%s' is not a valid disk image.\n", tok);
return 1;
@@ -4396,12 +4422,12 @@ int main_blockattach(int argc, char **ar
fprintf(stderr, "Error: `%s' is not a valid block device.\n", tok);
return 1;
}
- disk.physpath = strtok(NULL, "\0");
- if (!disk.physpath) {
+ disk.pdev_path = strtok(NULL, "\0");
+ if (!disk.pdev_path) {
fprintf(stderr, "Error: missing path to disk image.\n");
return 1;
}
- disk.virtpath = argv[optind+2];
+ disk.vdev_path = argv[optind+2];
disk.unpluggable = 1;
disk.readwrite = ((argc-optind <= 3) || (argv[optind+3][0] == 'w'));
diff -r 52e928af3637 tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c Fri Jan 28 19:37:49 2011 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c Sun Jan 30 11:47:22 2011 -0500
@@ -779,12 +779,12 @@ PyMODINIT_FUNC initxl(void)
_INT_CONST_LIBXL(m, CONSBACK_XENCONSOLED);
_INT_CONST_LIBXL(m, CONSBACK_IOEMU);
- _INT_CONST(m, PHYSTYPE_QCOW);
- _INT_CONST(m, PHYSTYPE_QCOW2);
- _INT_CONST(m, PHYSTYPE_VHD);
- _INT_CONST(m, PHYSTYPE_AIO);
- _INT_CONST(m, PHYSTYPE_FILE);
- _INT_CONST(m, PHYSTYPE_PHY);
+ _INT_CONST(m, DISK_FORMAT_QCOW);
+ _INT_CONST(m, DISK_FORMAT_QCOW2);
+ _INT_CONST(m, DISK_FORMAT_VHD);
+ _INT_CONST(m, DISK_IMPL_TYPE_AIO);
+ _INT_CONST(m, DISK_PDEV_TYPE_FILE);
+ _INT_CONST(m, DISK_PDEV_TYPE_PHY);
_INT_CONST(m, NICTYPE_IOEMU);
_INT_CONST(m, NICTYPE_VIF);
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] xl disk configuration handling
2011-01-30 17:46 [RFC] xl disk configuration handling Kamala Narasimhan
@ 2011-01-31 17:18 ` Stefano Stabellini
2011-01-31 19:25 ` Ian Campbell
2011-01-31 20:19 ` Kamala Narasimhan
0 siblings, 2 replies; 7+ messages in thread
From: Stefano Stabellini @ 2011-01-31 17:18 UTC (permalink / raw)
To: Kamala Narasimhan; +Cc: xen-devel
On Sun, 30 Jan 2011, Kamala Narasimhan wrote:
> diff -r 52e928af3637 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h Fri Jan 28 19:37:49 2011 +0000
> +++ b/tools/libxl/libxl.h Sun Jan 30 11:47:22 2011 -0500
> @@ -172,14 +172,27 @@ typedef enum {
> } libxl_console_consback;
>
> typedef enum {
> - PHYSTYPE_QCOW = 1,
> - PHYSTYPE_QCOW2,
> - PHYSTYPE_VHD,
> - PHYSTYPE_AIO,
> - PHYSTYPE_FILE,
> - PHYSTYPE_PHY,
> - PHYSTYPE_EMPTY,
> -} libxl_disk_phystype;
> + DISK_FORMAT_UNKNOWN = 0,
> + DISK_FORMAT_QCOW,
> + DISK_FORMAT_QCOW2,
> + DISK_FORMAT_VHD,
> + DISK_FORMAT_RAW,
> +} libxl_disk_format;
> +
> +typedef enum {
> + DISK_IMPL_TYPE_UNKNOWN = 0,
> + DISK_IMPL_TYPE_AIO,
> + DISK_IMPL_TYPE_TAPDISK,
> + DISK_IMPL_TYPE_IOEMU,
> +} libxl_disk_impl_type;
> +
> +typedef enum {
> + DISK_PDEV_TYPE_UNKNOWN = 0,
> + DISK_PDEV_TYPE_PHY,
> + DISK_PDEV_TYPE_FILE,
> + DISK_PDEV_TYPE_TAP,
> + DISK_PDEV_TYPE_TAP2,
> +} libxl_disk_pdev_type;
>
> typedef enum {
> NICTYPE_IOEMU = 1,
I agree that libxl_disk_phystype expresses both the format and the
backend type in a single confusing way, so there should be two enums:
one for the format (libxl_disk_format) and one for the backend type
(libxl_disk_pdev_type).
However I don't think libxl_disk_impl_type should be exposed at all, it
should be up to libxl to decide whether AIO should be enabled or not. It
might be useful to let the user disable the PV interface for a
particular disk (that is what ioemu stands for), but it is too late for
4.1, let's just ignore ioemu for the moment.
The backend types should be BLKBACK, TAPDISK2, and QEMU; TAPDISK refers
to blktap1 that is not supported by libxl. However libxl uses "tap:" as
backend string corresponding to TAPDISK2, I understand that might be
confusing but I wouldn't change it now.
Also it might be useful to retain the EMPTY format among the various
libxl_disk_format's, it should reduce the overall amount of changes.
> diff -r 52e928af3637 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c Fri Jan 28 19:37:49 2011 +0000
> +++ b/tools/libxl/libxl.c Sun Jan 30 11:47:22 2011 -0500
> @@ -588,7 +588,7 @@ int libxl_wait_for_disk_ejects(libxl_ctx
> for (i = 0; i < num_disks; i++) {
> if (asprintf(&(waiter[i].path), "%s/device/vbd/%d/eject",
> libxl__xs_get_dompath(&gc, domid),
> - libxl__device_disk_dev_number(disks[i].virtpath)) < 0)
> + libxl__device_disk_dev_number(disks[i].vdev_path)) < 0)
> goto out;
> if (asprintf(&(waiter[i].token), "%d", LIBXL_EVENT_DISK_EJECT) < 0)
> goto out;
it would be nice if all the renaming was done in a separate patch so
that the real changes are easier to read
> @@ -920,37 +920,31 @@ int libxl_device_disk_add(libxl_ctx *ctx
> device.domid = disk->domid;
> device.kind = DEVICE_VBD;
>
> - switch (disk->phystype) {
> - case PHYSTYPE_PHY: {
> + switch (disk->pdev_type) {
> + case DISK_PDEV_TYPE_PHY: {
>
> - libxl__device_physdisk_major_minor(disk->physpath, &major, &minor);
> + libxl__device_physdisk_major_minor(disk->pdev_path, &major, &minor);
> flexarray_append(back, "physical-device");
> flexarray_append(back, libxl__sprintf(&gc, "%x:%x", major, minor));
>
> flexarray_append(back, "params");
> - flexarray_append(back, disk->physpath);
> + flexarray_append(back, disk->pdev_path);
>
> device.backend_kind = DEVICE_VBD;
> break;
> }
> - case PHYSTYPE_EMPTY:
> - break;
> - 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:
> + case DISK_PDEV_TYPE_FILE:
> + case DISK_PDEV_TYPE_TAP:
> + case DISK_PDEV_TYPE_TAP2:
> if (libxl__blktap_enabled(&gc)) {
> const char *dev = libxl__blktap_devpath(&gc,
> - disk->physpath, disk->phystype);
> + disk->pdev_path, disk->pdev_type);
> if (!dev) {
> rc = ERROR_FAIL;
> goto out_free;
> }
> flexarray_append(back, "tapdisk-params");
> - flexarray_append(back, libxl__sprintf(&gc, "%s:%s", libxl__device_disk_string_of_phystype(disk->phystype), disk->physpath));
> + flexarray_append(back, libxl__sprintf(&gc, "%s:%s", libxl__device_disk_string_of_phystype(disk->pdev_type), disk->pdev_path));
> flexarray_append(back, "params");
> flexarray_append(back, libxl__strdup(&gc, dev));
> backend_type = "phy";
> @@ -963,7 +957,7 @@ int libxl_device_disk_add(libxl_ctx *ctx
> }
> flexarray_append(back, "params");
> flexarray_append(back, libxl__sprintf(&gc, "%s:%s",
> - libxl__device_disk_string_of_phystype(disk->phystype), disk->physpath));
> + libxl__device_disk_string_of_phystype(disk->pdev_type), disk->pdev_path));
>
> if (libxl__blktap_enabled(&gc))
> device.backend_kind = DEVICE_TAP;
It looks like EMPTY, QCOW and QCOW2 disappeared.
> @@ -1044,32 +1038,33 @@ char * libxl_device_disk_local_attach(li
> libxl__gc gc = LIBXL_INIT_GC(ctx);
> const char *dev = NULL;
> char *ret = NULL;
> - int phystype = disk->phystype;
> + int phystype = disk->pdev_type;
> switch (phystype) {
> - case PHYSTYPE_PHY: {
> - fprintf(stderr, "attaching PHY disk %s to domain 0\n", disk->physpath);
> - dev = disk->physpath;
> + case DISK_PDEV_TYPE_PHY: {
> + fprintf(stderr, "attaching PHY disk %s to domain 0\n", disk->pdev_path);
> + dev = disk->pdev_path;
> break;
> }
> - case PHYSTYPE_FILE:
> - /* let's pretend is tap:aio for the moment */
> - phystype = PHYSTYPE_AIO;
> - case PHYSTYPE_AIO:
> - if (!libxl__blktap_enabled(&gc)) {
> - dev = disk->physpath;
> + case DISK_PDEV_TYPE_FILE:
> + case DISK_PDEV_TYPE_TAP:
> + case DISK_PDEV_TYPE_TAP2:
> + if ( disk->impl_type == DISK_IMPL_TYPE_AIO ) {
> + if (!libxl__blktap_enabled(&gc)) {
> + dev = disk->pdev_path;
> + break;
> + }
> + }
I wouldn't take into account "aio" here.
> @@ -203,9 +205,11 @@ libxl_device_disk = Struct("device_disk"
> libxl_device_disk = Struct("device_disk", [
> ("backend_domid", uint32),
> ("domid", domid),
> - ("physpath", string),
> - ("phystype", libxl_disk_phystype),
> - ("virtpath", string),
> + ("pdev_path", string),
> + ("vdev_path", string),
> + ("pdev_type", libxl_disk_pdev_type),
> + ("impl_type", libxl_disk_impl_type),
> + ("format", libxl_disk_format),
> ("unpluggable", integer),
> ("readwrite", integer),
> ("is_cdrom", integer),
we don't need impl_type
> diff -r 52e928af3637 tools/libxl/libxl_device.c
> --- a/tools/libxl/libxl_device.c Fri Jan 28 19:37:49 2011 +0000
> +++ b/tools/libxl/libxl_device.c Sun Jan 30 11:47:22 2011 -0500
> @@ -121,31 +121,23 @@ out:
> return rc;
> }
>
> -char *libxl__device_disk_string_of_phystype(libxl_disk_phystype phystype)
> +char *libxl__device_disk_string_of_phystype(libxl_disk_pdev_type pdev_type)
> {
> - switch (phystype) {
> - case PHYSTYPE_QCOW: return "qcow";
> - case PHYSTYPE_QCOW2: return "qcow2";
> - case PHYSTYPE_VHD: return "vhd";
> - case PHYSTYPE_AIO: return "aio";
> - case PHYSTYPE_FILE: return "file";
> - case PHYSTYPE_PHY: return "phy";
> - case PHYSTYPE_EMPTY: return "file";
> + switch (pdev_type) {
> + case DISK_PDEV_TYPE_FILE: return "file";
> + case DISK_PDEV_TYPE_PHY: return "phy";
> + case DISK_PDEV_TYPE_TAP: return "tap";
> + case DISK_PDEV_TYPE_TAP2: return "tap2";
> default: return NULL;
> }
> }
I think we need to return the format string here, like "vhd", otherwise
tapdisk2 wouldn't work correctly. This function should take a
libxl_disk_format as an argument now.
>
> -char *libxl__device_disk_backend_type_of_phystype(libxl_disk_phystype phystype)
> +char *libxl__device_disk_backend_type_of_phystype(libxl_disk_pdev_type pdev_type)
> {
> - switch (phystype) {
> - case PHYSTYPE_QCOW: return "tap";
> - case PHYSTYPE_QCOW2: return "tap";
> - case PHYSTYPE_VHD: return "tap";
> - case PHYSTYPE_AIO: return "tap";
> + switch (pdev_type) {
> /* let's pretend file is tap:aio */
> - case PHYSTYPE_FILE: return "tap";
> - case PHYSTYPE_EMPTY: return "tap";
> - case PHYSTYPE_PHY: return "phy";
> + case DISK_PDEV_TYPE_FILE: return "tap";
> + case DISK_PDEV_TYPE_PHY: return "phy";
> default: return NULL;
> }
> }
> diff -r 52e928af3637 tools/libxl/libxl_dm.c
> --- a/tools/libxl/libxl_dm.c Fri Jan 28 19:37:49 2011 +0000
> +++ b/tools/libxl/libxl_dm.c Sun Jan 30 11:47:22 2011 -0500
> @@ -303,10 +303,10 @@ static char ** libxl_build_device_model_
> for (i; i < nb; i++) {
> if (disks[i].is_cdrom) {
> flexarray_append(dm_args, "-cdrom");
> - flexarray_append(dm_args, libxl__strdup(gc, disks[i].physpath));
> + flexarray_append(dm_args, libxl__strdup(gc, disks[i].pdev_path));
> } else {
> - flexarray_append(dm_args, libxl__sprintf(gc, "-%s", disks[i].virtpath));
> - flexarray_append(dm_args, libxl__strdup(gc, disks[i].physpath));
> + flexarray_append(dm_args, libxl__sprintf(gc, "-%s", disks[i].vdev_path));
> + flexarray_append(dm_args, libxl__strdup(gc, disks[i].pdev_path));
> }
> libxl_device_disk_destroy(&disks[i]);
> }
> diff -r 52e928af3637 tools/libxl/libxl_internal.h
> --- a/tools/libxl/libxl_internal.h Fri Jan 28 19:37:49 2011 +0000
> +++ b/tools/libxl/libxl_internal.h Sun Jan 30 11:47:22 2011 -0500
> @@ -178,8 +178,8 @@ _hidden void libxl__userdata_destroyall(
> _hidden void libxl__userdata_destroyall(libxl_ctx *ctx, uint32_t domid);
>
> /* from xl_device */
> -_hidden char *libxl__device_disk_backend_type_of_phystype(libxl_disk_phystype phystype);
> -_hidden char *libxl__device_disk_string_of_phystype(libxl_disk_phystype phystype);
> +_hidden char *libxl__device_disk_backend_type_of_phystype(libxl_disk_pdev_type pdev_type);
> +_hidden char *libxl__device_disk_string_of_phystype(libxl_disk_pdev_type pdev_type);
>
> _hidden int libxl__device_physdisk_major_minor(const char *physpath, int *major, int *minor);
> _hidden int libxl__device_disk_dev_number(char *virtpath);
> @@ -306,7 +306,7 @@ _hidden int libxl__blktap_enabled(libxl_
> */
> _hidden const char *libxl__blktap_devpath(libxl__gc *gc,
> const char *disk,
> - libxl_disk_phystype phystype);
> + libxl_disk_pdev_type pdev_type);
>
> _hidden char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid);
>
> diff -r 52e928af3637 tools/libxl/libxl_utils.c
> --- a/tools/libxl/libxl_utils.c Fri Jan 28 19:37:49 2011 +0000
> +++ b/tools/libxl/libxl_utils.c Sun Jan 30 11:47:22 2011 -0500
> @@ -275,34 +275,20 @@ out:
> return rc;
> }
>
> -int libxl_string_to_phystype(libxl_ctx *ctx, char *s, libxl_disk_phystype *phystype)
> +int libxl_string_to_phystype(libxl_ctx *ctx, char *s, libxl_disk_pdev_type *pdev_type)
> {
> - char *p;
> - int rc = 0;
> + *pdev_type = DISK_PDEV_TYPE_UNKNOWN;
>
> - if (!strcmp(s, "phy")) {
> - *phystype = PHYSTYPE_PHY;
> - } else if (!strcmp(s, "file")) {
> - *phystype = PHYSTYPE_FILE;
> - } else if (!strcmp(s, "tap")) {
> - p = strchr(s, ':');
> - if (!p) {
> - rc = ERROR_INVAL;
> - goto out;
> - }
> - p++;
> - if (!strcmp(p, "aio")) {
> - *phystype = PHYSTYPE_AIO;
> - } else if (!strcmp(p, "vhd")) {
> - *phystype = PHYSTYPE_VHD;
> - } else if (!strcmp(p, "qcow")) {
> - *phystype = PHYSTYPE_QCOW;
> - } else if (!strcmp(p, "qcow2")) {
> - *phystype = PHYSTYPE_QCOW2;
> - }
> - }
> -out:
> - return rc;
> + if ( !strncmp(s, "phy", sizeof("phy")-1) )
> + *pdev_type = DISK_PDEV_TYPE_PHY;
> + else if ( !strncmp(s, "file", sizeof("file")-1) )
> + *pdev_type = DISK_PDEV_TYPE_FILE;
> + else if ( !strncmp(s, "tap", sizeof("tap")-1) )
> + *pdev_type = DISK_PDEV_TYPE_TAP;
> + else if ( !strncmp(s, "tap2", sizeof("tap2")-1) )
> + *pdev_type = DISK_PDEV_TYPE_TAP2;
> +
> + return 0;
> }
Drop tap2.
> diff -r 52e928af3637 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c Fri Jan 28 19:37:49 2011 +0000
> +++ b/tools/libxl/xl_cmdimpl.c Sun Jan 30 11:47:22 2011 -0500
> @@ -361,9 +361,9 @@ static void printf_info(int domid,
> printf("\t\t(tap\n");
> printf("\t\t\t(backend_domid %d)\n", d_config->disks[i].backend_domid);
> printf("\t\t\t(domid %d)\n", d_config->disks[i].domid);
> - printf("\t\t\t(physpath %s)\n", d_config->disks[i].physpath);
> - printf("\t\t\t(phystype %d)\n", d_config->disks[i].phystype);
> - printf("\t\t\t(virtpath %s)\n", d_config->disks[i].virtpath);
> + printf("\t\t\t(physpath %s)\n", d_config->disks[i].pdev_path);
> + printf("\t\t\t(phystype %d)\n", d_config->disks[i].pdev_type);
> + printf("\t\t\t(virtpath %s)\n", d_config->disks[i].vdev_path);
> printf("\t\t\t(unpluggable %d)\n", d_config->disks[i].unpluggable);
> printf("\t\t\t(readwrite %d)\n", d_config->disks[i].readwrite);
> printf("\t\t\t(is_cdrom %d)\n", d_config->disks[i].is_cdrom);
> @@ -438,137 +438,164 @@ static int parse_action_on_shutdown(cons
> return 0;
> }
>
> -#define DSTATE_INITIAL 0
> -#define DSTATE_TAP 1
> -#define DSTATE_PHYSPATH 2
> -#define DSTATE_VIRTPATH 3
> -#define DSTATE_VIRTTYPE 4
> -#define DSTATE_RW 5
> -#define DSTATE_TERMINAL 6
> -
> +static int parse_disk_attrib(libxl_device_disk *disk, char *buf2)
> +{
> + char *attrib = strrchr(buf2, ',');
> + if ( !attrib ) {
> + LOG("Invalid disk configuration option %s. Refer to the xl disk "
> + "configuration document for further information", buf2);
> + return ERROR_INVAL;
> + }
> +
> + *attrib = '\0';
> + attrib++;
> + if ( attrib[0] == 'w' )
> + disk->readwrite = 1;
> + else if ( attrib[0] != 'r' ) {
> + LOG("Required access rights attribute is missing or incorrect in "
> + "disk configuration option %s", buf2);
> + return ERROR_INVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int parse_disk_vdev_info(libxl_device_disk *disk, char *buf2)
> +{
> + char *vdev_info, *vdev, *type;
> +
> + vdev_info = strrchr(buf2, ',');
> + if ( !vdev_info ) {
> + LOG("Required vdev info missing in disk configuration option");
> + return ERROR_INVAL;
> + }
> +
> + *vdev_info = '\0';
> + vdev_info++;
> +
> + type = strchr(vdev_info, ':');
> + if ( type ) {
> + *type = '\0';
> + type++;
> + if ( !strncmp(type, "cdrom", sizeof("cdrom")-1) ) {
> + disk->is_cdrom = 1;
> + disk->unpluggable = 1;
> + }
> + else {
> + LOG("Invalid vdev type %s specified in disk configuration option",
> + type);
> + return ERROR_INVAL;
> + }
> + }
> +
> + vdev = vdev_info;
> + if ( vdev[0] == '\0' ) {
> + LOG("Mandatory attribute vdev not specified");
> + return ERROR_INVAL;
> + }
> +
> + disk->vdev_path = strdup(vdev);
> + return 0;
> +}
> +
> +static int parse_disk_pdev_info(libxl_device_disk *disk, char *buf2)
> +{
> + char *pdev_info, *pdev_type, *pdev_impl, *pdev_format, *pdev_path;
> +
> + pdev_info = buf2;
> + if ( pdev_info[0] == '\0' && !disk->is_cdrom ) {
> + /* pdev can be empty string only for cdrom drive with no media inserted */
> + LOG("Required vdev info missing in disk configuration option");
> + return ERROR_INVAL;
> + }
> +
> + pdev_path = strrchr(pdev_info, ':');
> + if ( !pdev_path ) {
> + disk->pdev_path = strdup(pdev_info);
> + return 0;
> + }
> +
> + *pdev_path = '\0';
> + disk->pdev_path = strdup(++pdev_path);
> +
> + pdev_format = strrchr(pdev_info, ':');
> + if ( !pdev_format )
> + pdev_format = pdev_info;
> + else {
> + pdev_format = '\0';
> + pdev_format++;
> + }
> +
> + if ( !strncmp(pdev_format, "raw", sizeof("raw")-1) )
> + disk->format = DISK_FORMAT_RAW;
> + else if ( !strncmp(pdev_format, "qcow", sizeof("qcow")-1) )
> + disk->format = DISK_FORMAT_QCOW;
> + else if ( !strncmp(pdev_format, "qcow2", sizeof("qcow2")-1) )
> + disk->format = DISK_FORMAT_QCOW2;
> + else if ( !strncmp(pdev_format, "vhd", sizeof("vhd")-1) )
> + disk->format = DISK_FORMAT_VHD;
> +
> + if ( disk->format == DISK_FORMAT_UNKNOWN )
> + pdev_impl = pdev_format;
> + else {
> + pdev_impl = strrchr(pdev_info, ':');
> + if ( !pdev_impl )
> + return 0;
> + *pdev_impl = '\0';
> + pdev_impl++;
> + }
> +
> + if ( !strncmp(pdev_impl, "aio", sizeof("aio")-1) )
> + disk->impl_type = DISK_IMPL_TYPE_AIO;
> + else if ( !strncmp(pdev_impl, "tapdisk", sizeof("tapdisk")-1) )
> + disk->impl_type = DISK_IMPL_TYPE_TAPDISK;
> + else if ( !strncmp(pdev_impl, "ioemu", sizeof("ioemu")-1) )
> + disk->impl_type = DISK_IMPL_TYPE_IOEMU;
> +
> + if ( disk->impl_type == DISK_IMPL_TYPE_UNKNOWN )
> + pdev_type = pdev_impl;
> + else {
> + pdev_type = pdev_info;
> + if ( pdev_type[0] == '\0' )
> + return 0;
> + }
> +
> + if ( !strncmp(pdev_type, "phy", sizeof("phy")-1) )
> + disk->pdev_type = DISK_PDEV_TYPE_PHY;
> + else if ( !strncmp(pdev_type, "file", sizeof("file")-1) )
> + disk->pdev_type = DISK_PDEV_TYPE_FILE;
> + else if ( !strncmp(pdev_type, "tap", sizeof("tap")-1) )
> + disk->pdev_type = DISK_PDEV_TYPE_TAP;
> + else if ( !strncmp(pdev_type, "tap2", sizeof("tap2")-1) )
> + disk->pdev_type = DISK_PDEV_TYPE_TAP2;
> +
> + return 0;
> +}
> +
> +/*
> + * Note: Following code calls methods each of which will parse
> + * specific chunks of the disk configuration option, the specific
> + * chunks being attribute, vdev and pdev. As with any parsing code
> + * it makes assumption about the order in which specific chunks appear
> + * in the disk configuration option string. So, please use
> + * care while modifying the code below, esp. when it comes to
> + * the order of calls.
> + */
> static int parse_disk_config(libxl_device_disk *disk, char *buf2)
> {
> - int state = DSTATE_INITIAL;
> - char *p, *end, *tok;
> + int ret_val;
>
> memset(disk, 0, sizeof(*disk));
>
> - for(tok = p = buf2, end = buf2 + strlen(buf2) + 1; p < end; p++) {
> - switch(state){
> - case DSTATE_INITIAL:
> - if ( *p == ':' ) {
> - *p = '\0';
> - if ( !strcmp(tok, "phy") ) {
> - state = DSTATE_PHYSPATH;
> - disk->phystype = PHYSTYPE_PHY;
> - }else if ( !strcmp(tok, "file") ) {
> - state = DSTATE_PHYSPATH;
> - disk->phystype = PHYSTYPE_FILE;
> - }else if ( !strcmp(tok, "tap") ) {
> - state = DSTATE_TAP;
> - }else{
> - fprintf(stderr, "Unknown disk type: %s\n", tok);
> - return 0;
> - }
> - tok = p + 1;
> - } else if (*p == ',') {
> - state = DSTATE_VIRTPATH;
> - disk->phystype = PHYSTYPE_EMPTY;
> - disk->physpath = strdup("");
> - tok = p + 1;
> - }
> - break;
> - case DSTATE_TAP:
> - if ( *p == ':' ) {
> - *p = '\0';
> - if ( !strcmp(tok, "aio") ) {
> - disk->phystype = PHYSTYPE_AIO;
> - }else if ( !strcmp(tok, "vhd") ) {
> - disk->phystype = PHYSTYPE_VHD;
> - }else if ( !strcmp(tok, "qcow") ) {
> - disk->phystype = PHYSTYPE_QCOW;
> - }else if ( !strcmp(tok, "qcow2") ) {
> - disk->phystype = PHYSTYPE_QCOW2;
> - }else {
> - fprintf(stderr, "Unknown tapdisk type: %s\n", tok);
> - return 0;
> - }
> -
> - tok = p + 1;
> - state = DSTATE_PHYSPATH;
> - }
> - break;
> - case DSTATE_PHYSPATH:
> - if ( *p == ',' ) {
> - int ioemu_len;
> -
> - *p = '\0';
> - disk->physpath = (*tok) ? strdup(tok) : NULL;
> - tok = p + 1;
> -
> - /* hack for ioemu disk spec */
> - ioemu_len = strlen("ioemu:");
> - state = DSTATE_VIRTPATH;
> - if ( tok + ioemu_len < end &&
> - !strncmp(tok, "ioemu:", ioemu_len)) {
> - tok += ioemu_len;
> - p += ioemu_len;
> - }
> - }
> - break;
> - case DSTATE_VIRTPATH:
> - if ( *p == ',' || *p == ':' || *p == '\0' ) {
> - switch(*p) {
> - case ':':
> - state = DSTATE_VIRTTYPE;
> - break;
> - case ',':
> - state = DSTATE_RW;
> - break;
> - case '\0':
> - state = DSTATE_TERMINAL;
> - break;
> - }
> - if ( tok == p )
> - goto out;
> - *p = '\0';
> - disk->virtpath = (*tok) ? strdup(tok) : NULL;
> - tok = p + 1;
> - }
> - break;
> - case DSTATE_VIRTTYPE:
> - if ( *p == ',' || *p == '\0' ) {
> - *p = '\0';
> - if ( !strcmp(tok, "cdrom") ) {
> - disk->is_cdrom = 1;
> - disk->unpluggable = 1;
> - }else{
> - fprintf(stderr, "Unknown virtual disk type: %s\n", tok);
> - return 0;
> - }
> - tok = p + 1;
> - state = (*p == ',') ? DSTATE_RW : DSTATE_TERMINAL;
> - }
> - break;
> - case DSTATE_RW:
> - if ( *p == '\0' ) {
> - disk->readwrite = (tok[0] == 'w');
> - tok = p + 1;
> - state = DSTATE_TERMINAL;
> - }
> - break;
> - case DSTATE_TERMINAL:
> - goto out;
> - }
> - }
> -
> -out:
> - if ( tok != p || state != DSTATE_TERMINAL ) {
> - fprintf(stderr, "parse error in disk config near '%s'\n", tok);
> - return 0;
> - }
> -
> - return 1;
> + ret_val = parse_disk_attrib(disk, buf2);
> + if ( ret_val )
> + return ret_val;
> +
> + ret_val = parse_disk_vdev_info(disk, buf2);
> + if ( ret_val )
> + return ret_val;
> +
> + return parse_disk_pdev_info(disk, buf2);
> }
>
> static void parse_config_data(const char *configfile_filename_report,
do we really need to change the parsing function that much? I
understand there are significant changes but this is a total rewrite.
I am concerned about all the bugs we might find later after the
release...
> @@ -762,7 +789,7 @@ static void parse_config_data(const char
>
> d_config->disks = (libxl_device_disk *) realloc(d_config->disks, sizeof (libxl_device_disk) * (d_config->num_disks + 1));
> disk = d_config->disks + d_config->num_disks;
> - if ( !parse_disk_config(disk, buf2) ) {
> + if ( parse_disk_config(disk, buf2) ) {
> exit(1);
> }
>
> @@ -1838,25 +1865,24 @@ static void cd_insert(const char *dom, c
> p = strchr(phys, ':');
> if (!p) {
> fprintf(stderr, "No type specified, ");
> - disk.physpath = phys;
> + disk.pdev_path = phys;
> if (!strncmp(phys, "/dev", 4)) {
> fprintf(stderr, "assuming phy:\n");
> - disk.phystype = PHYSTYPE_PHY;
> + disk.pdev_type = DISK_PDEV_TYPE_PHY;
> } else {
> fprintf(stderr, "assuming file:\n");
> - disk.phystype = PHYSTYPE_FILE;
> + disk.pdev_type = DISK_PDEV_TYPE_FILE;
> }
> } else {
> *p = '\0';
> p++;
> - disk.physpath = p;
> - libxl_string_to_phystype(&ctx, phys, &disk.phystype);
> - }
> - } else {
> - disk.physpath = strdup("");
> - disk.phystype = PHYSTYPE_EMPTY;
> - }
> - disk.virtpath = (char*)virtdev;
> + disk.pdev_path = p;
> + libxl_string_to_phystype(&ctx, phys, &disk.pdev_type);
> + }
> + } else
> + disk.pdev_path = strdup("");
> +
> + disk.vdev_path = (char*)virtdev;
> disk.unpluggable = 1;
> disk.readwrite = 0;
> disk.is_cdrom = 1;
> @@ -4375,19 +4401,19 @@ int main_blockattach(int argc, char **ar
>
> tok = strtok(argv[optind+1], ":");
> if (!strcmp(tok, "phy")) {
> - disk.phystype = PHYSTYPE_PHY;
> + disk.pdev_type = DISK_PDEV_TYPE_PHY;
> } else if (!strcmp(tok, "file")) {
> - disk.phystype = PHYSTYPE_FILE;
> + disk.pdev_type = DISK_PDEV_TYPE_FILE;
> } else if (!strcmp(tok, "tap")) {
> tok = strtok(NULL, ":");
> if (!strcmp(tok, "aio")) {
> - disk.phystype = PHYSTYPE_AIO;
> + disk.impl_type = DISK_IMPL_TYPE_AIO;
I would completely ignore "aio:" here.
I would also ignore "ioemu:" the same way.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] xl disk configuration handling
2011-01-31 17:18 ` Stefano Stabellini
@ 2011-01-31 19:25 ` Ian Campbell
2011-01-31 20:39 ` Kamala Narasimhan
2011-01-31 20:19 ` Kamala Narasimhan
1 sibling, 1 reply; 7+ messages in thread
From: Ian Campbell @ 2011-01-31 19:25 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Kamala Narasimhan, xen-devel
On Mon, 2011-01-31 at 17:18 +0000, Stefano Stabellini wrote:
> On Sun, 30 Jan 2011, Kamala Narasimhan wrote:
> > diff -r 52e928af3637 tools/libxl/libxl.h
> > --- a/tools/libxl/libxl.h Fri Jan 28 19:37:49 2011 +0000
> > +++ b/tools/libxl/libxl.h Sun Jan 30 11:47:22 2011 -0500
> > @@ -172,14 +172,27 @@ typedef enum {
> > } libxl_console_consback;
> >
> > typedef enum {
> > - PHYSTYPE_QCOW = 1,
> > - PHYSTYPE_QCOW2,
> > - PHYSTYPE_VHD,
> > - PHYSTYPE_AIO,
> > - PHYSTYPE_FILE,
> > - PHYSTYPE_PHY,
> > - PHYSTYPE_EMPTY,
> > -} libxl_disk_phystype;
> > + DISK_FORMAT_UNKNOWN = 0,
> > + DISK_FORMAT_QCOW,
> > + DISK_FORMAT_QCOW2,
> > + DISK_FORMAT_VHD,
> > + DISK_FORMAT_RAW,
> > +} libxl_disk_format;
> > +
> > +typedef enum {
> > + DISK_IMPL_TYPE_UNKNOWN = 0,
> > + DISK_IMPL_TYPE_AIO,
> > + DISK_IMPL_TYPE_TAPDISK,
> > + DISK_IMPL_TYPE_IOEMU,
> > +} libxl_disk_impl_type;
> > +
> > +typedef enum {
> > + DISK_PDEV_TYPE_UNKNOWN = 0,
> > + DISK_PDEV_TYPE_PHY,
> > + DISK_PDEV_TYPE_FILE,
> > + DISK_PDEV_TYPE_TAP,
> > + DISK_PDEV_TYPE_TAP2,
> > +} libxl_disk_pdev_type;
> >
> > typedef enum {
> > NICTYPE_IOEMU = 1,
>
> I agree that libxl_disk_phystype expresses both the format and the
> backend type in a single confusing way, so there should be two enums:
> one for the format (libxl_disk_format) and one for the backend type
> (libxl_disk_pdev_type).
pdev_type doesn't seem like a very good name for "backend type" (and it
is unnecessarily abbreviated which I personally dislike, especially in
public interfaces).
Would libxl_disk_backend_type work?
> > diff -r 52e928af3637 tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c Fri Jan 28 19:37:49 2011 +0000
> > +++ b/tools/libxl/libxl.c Sun Jan 30 11:47:22 2011 -0500
> > @@ -588,7 +588,7 @@ int libxl_wait_for_disk_ejects(libxl_ctx
> > for (i = 0; i < num_disks; i++) {
> > if (asprintf(&(waiter[i].path), "%s/device/vbd/%d/eject",
> > libxl__xs_get_dompath(&gc, domid),
> > - libxl__device_disk_dev_number(disks[i].virtpath)) < 0)
> > + libxl__device_disk_dev_number(disks[i].vdev_path)) < 0)
> > goto out;
> > if (asprintf(&(waiter[i].token), "%d", LIBXL_EVENT_DISK_EJECT) < 0)
> > goto out;
>
> it would be nice if all the renaming was done in a separate patch so
> that the real changes are easier to read
Aren't the renamings a bit cosmetic anyway, i.e. could/should be left
for 4.2? I guess I can see the argument of changing the field name if
the type name changes, assuming the type names are well chosen.
The virtpath->vdev_path rename in particular seems odd. The main thing
which is wrong with virtpath is that the thing it contains is a vdev
which doesn't really have any path-like properties, but we retain the
path part in the new name. If it's renamed to anything I reckon it
should just be vdev.
Ian.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] xl disk configuration handling
2011-01-31 17:18 ` Stefano Stabellini
2011-01-31 19:25 ` Ian Campbell
@ 2011-01-31 20:19 ` Kamala Narasimhan
2011-02-01 11:13 ` Stefano Stabellini
1 sibling, 1 reply; 7+ messages in thread
From: Kamala Narasimhan @ 2011-01-31 20:19 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel
> I agree that libxl_disk_phystype expresses both the format and the
> backend type in a single confusing way, so there should be two enums:
> one for the format (libxl_disk_format) and one for the backend type
> (libxl_disk_pdev_type).
I will switch to two enums instead of three.
> However I don't think libxl_disk_impl_type should be exposed at all, it
> should be up to libxl to decide whether AIO should be enabled or not. It
> might be useful to let the user disable the PV interface for a
> particular disk (that is what ioemu stands for), but it is too late for
> 4.1, let's just ignore ioemu for the moment.
Ok.
> The backend types should be BLKBACK, TAPDISK2, and QEMU; TAPDISK refers
> to blktap1 that is not supported by libxl. However libxl uses "tap:" as
> backend string corresponding to TAPDISK2, I understand that might be
> confusing but I wouldn't change it now.
> Also it might be useful to retain the EMPTY format among the various
> libxl_disk_format's, it should reduce the overall amount of changes.
>
EMPTY, an indicator that there is no media in the cd-rom drive didn't really go
with the any of the enums which is why I removed it. But later when I was
changing code I did find it inconvenient to check for both empty path plus
cdrom, so I will add it to disk format types though I am really not sure if it
belongs there.
> it would be nice if all the renaming was done in a separate patch so
> that the real changes are easier to read
>
I was worried you may not accept a patch with just renaming changes! I could
separate interface changes (which would include renaming) from parsing and send
them as two separate patches. Would that be ok?
>
Stefano - I did go through your comments on a subset of code here but as I
mentioned in my earlier email, please ignore that code for now as I was going to
modify it anyway. It was mostly to help understand the places that require
change plus for the code to compile.
>
> do we really need to change the parsing function that much? I
> understand there are significant changes but this is a total rewrite.
> I am concerned about all the bugs we might find later after the
> release...
>
This is one change I would really like to go with. Not only does it help with
the changes we needed, it also gets rid of code duplication. With this change
block-attach can rely on the same parsing code (that is once I submit the
block-attach changes patch).
>
> I would completely ignore "aio:" here.
> I would also ignore "ioemu:" the same way.
>
This redundant logic in block attach for parsing will be gone and disk parsing
logic will be reused.
Kamala
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] xl disk configuration handling
2011-01-31 19:25 ` Ian Campbell
@ 2011-01-31 20:39 ` Kamala Narasimhan
0 siblings, 0 replies; 7+ messages in thread
From: Kamala Narasimhan @ 2011-01-31 20:39 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel, Stefano Stabellini
Ian Campbell wrote:
> On Mon, 2011-01-31 at 17:18 +0000, Stefano Stabellini wrote:
>> I agree that libxl_disk_phystype expresses both the format and the
>> backend type in a single confusing way, so there should be two enums:
>> one for the format (libxl_disk_format) and one for the backend type
>> (libxl_disk_pdev_type).
>
> pdev_type doesn't seem like a very good name for "backend type" (and it
> is unnecessarily abbreviated which I personally dislike, especially in
> public interfaces).
>
> Would libxl_disk_backend_type work?
>
Yes, I will go with libxl_disk_backend_type.
>>> diff -r 52e928af3637 tools/libxl/libxl.c
>>> --- a/tools/libxl/libxl.c Fri Jan 28 19:37:49 2011 +0000
>>> +++ b/tools/libxl/libxl.c Sun Jan 30 11:47:22 2011 -0500
>>> @@ -588,7 +588,7 @@ int libxl_wait_for_disk_ejects(libxl_ctx
>>> for (i = 0; i < num_disks; i++) {
>>> if (asprintf(&(waiter[i].path), "%s/device/vbd/%d/eject",
>>> libxl__xs_get_dompath(&gc, domid),
>>> - libxl__device_disk_dev_number(disks[i].virtpath)) < 0)
>>> + libxl__device_disk_dev_number(disks[i].vdev_path)) < 0)
>>> goto out;
>>> if (asprintf(&(waiter[i].token), "%d", LIBXL_EVENT_DISK_EJECT) < 0)
>>> goto out;
>> it would be nice if all the renaming was done in a separate patch so
>> that the real changes are easier to read
>
> Aren't the renamings a bit cosmetic anyway, i.e. could/should be left
> for 4.2? I guess I can see the argument of changing the field name if
> the type name changes, assuming the type names are well chosen.
>
I did consider keeping away from such renaming but then I figured I might as
well if I am going to make other changes to the interface.
> The virtpath->vdev_path rename in particular seems odd. The main thing
> which is wrong with virtpath is that the thing it contains is a vdev
> which doesn't really have any path-like properties, but we retain the
> path part in the new name. If it's renamed to anything I reckon it
> should just be vdev.
That makes sense. I will rename it to just vdev.
Kamala
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] xl disk configuration handling
2011-01-31 20:19 ` Kamala Narasimhan
@ 2011-02-01 11:13 ` Stefano Stabellini
2011-02-01 14:00 ` Kamala Narasimhan
0 siblings, 1 reply; 7+ messages in thread
From: Stefano Stabellini @ 2011-02-01 11:13 UTC (permalink / raw)
To: Kamala Narasimhan; +Cc: xen-devel, Stefano Stabellini
On Mon, 31 Jan 2011, Kamala Narasimhan wrote:
> EMPTY, an indicator that there is no media in the cd-rom drive didn't really go
> with the any of the enums which is why I removed it. But later when I was
> changing code I did find it inconvenient to check for both empty path plus
> cdrom, so I will add it to disk format types though I am really not sure if it
> belongs there.
It probably doesn't, but it is better to limit the scope of the fix at
the point.
> > it would be nice if all the renaming was done in a separate patch so
> > that the real changes are easier to read
> >
> I was worried you may not accept a patch with just renaming changes! I could
> separate interface changes (which would include renaming) from parsing and send
> them as two separate patches. Would that be ok?
>
Yep, that would be fine.
> > do we really need to change the parsing function that much? I
> > understand there are significant changes but this is a total rewrite.
> > I am concerned about all the bugs we might find later after the
> > release...
> >
> This is one change I would really like to go with. Not only does it help with
> the changes we needed, it also gets rid of code duplication. With this change
> block-attach can rely on the same parsing code (that is once I submit the
> block-attach changes patch).
It took us several iterations to get the parsing right, I would like to
keep the state machine and the field parsing as it is, but each case
could have its own function to implement it. In other words, would you
be OK with calling parse_disk_attrib, parse_disk_vdev_info and
parse_disk_pdev_info from the main switch under DSTATE_PHYSPATH,
DSTATE_VIRTPATH, etc?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] xl disk configuration handling
2011-02-01 11:13 ` Stefano Stabellini
@ 2011-02-01 14:00 ` Kamala Narasimhan
0 siblings, 0 replies; 7+ messages in thread
From: Kamala Narasimhan @ 2011-02-01 14:00 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel
>>> do we really need to change the parsing function that much? I
>>> understand there are significant changes but this is a total rewrite.
>>> I am concerned about all the bugs we might find later after the
>>> release...
>>>
>> This is one change I would really like to go with. Not only does it help with
>> the changes we needed, it also gets rid of code duplication. With this change
>> block-attach can rely on the same parsing code (that is once I submit the
>> block-attach changes patch).
>
> It took us several iterations to get the parsing right, I would like to
> keep the state machine and the field parsing as it is, but each case
> could have its own function to implement it. In other words, would you
> be OK with calling parse_disk_attrib, parse_disk_vdev_info and
> parse_disk_pdev_info from the main switch under DSTATE_PHYSPATH,
> DSTATE_VIRTPATH, etc?
Sure, I will go with appropriate DSTATE_* for each chunk we parse.
Kamala
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-02-01 14:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-30 17:46 [RFC] xl disk configuration handling Kamala Narasimhan
2011-01-31 17:18 ` Stefano Stabellini
2011-01-31 19:25 ` Ian Campbell
2011-01-31 20:39 ` Kamala Narasimhan
2011-01-31 20:19 ` Kamala Narasimhan
2011-02-01 11:13 ` Stefano Stabellini
2011-02-01 14:00 ` Kamala Narasimhan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.