* VMX status report. Xen: #17630 & Xen0: #540 -- blocked @ 2008-05-14 9:50 Li, Haicheng 2008-05-14 10:44 ` Ian Jackson 0 siblings, 1 reply; 13+ messages in thread From: Li, Haicheng @ 2008-05-14 9:50 UTC (permalink / raw) To: xen-devel Hi, all Today's nightly testing is still blocked for both PAE and 32E, c/s 17606 introduced this issue. Previous blocker issue (#1249) got fixed. New issue: [Bug 1250] HVM guest can not boot with Qcow image http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1250 Fixed issue: [Bug 1249] failed to create HVM guest on both 32E and PAE http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1249 -- haicheng ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: VMX status report. Xen: #17630 & Xen0: #540 -- blocked 2008-05-14 9:50 VMX status report. Xen: #17630 & Xen0: #540 -- blocked Li, Haicheng @ 2008-05-14 10:44 ` Ian Jackson 2008-05-15 7:40 ` Li, Haicheng 0 siblings, 1 reply; 13+ messages in thread From: Ian Jackson @ 2008-05-14 10:44 UTC (permalink / raw) To: Li, Haicheng; +Cc: xen-devel Li, Haicheng writes ("[Xen-devel] VMX status report. Xen: #17630 & Xen0: #540 -- blocked"): > Today's nightly testing is still blocked for both PAE and 32E, c/s 17606 > introduced this issue. ... > New issue: > [Bug 1250] HVM guest can not boot with Qcow image > http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1250 I'm afraid you are using the wrong syntax. file:... is for raw images. Prior to changeset 17606, the emulated IDE controller would, if the specified file looked like a qcow image, interpret it that way - but the PV presentation of the same device (via blktap) to the same guest would treat it as a raw image. This (and other related scenarios) allows a malicious guest to read any file on the host (including for example the host's underlying disk devices), because the qcow image format contains the (host) pathname of the backing file. In changeset 17606 the behaviour was changed so that file:... always treats the specified file as a raw disk image. To use a file containing a qcow image, you have to say tap:qcow:... This was discussed here on the list quite recently; feel free to comment further if you think the fix is the wrong one. Ian. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: VMX status report. Xen: #17630 & Xen0: #540 -- blocked 2008-05-14 10:44 ` Ian Jackson @ 2008-05-15 7:40 ` Li, Haicheng 2008-05-15 8:30 ` Xu, Dongxiao 0 siblings, 1 reply; 13+ messages in thread From: Li, Haicheng @ 2008-05-15 7:40 UTC (permalink / raw) To: Ian Jackson; +Cc: xen-devel Ian Jackson wrote: > Li, Haicheng writes ("[Xen-devel] VMX status report. Xen: #17630 & > Xen0: #540 -- blocked"): >> Today's nightly testing is still blocked for both PAE and 32E, c/s >> 17606 introduced this issue. > ... >> New issue: >> [Bug 1250] HVM guest can not boot with Qcow image >> http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1250 > > I'm afraid you are using the wrong syntax. > file:... is for raw images. > > Prior to changeset 17606, the emulated IDE controller would, if the > specified file looked like a qcow image, interpret it that way - but > the PV presentation of the same device (via blktap) to the same guest > would treat it as a raw image. This (and other related scenarios) > allows a malicious guest to read any file on the host (including for > example the host's underlying disk devices), because the qcow image > format contains the (host) pathname of the backing file. > > In changeset 17606 the behaviour was changed so that file:... always > treats the specified file as a raw disk image. To use a file > containing a qcow image, you have to say tap:qcow:... > > This was discussed here on the list quite recently; feel free to > comment further if you think the fix is the wrong one. > > Ian. Ian, even with syntax like "tap:qcow:" as following, it still doesn't work, guest bios shows the size of this disk is 0 MB. disk = [ 'tap:qcow:/mnt/sda5/hli22/xen-test/manual-test/xpsp2_smp_ia32.qcow,hda,w '] Qemu log is pasted here: domid: 5 qemu: the number of cpus is 4 Strip off blktap sub-type prefix to /mnt/sda5/hli22/xen-test/manual-test/xpsp2_smp_ia32.qcow (drv 'qcow') qemu: could not open vbd '/local/domain/5/device/vbd/768/phantom_vbd' or hard disk image '/mnt/sda5/hli22/xen-test/manual-test/xpsp2_smp_ia32.qcow' (drv 'qcow') Watching /local/domain/0/device-model/5/logdirty/next-active Watching /local/domain/0/device-model/5/command char device redirected to /dev/pts/5 qemu_map_cache_init nr_buckets = 4000 size 196608 shared page at pfn 3fffe buffered io page at pfn 3fffc Time offset set 0 Register xen platform. Done register platform. I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0 I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0 I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0 I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0 -- haicheng ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: VMX status report. Xen: #17630 & Xen0: #540 -- blocked 2008-05-15 7:40 ` Li, Haicheng @ 2008-05-15 8:30 ` Xu, Dongxiao 2008-05-15 9:00 ` Kevin Wolf 0 siblings, 1 reply; 13+ messages in thread From: Xu, Dongxiao @ 2008-05-15 8:30 UTC (permalink / raw) To: Li, Haicheng, Ian Jackson; +Cc: xen-devel I just looked into this issue, and I was confused by the recursive function call while opening a qcow file (bdrv_open2->qcow_open->bdrv_file_open->bdrv_open2->...). Remove the modification in block.c in C/S 17606 could work around this issue, but it is not a good way. Best regards, -- Dongxiao -----Original Message----- From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Li, Haicheng Sent: 2008年5月15日 15:40 To: Ian Jackson Cc: xen-devel@lists.xensource.com Subject: RE: [Xen-devel] VMX status report. Xen: #17630 & Xen0: #540 -- blocked Ian Jackson wrote: > Li, Haicheng writes ("[Xen-devel] VMX status report. Xen: #17630 & > Xen0: #540 -- blocked"): >> Today's nightly testing is still blocked for both PAE and 32E, c/s >> 17606 introduced this issue. > ... >> New issue: >> [Bug 1250] HVM guest can not boot with Qcow image >> http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1250 > > I'm afraid you are using the wrong syntax. > file:... is for raw images. > > Prior to changeset 17606, the emulated IDE controller would, if the > specified file looked like a qcow image, interpret it that way - but > the PV presentation of the same device (via blktap) to the same guest > would treat it as a raw image. This (and other related scenarios) > allows a malicious guest to read any file on the host (including for > example the host's underlying disk devices), because the qcow image > format contains the (host) pathname of the backing file. > > In changeset 17606 the behaviour was changed so that file:... always > treats the specified file as a raw disk image. To use a file > containing a qcow image, you have to say tap:qcow:... > > This was discussed here on the list quite recently; feel free to > comment further if you think the fix is the wrong one. > > Ian. Ian, even with syntax like "tap:qcow:" as following, it still doesn't work, guest bios shows the size of this disk is 0 MB. disk = [ 'tap:qcow:/mnt/sda5/hli22/xen-test/manual-test/xpsp2_smp_ia32.qcow,hda,w '] Qemu log is pasted here: domid: 5 qemu: the number of cpus is 4 Strip off blktap sub-type prefix to /mnt/sda5/hli22/xen-test/manual-test/xpsp2_smp_ia32.qcow (drv 'qcow') qemu: could not open vbd '/local/domain/5/device/vbd/768/phantom_vbd' or hard disk image '/mnt/sda5/hli22/xen-test/manual-test/xpsp2_smp_ia32.qcow' (drv 'qcow') Watching /local/domain/0/device-model/5/logdirty/next-active Watching /local/domain/0/device-model/5/command char device redirected to /dev/pts/5 qemu_map_cache_init nr_buckets = 4000 size 196608 shared page at pfn 3fffe buffered io page at pfn 3fffc Time offset set 0 Register xen platform. Done register platform. I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0 I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0 I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0 I/O request not ready: 0, ptr: 0, port: 0, data: 0, count: 0, size: 0 -- haicheng _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: VMX status report. Xen: #17630 & Xen0: #540 -- blocked 2008-05-15 8:30 ` Xu, Dongxiao @ 2008-05-15 9:00 ` Kevin Wolf 2008-05-15 9:31 ` Xu, Dongxiao 0 siblings, 1 reply; 13+ messages in thread From: Kevin Wolf @ 2008-05-15 9:00 UTC (permalink / raw) To: Xu, Dongxiao; +Cc: Li, Haicheng, xen-devel, Ian Jackson Xu, Dongxiao schrieb: > I just looked into this issue, and I was confused by the recursive function call while opening a qcow file (bdrv_open2->qcow_open->bdrv_file_open->bdrv_open2->...). > Remove the modification in block.c in C/S 17606 could work around this issue, but it is not a good way. I think the right solution is to remove bdrv_open altogether and force the callers to provide the driver as parameter to bdrv_open2. For qcow itself the following patch could be enough (compiles, but completely untested). But then, I don't even understand why find_protocol has been crippled - isn't this one harmless and we should rather have removed the actual guessing in find_image_format? Kevin diff -r 0beee5c839ea tools/ioemu/block.c --- a/tools/ioemu/block.c Tue May 13 12:17:08 2008 +++ b/tools/ioemu/block.c Thu May 15 11:51:15 2008 @@ -329,7 +329,7 @@ bs = bdrv_new(""); if (!bs) return -ENOMEM; - ret = bdrv_open2(bs, filename, flags | BDRV_O_FILE, NULL); + ret = bdrv_open2(bs, filename, flags | BDRV_O_FILE, &bdrv_raw); if (ret < 0) { bdrv_delete(bs); return ret; ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: VMX status report. Xen: #17630 & Xen0: #540 -- blocked 2008-05-15 9:00 ` Kevin Wolf @ 2008-05-15 9:31 ` Xu, Dongxiao 2008-05-15 9:50 ` Kevin Wolf 0 siblings, 1 reply; 13+ messages in thread From: Xu, Dongxiao @ 2008-05-15 9:31 UTC (permalink / raw) To: Kevin Wolf; +Cc: Li, Haicheng, xen-devel, Ian Jackson I just tried your patch, and seems that it doesn't work. Actually, find_protocol() could not find out the qcow protocol, and for qcow image, that function will always return NULL after C/S 17606. I think this is the root cause why qcow image could not be booted. -- Dongxiao -----Original Message----- From: Kevin Wolf [mailto:kwolf@suse.de] Sent: 2008年5月15日 17:01 To: Xu, Dongxiao Cc: Li, Haicheng; Ian Jackson; xen-devel@lists.xensource.com Subject: Re: [Xen-devel] VMX status report. Xen: #17630 & Xen0: #540 -- blocked Xu, Dongxiao schrieb: > I just looked into this issue, and I was confused by the recursive function call while opening a qcow file (bdrv_open2->qcow_open->bdrv_file_open->bdrv_open2->...). > Remove the modification in block.c in C/S 17606 could work around this issue, but it is not a good way. I think the right solution is to remove bdrv_open altogether and force the callers to provide the driver as parameter to bdrv_open2. For qcow itself the following patch could be enough (compiles, but completely untested). But then, I don't even understand why find_protocol has been crippled - isn't this one harmless and we should rather have removed the actual guessing in find_image_format? Kevin diff -r 0beee5c839ea tools/ioemu/block.c --- a/tools/ioemu/block.c Tue May 13 12:17:08 2008 +++ b/tools/ioemu/block.c Thu May 15 11:51:15 2008 @@ -329,7 +329,7 @@ bs = bdrv_new(""); if (!bs) return -ENOMEM; - ret = bdrv_open2(bs, filename, flags | BDRV_O_FILE, NULL); + ret = bdrv_open2(bs, filename, flags | BDRV_O_FILE, &bdrv_raw); if (ret < 0) { bdrv_delete(bs); return ret; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: VMX status report. Xen: #17630 & Xen0: #540 -- blocked 2008-05-15 9:31 ` Xu, Dongxiao @ 2008-05-15 9:50 ` Kevin Wolf 2008-05-15 11:07 ` Ian Jackson 0 siblings, 1 reply; 13+ messages in thread From: Kevin Wolf @ 2008-05-15 9:50 UTC (permalink / raw) To: Xu, Dongxiao; +Cc: Li, Haicheng, xen-devel, Ian Jackson Xu, Dongxiao schrieb: > I just tried your patch, and seems that it doesn't work. > Actually, find_protocol() could not find out the qcow protocol, and for qcow image, that function will always return NULL after C/S 17606. I think this is the root cause why qcow image could not be booted. find_protocol isn't even responsible for finding out that it's qcow. There is a difference between image format and protocol (IIRC, the latter is used in qemu for things like vvfat which maps a host directory to a virtual FAT floppy/harddisk). I suspect the problem arises from the bdrv_file_open call in find_image_format which won't correctly get &bdrv_raw but NULL as driver. And you're right, the patch can't work. I missed that in bdrv_open2 for BDRV_O_FILE drv is ignored and always overwritten. Kevin ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: VMX status report. Xen: #17630 & Xen0: #540 -- blocked 2008-05-15 9:50 ` Kevin Wolf @ 2008-05-15 11:07 ` Ian Jackson 2008-05-15 11:54 ` Kevin Wolf 2008-05-16 1:38 ` Xu, Dongxiao 0 siblings, 2 replies; 13+ messages in thread From: Ian Jackson @ 2008-05-15 11:07 UTC (permalink / raw) To: Kevin Wolf; +Cc: Li, Haicheng, Xu, Dongxiao, xen-devel [-- Attachment #1: message body text --] [-- Type: text/plain, Size: 127 bytes --] Could those of you having this problem please try the attached patch ? I have tested this one much more thoroughly :-/. Ian. [-- Attachment #2: ioemu should not try to guess backing file format (!) --] [-- Type: text/plain, Size: 3630 bytes --] diff -r 547d10d2d384 tools/ioemu/block.c --- a/tools/ioemu/block.c Wed May 14 09:52:25 2008 +0100 +++ b/tools/ioemu/block.c Thu May 15 12:03:29 2008 +0100 @@ -240,8 +240,28 @@ static int is_windows_drive(const char * } #endif +static int bdrv_invalid_protocol_open(BlockDriverState *bs, + const char *filename, int flags) { + return -ENOENT; +} + +static BlockDriver bdrv_invalid_protocol = { + "invalid_protocol", + .bdrv_open = bdrv_invalid_protocol_open, +}; + static BlockDriver *find_protocol(const char *filename) { + /* Return values: + * &bdrv_xxx + * filename specifies protocol xxx + * caller should use that + * NULL filename does not specify any protocol + * caller may apply their own default + * &bdrv_invalid_protocol filename speciies an unknown protocol + * caller should return -ENOENT; or may just try to open with + * that bdrv, which always fails that way. + */ BlockDriver *drv1; char protocol[128]; int len; @@ -254,7 +274,7 @@ static BlockDriver *find_protocol(const #endif p = strchr(filename, ':'); if (!p) - return NULL; /* do not ever guess raw, it is a security problem! */ + return NULL; len = p - filename; if (len > sizeof(protocol) - 1) len = sizeof(protocol) - 1; @@ -265,7 +285,7 @@ static BlockDriver *find_protocol(const !strcmp(drv1->protocol_name, protocol)) return drv1; } - return NULL; + return &bdrv_invalid_protocol; } /* XXX: force raw format if block or character device ? It would @@ -295,8 +315,8 @@ static BlockDriver *find_image_format(co #endif drv = find_protocol(filename); - /* no need to test disk image formats for vvfat */ - if (drv == &bdrv_vvfat) + /* no need to test disk image format if the filename told us */ + if (drv != NULL) return drv; ret = bdrv_file_open(&bs, filename, BDRV_O_RDONLY); @@ -390,7 +410,7 @@ int bdrv_open2(BlockDriverState *bs, con if (flags & BDRV_O_FILE) { drv = find_protocol(filename); if (!drv) - return -ENOENT; + drv = &bdrv_raw; } else { if (!drv) { drv = find_image_format(filename); @@ -438,7 +458,7 @@ int bdrv_open2(BlockDriverState *bs, con } path_combine(backing_filename, sizeof(backing_filename), filename, bs->backing_file); - if (bdrv_open(bs->backing_hd, backing_filename, 0) < 0) + if (bdrv_open2(bs->backing_hd, backing_filename, 0, &bdrv_raw) < 0) goto fail; } diff -r 547d10d2d384 tools/ioemu/xenstore.c --- a/tools/ioemu/xenstore.c Wed May 14 09:52:25 2008 +0100 +++ b/tools/ioemu/xenstore.c Thu May 15 12:04:12 2008 +0100 @@ -260,6 +260,8 @@ void xenstore_parse_domain_config(int hv /* autoguess qcow vs qcow2 */ } else if (!strcmp(drv,"file") || !strcmp(drv,"phy")) { format = &bdrv_raw; + } else if (!strcmp(drv,"phy")) { + format = &bdrv_raw; } else { format = bdrv_find_format(drv); if (!format) { @@ -269,7 +271,7 @@ void xenstore_parse_domain_config(int hv } } if (bdrv_open2(bs, params, 0 /* snapshot */, format) < 0) - fprintf(stderr, "qemu: could not open vbd '%s' or hard disk image '%s' (drv '%s')\n", buf, params, drv ? drv : "?"); + fprintf(stderr, "qemu: could not open vbd '%s' or hard disk image '%s' (drv '%s' format '%s')\n", buf, params, drv ? drv : "?", format ? format->format_name : "0"); } } [-- 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] 13+ messages in thread
* Re: VMX status report. Xen: #17630 & Xen0: #540 -- blocked 2008-05-15 11:07 ` Ian Jackson @ 2008-05-15 11:54 ` Kevin Wolf 2008-05-15 13:07 ` Ian Jackson 2008-05-16 1:38 ` Xu, Dongxiao 1 sibling, 1 reply; 13+ messages in thread From: Kevin Wolf @ 2008-05-15 11:54 UTC (permalink / raw) To: Ian Jackson; +Cc: Li, Haicheng, Xu, Dongxiao, xen-devel Ian Jackson schrieb: > Could those of you having this problem please try the attached patch ? > I have tested this one much more thoroughly :-/. It works for me, I don't like it though. Maybe I don't understand correctly what the intent of the whole protocol stuff was. But if I do, I think you're destroying this with your patch. This is not necessarily bad as we don't use it anyway. But then it would be much cleaner to remove the functionality altogether. To be a bit more concrete, I think the following change is wrong (even if there is no user anyway): - /* no need to test disk image formats for vvfat */ - if (drv == &bdrv_vvfat) + /* no need to test disk image format if the filename told us */ + if (drv != NULL) return drv; find_protocol doesn't tell you the image format of a file, it tells you a protocol through which you should obtain the image. And the image you get could be a qcow image then. I think the reason why they are using bdrv functions for all file accesses is that e.g. the qcow driver could use the protocol driver then (if there was any protocol driver...) Currently xend won't let you specify a filename with a protocol specifier anyway, so removing the whole thing is certainly an option. Kevin ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: VMX status report. Xen: #17630 & Xen0: #540 -- blocked 2008-05-15 11:54 ` Kevin Wolf @ 2008-05-15 13:07 ` Ian Jackson 2008-05-15 13:53 ` Kevin Wolf 0 siblings, 1 reply; 13+ messages in thread From: Ian Jackson @ 2008-05-15 13:07 UTC (permalink / raw) To: Kevin Wolf; +Cc: Li, Haicheng, Xu, Dongxiao, xen-devel Kevin Wolf writes ("Re: [Xen-devel] VMX status report. Xen: #17630 & Xen0: #540 -- blocked"): > Ian Jackson schrieb: > > Could those of you having this problem please try the attached patch ? > > I have tested this one much more thoroughly :-/. > > It works for me, I don't like it though. Maybe I don't understand > correctly what the intent of the whole protocol stuff was. But if I do, > I think you're destroying this with your patch. This is not necessarily > bad as we don't use it anyway. But then it would be much cleaner to > remove the functionality altogether. I think you've misunderstood, but I could be wrong. To me it seems that Qemu uses the terms `protocol' and `format' almost interchangeably - I don't think they refer to different things. find_protocol, find_format and find_image_format all select from the same set of bdrv_xxx drivers: find_format take a name and returns the driver with that name; find_protocol takes a filename and looks for a `:' in it, and if so it assumes that the part before the `:' is the name and finds the driver with that name. (Just for extra confusion the two namespaces are different and some drivers don't have a name that can be put in filenames, but some drivers have both names.) find_image_format reads the start of the file and passes it to drivers until one of them recognises it. > To be a bit more concrete, I think the following change is wrong (even > if there is no user anyway): > > - /* no need to test disk image formats for vvfat */ > - if (drv == &bdrv_vvfat) > + /* no need to test disk image format if the filename told us */ > + if (drv != NULL) > return drv; > > find_protocol doesn't tell you the image format of a file, it tells you > a protocol through which you should obtain the image. And the image you > get could be a qcow image then. These `protocols' aren't network protocols, they're disk image formats. The only ones which aren't a disk image format are vvfat and (in ioemu) vbd. vvfat is a crazy thing which you can't currently sanely layer anything on top of (even though you might want to) and for which layering a block driver underneath makes no sense. vbd is a mistake; it should be minios's block_raw. > I think the reason why they are using > bdrv functions for all file accesses is that e.g. the qcow driver could > use the protocol driver then (if there was any protocol driver...) The reason they're using bdrv functions is so that they can go through the bdrv_raw driver for backing file and image file accesses; this is necessary to preserve the properties of the block IO abstraction through the various layers. Ian. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: VMX status report. Xen: #17630 & Xen0: #540 -- blocked 2008-05-15 13:07 ` Ian Jackson @ 2008-05-15 13:53 ` Kevin Wolf 0 siblings, 0 replies; 13+ messages in thread From: Kevin Wolf @ 2008-05-15 13:53 UTC (permalink / raw) To: Ian Jackson; +Cc: Li, Haicheng, Xu, Dongxiao, xen-devel Ian Jackson schrieb: > Kevin Wolf writes ("Re: [Xen-devel] VMX status report. Xen: #17630 & Xen0: #540 -- blocked"): >> Ian Jackson schrieb: >> > Could those of you having this problem please try the attached patch ? >> > I have tested this one much more thoroughly :-/. >> >> It works for me, I don't like it though. Maybe I don't understand >> correctly what the intent of the whole protocol stuff was. But if I do, >> I think you're destroying this with your patch. This is not necessarily >> bad as we don't use it anyway. But then it would be much cleaner to >> remove the functionality altogether. > > I think you've misunderstood, but I could be wrong. To me it seems > that Qemu uses the terms `protocol' and `format' almost > interchangeably - I don't think they refer to different things. I certainly don't want to exclude that I've misunderstood. But if it was meant to select the image format, the code looks overcomplicated to me. And now that upstream qemu has a -drive parameter with a format option, we could get rid of this filename parsing in both ioemu and upstream qemu then. In Xen we don't use it anyway. > find_protocol, find_format and find_image_format all select from the > same set of bdrv_xxx drivers: find_format take a name and returns the > driver with that name; find_protocol takes a filename and looks for a > `:' in it, and if so it assumes that the part before the `:' is the > name and finds the driver with that name. (Just for extra confusion > the two namespaces are different and some drivers don't have a name > that can be put in filenames, but some drivers have both names.) > find_image_format reads the start of the file and passes it to drivers > until one of them recognises it. I think I understand quite well how it's processed in the code. ;-) And I also think that the code allows both of our understandings. If you wanted, I think you could add a "protocol" which in fact acts like a network protocol. And the different namespaces still don't make too much sense to me if formats and protocols are really meant to be the same... >> To be a bit more concrete, I think the following change is wrong (even >> if there is no user anyway): >> >> - /* no need to test disk image formats for vvfat */ >> - if (drv == &bdrv_vvfat) >> + /* no need to test disk image format if the filename told us */ >> + if (drv != NULL) >> return drv; >> >> find_protocol doesn't tell you the image format of a file, it tells you >> a protocol through which you should obtain the image. And the image you >> get could be a qcow image then. > > These `protocols' aren't network protocols, they're disk image > formats. The only ones which aren't a disk image format are vvfat and > (in ioemu) vbd. > > vvfat is a crazy thing which you can't currently sanely layer anything > on top of (even though you might want to) and for which layering > a block driver underneath makes no sense. I agree that vvfat isn't really a protocol as I understand them. It's only another block driver and it's using the protocol thing only because there was no format option qemu until recently. Maybe you're right and the "protocol" was only introduced as a hack to allow overriding the image format. Kevin ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: VMX status report. Xen: #17630 & Xen0: #540 -- blocked 2008-05-15 11:07 ` Ian Jackson 2008-05-15 11:54 ` Kevin Wolf @ 2008-05-16 1:38 ` Xu, Dongxiao 2008-05-16 9:14 ` Ian Jackson 1 sibling, 1 reply; 13+ messages in thread From: Xu, Dongxiao @ 2008-05-16 1:38 UTC (permalink / raw) To: Ian Jackson, Kevin Wolf; +Cc: Li, Haicheng, xen-devel Hi, Ian, In your new patch, if find_protocol() function returns NULL, then you will let drv = &bdrv_raw, it is just guessing the image to be raw, I remember you mentioned it as a security problem. So I don't find any difference as the following one, it just reverts the change in block.c in C/S 17606. diff -r 86587698116d tools/ioemu/block.c --- a/tools/ioemu/block.c Wed May 14 14:12:53 2008 +0100 +++ b/tools/ioemu/block.c Fri May 16 09:24:44 2008 +0800 @@ -254,7 +254,7 @@ static BlockDriver *find_protocol(const #endif p = strchr(filename, ':'); if (!p) - return NULL; /* do not ever guess raw, it is a security problem! */ + return &bdrv_raw; len = p - filename; if (len > sizeof(protocol) - 1) len = sizeof(protocol) - 1; -----Original Message----- From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com] Sent: 2008年5月15日 19:08 To: Kevin Wolf Cc: Xu, Dongxiao; Li, Haicheng; xen-devel@lists.xensource.com Subject: Re: [Xen-devel] VMX status report. Xen: #17630 & Xen0: #540 -- blocked Could those of you having this problem please try the attached patch ? I have tested this one much more thoroughly :-/. Ian. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: VMX status report. Xen: #17630 & Xen0: #540 -- blocked 2008-05-16 1:38 ` Xu, Dongxiao @ 2008-05-16 9:14 ` Ian Jackson 0 siblings, 0 replies; 13+ messages in thread From: Ian Jackson @ 2008-05-16 9:14 UTC (permalink / raw) To: Xu, Dongxiao; +Cc: Li, Haicheng, xen-devel, Ian Jackson, Kevin Wolf Xu, Dongxiao writes ("RE: [Xen-devel] VMX status report. Xen: #17630 & > Xen0: #540 -- blocked"): > In your new patch, if find_protocol() function returns NULL, > then you will let drv = &bdrv_raw, it is just guessing the image to > be raw, I remember you mentioned it as a security problem. So I > don't find any difference as the following one, it just reverts the > change in block.c in C/S 17606. Yes, I did revert that specific change; it's dealt with by find_protocol's caller, instead. I've also tested that tap:qcow:/path/to/raw/image.iso does not work. Ian. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2008-05-16 9:14 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-05-14 9:50 VMX status report. Xen: #17630 & Xen0: #540 -- blocked Li, Haicheng 2008-05-14 10:44 ` Ian Jackson 2008-05-15 7:40 ` Li, Haicheng 2008-05-15 8:30 ` Xu, Dongxiao 2008-05-15 9:00 ` Kevin Wolf 2008-05-15 9:31 ` Xu, Dongxiao 2008-05-15 9:50 ` Kevin Wolf 2008-05-15 11:07 ` Ian Jackson 2008-05-15 11:54 ` Kevin Wolf 2008-05-15 13:07 ` Ian Jackson 2008-05-15 13:53 ` Kevin Wolf 2008-05-16 1:38 ` Xu, Dongxiao 2008-05-16 9:14 ` Ian Jackson
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.