On 12/16/2014 04:33 PM, Eric Blake wrote:
> On 12/16/2014 06:54 AM, Peter Krempa wrote:
>> On 12/16/14 09:04, Eric Blake wrote:
>>> We want to avoid read()ing a file while qemu is running. We still
>>> have to open() block devices to determine their physical size, but
>>> that is safer. This patch rearranges code to make it easier to
>>> skip the metadata collection when possible.
>>>
>>> * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Check for empty
>>> disk up front. Perform stat first. Place metadata reading next
>>> to use.
>>>
>
>>> + if (stat(disk->src->path, &sb) < 0) {
>>> + virReportSystemError(errno,
>>> + _("cannot stat file '%s'"), disk->src->path);
>>> goto endjob;
>>> }
>>
>> Um this will cause problems on NFS. The code after that that you are
>> moving uses the FD to do the stat() call. The fd is opened by the helper
>> that opens the file with correct perms.
>
> Oh, you're right. I'll see how hairy it is to pull this patch out of
> the current series, and save the rework of avoiding open()ing the files
> to later (I may end up with a situation that is less efficient, by
> possibly open()ing some files twice for offline domains, once for
> capacity, and once for determining the last offset of a block device,
> instead of the current code that tries to do both from a single fd but
> becomes harder to untangle). 1-4 are pushed, and I'm seeing what
> happens to the rest of the series if I leave this out for now...
>
>> Otherwise looks okay.
>>
>> Peter
>
Turned out to be a little bit hairy; what I ended up pushing was JUST
the refactoring for earlier checking for empty images and later grouping
of read-related code, leaving the open-code untouched for later:
diff --git c/src/qemu/qemu_driver.c i/src/qemu/qemu_driver.c
index 75ea218..3d81a5b 100644
--- c/src/qemu/qemu_driver.c
+++ i/src/qemu/qemu_driver.c
@@ -11057,6 +11057,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
disk = vm->def->disks[idx];
src = disk->src;
+ if (virStorageSourceIsEmpty(src)) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("disk '%s' does not currently have a source
assigned"),
+ path);
+ goto endjob;
+ }
/* FIXME: For an offline domain, we always want to check current
* on-disk statistics (as users have been known to change offline
@@ -11079,13 +11085,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
* change.
*/
if (virStorageSourceIsLocalStorage(src)) {
- if (!src->path) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("disk '%s' does not currently have a
source assigned"),
- path);
- goto endjob;
- }
-
if ((fd = qemuOpenFile(driver, vm, src->path, O_RDONLY,
NULL, NULL)) == -1)
goto endjob;
@@ -11116,26 +11115,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
}
}
- /* Probe for magic formats */
- if (virDomainDiskGetFormat(disk)) {
- format = virDomainDiskGetFormat(disk);
- } else {
- if (!cfg->allowDiskFormatProbing) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("no disk format for %s and probing is
disabled"),
- path);
- goto endjob;
- }
-
- if ((format = virStorageFileProbeFormatFromBuf(src->path,
- buf, len)) < 0)
- goto endjob;
- }
-
- if (!(meta = virStorageFileGetMetadataFromBuf(src->path, buf, len,
- format, NULL)))
- goto endjob;
-
/* Get info for normal formats */
if (S_ISREG(sb.st_mode) || fd == -1) {
#ifndef WIN32
@@ -11164,6 +11143,21 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
/* If the file we probed has a capacity set, then override
* what we calculated from file/block extents */
+ if (!(format = src->format)) {
+ if (!cfg->allowDiskFormatProbing) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("no disk format for %s and probing is
disabled"),
+ path);
+ goto endjob;
+ }
+
+ if ((format = virStorageFileProbeFormatFromBuf(src->path,
+ buf, len)) < 0)
+ goto endjob;
+ }
+ if (!(meta = virStorageFileGetMetadataFromBuf(src->path, buf, len,
+ format, NULL)))
+ goto endjob;
if (meta->capacity)
src->capacity = meta->capacity;
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list