7:30 a.m.
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