From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH 2/5] Xl interface change plus changes to code it impacts Date: Tue, 15 Feb 2011 19:22:12 +0000 Message-ID: <19802.53860.260105.696834@mariner.uk.xensource.com> References: <4D5060EB.3060109@gmail.com> <4D52DB1E.6080101@gmail.com> <4D541AC6.6040802@gmail.com> <4D5443E6.8010704@gmail.com> <4D555368.3010504@gmail.com> <4D558A02.6010106@gmail.com> <19801.27199.25918.814669@mariner.uk.xensource.com> <4D5987C6.4080807@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4D5987C6.4080807@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Kamala Narasimhan Cc: "xen-devel@lists.xensource.com" , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org Kamala Narasimhan writes ("Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts"): > Attached patch should address your concerns. Thanks for this. However, there were other changes made between this most recent version and the previous version, besides the ones I mentioned in my comments and which you said you'd address. When you post an updated version of a patch you should state separately - what the patch does to the tree, including intended changes, motivation, etc., as you have done (for the commit message) - how the patch differs from the previous version, if applicable Can you explain what the changes you made and why you made them ? Ideally in general you should use revision control system tools to make sure that you know what they all are. But, for your info here is the output of diff -w --exclude=\*{~,.o,.d,.opic} -ru A B |grep -v '^Only in' Most of this is formatting noise, which addresses my comments. However, you have also: * Initialised disk->format and disk->backend somewhere you previously didn't * Recognised the "tap2" prefix in a place you previously didn't * Changed the handling of the "aio" and "raw" prefixes Normally patch such as this one, which is presented as being mature, ought not to need unexplained semantic changes at this late stage. Ian. diff -w --exclude='*~' --exclude='*.o' --exclude='*.d' --exclude='*.opic' -ru tools/libxl/libxl.c /u/iwj/work/xen-unstable-tools.hg/tools/libxl/libxl.c --- tools/libxl/libxl.c 2011-02-15 19:13:12.000000000 +0000 +++ /u/iwj/work/xen-unstable-tools.hg/tools/libxl/libxl.c 2011-02-15 19:12:48.000000000 +0000 @@ -931,7 +931,7 @@ device.kind = DEVICE_VBD; switch (disk->backend) { - case DISK_BACKEND_PHY: { + case DISK_BACKEND_PHY: 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)); @@ -941,9 +941,8 @@ device.backend_kind = DEVICE_VBD; break; - } case DISK_BACKEND_TAP: - case DISK_BACKEND_QDISK: { + case DISK_BACKEND_QDISK: if (disk->format == DISK_FORMAT_EMPTY) break; if (libxl__blktap_enabled(&gc)) { @@ -972,12 +971,11 @@ libxl__device_disk_string_of_format(disk->format), disk->pdev_path)); if (libxl__blktap_enabled(&gc) && - disk->format != DISK_BACKEND_QDISK) + disk->backend != DISK_BACKEND_QDISK) device.backend_kind = DEVICE_TAP; else device.backend_kind = DEVICE_QDISK; break; - } default: LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n", disk->backend); rc = ERROR_INVAL; @@ -1053,7 +1051,7 @@ char *ret = NULL; switch (disk->backend) { - case DISK_BACKEND_PHY: { + case DISK_BACKEND_PHY: if (disk->format != DISK_FORMAT_RAW) { LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "physical block device must" " be raw"); @@ -1063,8 +1061,7 @@ disk->pdev_path); dev = disk->pdev_path; break; - } - case DISK_BACKEND_TAP: { + case DISK_BACKEND_TAP: if (disk->format == DISK_FORMAT_VHD || disk->format == DISK_FORMAT_RAW) { if (libxl__blktap_enabled(&gc)) @@ -1091,8 +1088,7 @@ "type: %d", disk->backend); break; } - } - case DISK_BACKEND_QDISK: { + case DISK_BACKEND_QDISK: if (disk->format != DISK_FORMAT_RAW) { LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally attach a qdisk " "image if the format is not raw"); @@ -1102,15 +1098,12 @@ disk->pdev_path); dev = disk->pdev_path; break; - - } case DISK_BACKEND_UNKNOWN: - default: { + default: LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend " "type: %d", disk->backend); break; } - } if (dev != NULL) ret = strdup(dev); diff -w --exclude='*~' --exclude='*.o' --exclude='*.d' --exclude='*.opic' -ru tools/libxl/xl_cmdimpl.c /u/iwj/work/xen-unstable-tools.hg/tools/libxl/xl_cmdimpl.c --- tools/libxl/xl_cmdimpl.c 2011-02-15 19:13:12.000000000 +0000 +++ /u/iwj/work/xen-unstable-tools.hg/tools/libxl/xl_cmdimpl.c 2011-02-15 19:12:48.000000000 +0000 @@ -452,6 +452,8 @@ char *p, *end, *tok; memset(disk, 0, sizeof(*disk)); + disk->format = DISK_FORMAT_RAW; + disk->backend = DISK_BACKEND_TAP; for(tok = p = buf2, end = buf2 + strlen(buf2) + 1; p < end; p++) { switch(state){ @@ -466,7 +468,8 @@ state = DSTATE_PHYSPATH; disk->format = DISK_FORMAT_RAW; disk->backend = DISK_BACKEND_TAP; - }else if (!strcmp(tok, "tap")) { + }else if ((!strcmp(tok, "tap")) || + (!strcmp(tok, "tap2"))) { state = DSTATE_TAP; }else{ fprintf(stderr, "Unknown disk type: %s\n", tok); @@ -485,9 +488,10 @@ if ( *p == ':' ) { *p = '\0'; if (!strcmp(tok, "aio")) { - disk->format = DISK_FORMAT_RAW; - disk->backend = DISK_BACKEND_TAP; - }else if (!strcmp(tok, "vhd")) { + tok = p + 1; + break; + } + if (!strcmp(tok, "vhd")) { disk->format = DISK_FORMAT_VHD; disk->backend = DISK_BACKEND_TAP; }else if (!strcmp(tok, "qcow")) { @@ -496,7 +500,11 @@ }else if (!strcmp(tok, "qcow2")) { disk->format = DISK_FORMAT_QCOW2; disk->backend = DISK_BACKEND_QDISK; - }else { + }else if (!strcmp(tok, "raw")) { + disk->format = DISK_FORMAT_RAW; + disk->backend = DISK_BACKEND_TAP; + } + else { fprintf(stderr, "Unknown tapdisk type: %s\n", tok); return 0; }