[libvirt] [PATCH v2 0/7] optionally expose disk usage during dumpxml

v1 was here: https://www.redhat.com/archives/libvir-list/2014-September/msg01077.html This adds the framework for exposing disk of all disks from a single dumpxml command. I'm still working on a followup patch to further list usage of disks in the backing chain, rather than just the top-level disk, but want to get review started on this code. Eric Blake (7): dumpxml: add flag to virDomainGetXMLDesc qemu: refactor dumpxml job handling qemu: refactor blockinfo job handling dumpxml: let blockinfo reuse virStorageSource qemu: refactor blockinfo data gathering dumpxml: query disk limits as needed for qemu dumpxml: add output of block info docs/schemas/domaincommon.rng | 22 +++ include/libvirt/libvirt-domain.h | 1 + src/conf/domain_conf.c | 16 +- src/libvirt-domain-snapshot.c | 5 +- src/libvirt-domain.c | 10 +- src/qemu/qemu_driver.c | 397 ++++++++++++++++++++++----------------- src/util/virstoragefile.h | 3 +- tools/virsh-domain.c | 6 + tools/virsh.pod | 6 +- 9 files changed, 284 insertions(+), 182 deletions(-) -- 1.9.3

The information in virDomainGetBlockInfo() is important for clients that use qcow2 format on LVM block devices - by tracking the allocation in relation to the physical size, management can tell if a disk needs to be expanded before the guest (file system contents) and/or qemu (copy-on-write differing more from a backing file) runs out of space. Normally, only the active layer matters, but during a block commit operation, the allocation of the backing file ALSO grows, and management would like to track that growth. Right now, virDomainGetBlockInfo() can only convey information about the active layer of a disk, via a single API call per disk. It can also be easily extended to support "vda[1]" notation that we recently added for blockcommit and friends, to get similar information about a backing element; but that still implies one call per layer, which adds up to a lot of overhead. This API addition will make it possible to grab this information for ALL guest disks in a single call, by letting the user request additional information about each disk in the backing chain to be output as part of the domain XML. My ultimate goal is to have this flag and virStorageVolGetXMLDesc() expose the same bits of information about a given storage volume (there are slight incompatiblities in the XML naming that we'll have to keep for back-compat sake, but the idea is to get all the information out there). * include/libvirt/libvirt.h.in (virDomainXMLFlags): Add new flag. * src/libvirt.c (virDomainGetXMLDesc): Document it. (virDomainSaveImageGetXMLDesc, virDomainSnapshotGetXMLDesc): For now, mention the flag won't help here. * tools/virsh-domain.c (cmdDumpXML): Add new flag. * tools/virsh.pod (dumpxml): Document it. Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt-domain.h | 1 + src/libvirt-domain-snapshot.c | 5 +++-- src/libvirt-domain.c | 10 ++++++++-- tools/virsh-domain.c | 6 ++++++ tools/virsh.pod | 6 ++++-- 5 files changed, 22 insertions(+), 6 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 1fac2a3..caf1f46 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1248,6 +1248,7 @@ typedef enum { VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */ VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */ VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for migration */ + VIR_DOMAIN_XML_BLOCK_INFO = (1 << 4), /* include storage volume information about each disk source */ } virDomainXMLFlags; char * virDomainGetXMLDesc (virDomainPtr domain, diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 9feb669..f12af45 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -269,8 +269,9 @@ virDomainSnapshotCreateXML(virDomainPtr domain, * * No security-sensitive data will be included unless @flags contains * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only - * connections. For this API, @flags should not contain either - * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU. + * connections. For this API, @flags should not contain + * VIR_DOMAIN_XML_INACTIVE, VIR_DOMAIN_XML_UPDATE_CPU, or + * VIR_DOMAIN_XML_BLOCK_INFO. * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 2b0defc..2ef1a51 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -1086,8 +1086,9 @@ virDomainRestoreFlags(virConnectPtr conn, const char *from, const char *dxml, * * No security-sensitive data will be included unless @flags contains * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only - * connections. For this API, @flags should not contain either - * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU. + * connections. For this API, @flags should not contain + * VIR_DOMAIN_XML_INACTIVE, VIR_DOMAIN_XML_UPDATE_CPU, or + * VIR_DOMAIN_XML_BLOCK_INFO. * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of * error. The caller must free() the returned value. @@ -2592,6 +2593,11 @@ virDomainGetControlInfo(virDomainPtr domain, * describing CPU capabilities is modified to match actual * capabilities of the host. * + * If @flags contains VIR_DOMAIN_XML_BLOCK_INFO, the listing for each + * <disk> device will contain additional information such as capacity + * and allocation, similar to what is displayed by virStorageVolGetXMLDesc(), + * and avoiding the need to call virDomainGetBlockInfo() separately. + * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. */ diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a7e9151..4c63006 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8984,6 +8984,10 @@ static const vshCmdOptDef opts_dumpxml[] = { .type = VSH_OT_BOOL, .help = N_("provide XML suitable for migrations") }, + {.name = "block-info", + .type = VSH_OT_BOOL, + .help = N_("include additional block info for each <disk> in XML dump") + }, {.name = NULL} }; @@ -9007,6 +9011,8 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_XML_UPDATE_CPU; if (migratable) flags |= VIR_DOMAIN_XML_MIGRATABLE; + if (vshCommandOptBool(cmd, "block-info")) + flags |= VIR_DOMAIN_XML_BLOCK_INFO; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; diff --git a/tools/virsh.pod b/tools/virsh.pod index d5608cc..28f928f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1290,7 +1290,7 @@ NOTE: Some hypervisors may require the user to manually ensure proper permissions on file and path specified by argument I<corefilepath>. =item B<dumpxml> I<domain> [I<--inactive>] [I<--security-info>] -[I<--update-cpu>] [I<--migratable>] +[I<--update-cpu>] [I<--migratable>] [I<--block-info>] Output the domain information as an XML dump to stdout, this format can be used by the B<create> command. Additional options affecting the XML dump may be @@ -1301,7 +1301,9 @@ in the XML dump. I<--update-cpu> updates domain CPU requirements according to host CPU. With I<--migratable> one can request an XML that is suitable for migrations, i.e., compatible with older libvirt releases and possibly amended with internal run-time options. This option may automatically enable other -options (I<--update-cpu>, I<--security-info>, ...) as necessary. +options (I<--update-cpu>, I<--security-info>, ...) as necessary. Using +I<--block-info> will add additional information about each <disk> source, +comparable to what B<vol-dumpxml> or B<domblkinfo> would show. =item B<edit> I<domain> -- 1.9.3

On 18/11/14 06:31 -0700, Eric Blake wrote:
The information in virDomainGetBlockInfo() is important for clients that use qcow2 format on LVM block devices - by tracking the allocation in relation to the physical size, management can tell if a disk needs to be expanded before the guest (file system contents) and/or qemu (copy-on-write differing more from a backing file) runs out of space. Normally, only the active layer matters, but during a block commit operation, the allocation of the backing file ALSO grows, and management would like to track that growth.
Right now, virDomainGetBlockInfo() can only convey information about the active layer of a disk, via a single API call per disk. It can also be easily extended to support "vda[1]" notation that we recently added for blockcommit and friends, to get similar information about a backing element; but that still implies one call per layer, which adds up to a lot of overhead.
This API addition will make it possible to grab this information for ALL guest disks in a single call, by letting the user request additional information about each disk in the backing chain to be output as part of the domain XML. My ultimate goal is to have this flag and virStorageVolGetXMLDesc() expose the same bits of information about a given storage volume (there are slight incompatiblities in the XML naming that we'll have to keep for back-compat sake, but the idea is to get all the information out there).
* include/libvirt/libvirt.h.in (virDomainXMLFlags): Add new flag. * src/libvirt.c (virDomainGetXMLDesc): Document it. (virDomainSaveImageGetXMLDesc, virDomainSnapshotGetXMLDesc): For now, mention the flag won't help here. * tools/virsh-domain.c (cmdDumpXML): Add new flag. * tools/virsh.pod (dumpxml): Document it.
Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt-domain.h | 1 + src/libvirt-domain-snapshot.c | 5 +++-- src/libvirt-domain.c | 10 ++++++++-- tools/virsh-domain.c | 6 ++++++ tools/virsh.pod | 6 ++++-- 5 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 1fac2a3..caf1f46 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1248,6 +1248,7 @@ typedef enum { VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */ VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */ VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for migration */ + VIR_DOMAIN_XML_BLOCK_INFO = (1 << 4), /* include storage volume information about each disk source */
I'll admit I'm not a master API designer but this is a red flag for me (pun most definitely intended). Up to this point, the individual virDomainXMLFlags are used to ask for XML for a certain purpose. For example, VIR_DOMAIN_XML_INACTIVE retrieves XML suitable for a call to virDomainCreateXML whereas VIR_DOMAIN_XML_MIGRATABLE is used to "dump XML suitable for migration". This new flag is somewhat arbitrarily slicing up some of the ACTIVE VM information. Slippery slope logic would lead us to a future API with a proliferation of flags that turn various bits of info on and off which would be very cumbersome to maintain and use. Since this information is really ACTIVE VM info, I'd prefer it to be provided whenever VIR_DOMAIN_XML_INACTIVE is not set. Callers who want a tight XML document can always use VIR_DOMAIN_XML_INACTIVE.
} virDomainXMLFlags;
char * virDomainGetXMLDesc (virDomainPtr domain, diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 9feb669..f12af45 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -269,8 +269,9 @@ virDomainSnapshotCreateXML(virDomainPtr domain, * * No security-sensitive data will be included unless @flags contains * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only - * connections. For this API, @flags should not contain either - * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU. + * connections. For this API, @flags should not contain + * VIR_DOMAIN_XML_INACTIVE, VIR_DOMAIN_XML_UPDATE_CPU, or + * VIR_DOMAIN_XML_BLOCK_INFO. * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 2b0defc..2ef1a51 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -1086,8 +1086,9 @@ virDomainRestoreFlags(virConnectPtr conn, const char *from, const char *dxml, * * No security-sensitive data will be included unless @flags contains * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only - * connections. For this API, @flags should not contain either - * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU. + * connections. For this API, @flags should not contain + * VIR_DOMAIN_XML_INACTIVE, VIR_DOMAIN_XML_UPDATE_CPU, or + * VIR_DOMAIN_XML_BLOCK_INFO. * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of * error. The caller must free() the returned value. @@ -2592,6 +2593,11 @@ virDomainGetControlInfo(virDomainPtr domain, * describing CPU capabilities is modified to match actual * capabilities of the host. * + * If @flags contains VIR_DOMAIN_XML_BLOCK_INFO, the listing for each + * <disk> device will contain additional information such as capacity + * and allocation, similar to what is displayed by virStorageVolGetXMLDesc(), + * and avoiding the need to call virDomainGetBlockInfo() separately. + * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. */ diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a7e9151..4c63006 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8984,6 +8984,10 @@ static const vshCmdOptDef opts_dumpxml[] = { .type = VSH_OT_BOOL, .help = N_("provide XML suitable for migrations") }, + {.name = "block-info", + .type = VSH_OT_BOOL, + .help = N_("include additional block info for each <disk> in XML dump") + }, {.name = NULL} };
@@ -9007,6 +9011,8 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_XML_UPDATE_CPU; if (migratable) flags |= VIR_DOMAIN_XML_MIGRATABLE; + if (vshCommandOptBool(cmd, "block-info")) + flags |= VIR_DOMAIN_XML_BLOCK_INFO;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; diff --git a/tools/virsh.pod b/tools/virsh.pod index d5608cc..28f928f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1290,7 +1290,7 @@ NOTE: Some hypervisors may require the user to manually ensure proper permissions on file and path specified by argument I<corefilepath>.
=item B<dumpxml> I<domain> [I<--inactive>] [I<--security-info>] -[I<--update-cpu>] [I<--migratable>] +[I<--update-cpu>] [I<--migratable>] [I<--block-info>]
Output the domain information as an XML dump to stdout, this format can be used by the B<create> command. Additional options affecting the XML dump may be @@ -1301,7 +1301,9 @@ in the XML dump. I<--update-cpu> updates domain CPU requirements according to host CPU. With I<--migratable> one can request an XML that is suitable for migrations, i.e., compatible with older libvirt releases and possibly amended with internal run-time options. This option may automatically enable other -options (I<--update-cpu>, I<--security-info>, ...) as necessary. +options (I<--update-cpu>, I<--security-info>, ...) as necessary. Using +I<--block-info> will add additional information about each <disk> source, +comparable to what B<vol-dumpxml> or B<domblkinfo> would show.
=item B<edit> I<domain>
-- 1.9.3
-- Adam Litke

On 11/18/2014 07:50 AM, Adam Litke wrote:
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 1fac2a3..caf1f46 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1248,6 +1248,7 @@ typedef enum { VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */ VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */ VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for migration */ + VIR_DOMAIN_XML_BLOCK_INFO = (1 << 4), /* include storage volume information about each disk source */
I'll admit I'm not a master API designer but this is a red flag for me (pun most definitely intended). Up to this point, the individual virDomainXMLFlags are used to ask for XML for a certain purpose. For example, VIR_DOMAIN_XML_INACTIVE retrieves XML suitable for a call to virDomainCreateXML whereas VIR_DOMAIN_XML_MIGRATABLE is used to "dump XML suitable for migration".
Not quite. The VIR_DOMAIN_XML_SECURE and VIR_DOMAIN_XML_UPDATE_CPU flags both have the behavior of dumping additional information that is omitted by default. And the VIR_DOMAIN_XML_INACTIVE flag really means 'dump the xml that will be used on the next boot' if the domain is persistent (if the domain is transient, it says 'dump the xml that would be used on the next boot if I make the domain persistent').
This new flag is somewhat arbitrarily slicing up some of the ACTIVE VM information. Slippery slope logic would lead us to a future API with a proliferation of flags that turn various bits of info on and off which would be very cumbersome to maintain and use. Since this information is really ACTIVE VM info, I'd prefer it to be provided whenever VIR_DOMAIN_XML_INACTIVE is not set. Callers who want a tight XML document can always use VIR_DOMAIN_XML_INACTIVE.
The thing is that this new flag is providing additional informative information that is NOT being stored as part of the domain itself, but which is being queried from the disks each time the xml is gathered. What's more, the information is valid for both active and inactive xml (it is not an active-only flag), and gathering the information potentially requires a monitor command for an active domain (none of the other XML information requires a monitor command). It is not good to require a monitor command by default, which is why I felt that having the flag opt-in to the behavior is the best way to preserve back-compat for callers that don't need the extra information. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 18/11/14 09:33 -0700, Eric Blake wrote:
On 11/18/2014 07:50 AM, Adam Litke wrote:
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 1fac2a3..caf1f46 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1248,6 +1248,7 @@ typedef enum { VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */ VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */ VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for migration */ + VIR_DOMAIN_XML_BLOCK_INFO = (1 << 4), /* include storage volume information about each disk source */
I'll admit I'm not a master API designer but this is a red flag for me (pun most definitely intended). Up to this point, the individual virDomainXMLFlags are used to ask for XML for a certain purpose. For example, VIR_DOMAIN_XML_INACTIVE retrieves XML suitable for a call to virDomainCreateXML whereas VIR_DOMAIN_XML_MIGRATABLE is used to "dump XML suitable for migration".
Not quite. The VIR_DOMAIN_XML_SECURE and VIR_DOMAIN_XML_UPDATE_CPU flags both have the behavior of dumping additional information that is omitted by default. And the VIR_DOMAIN_XML_INACTIVE flag really means 'dump the xml that will be used on the next boot' if the domain is persistent (if the domain is transient, it says 'dump the xml that would be used on the next boot if I make the domain persistent').
This new flag is somewhat arbitrarily slicing up some of the ACTIVE VM information. Slippery slope logic would lead us to a future API with a proliferation of flags that turn various bits of info on and off which would be very cumbersome to maintain and use. Since this information is really ACTIVE VM info, I'd prefer it to be provided whenever VIR_DOMAIN_XML_INACTIVE is not set. Callers who want a tight XML document can always use VIR_DOMAIN_XML_INACTIVE.
The thing is that this new flag is providing additional informative information that is NOT being stored as part of the domain itself, but which is being queried from the disks each time the xml is gathered. What's more, the information is valid for both active and inactive xml (it is not an active-only flag), and gathering the information potentially requires a monitor command for an active domain (none of the other XML information requires a monitor command). It is not good to require a monitor command by default, which is why I felt that having the flag opt-in to the behavior is the best way to preserve back-compat for callers that don't need the extra information.
Then perhaps the right flag to add would be VIR_DOMAIN_XML_QUERY or some such to indicate that extra data needs to be retrieved via potentially expensive means. Then, the flag (and its semantics) could be reused in the future. -- Adam Litke

In order for a future patch to actually implement the new VIR_DOMAIN_XML_BLOCK_INFO flag, we'll need to make a qemu monitor command call for each disk backed by block storage. Not only does a monitor command require a job, but even iterating over disks to determine if the monitor command is needed requires a job (so no other command is hot-plugging disks in the meantime). Previously, we needed a job if we were talking to old qemu that lacked balloon events, and just silently ignored that efforts if a long-running job was already in effect; but since the new API is an explicit flag, we aren't grabbing the job unless a user requests it, and the user request gives us reason to wait for the earlier job to finish. Ideally, this rewrite should have no semantic changes for callers that do not pass the new flag. * src/qemu/qemu_driver.c (qemuDomainGetXMLDesc): Rearrange job control, in order to make future patch for grabbing block info easier to manage. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 73 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 44 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a84fd47..9b77b00 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6188,6 +6188,8 @@ static char *qemuDomainGetXMLDesc(virDomainPtr dom, unsigned long long balloon; int err = 0; qemuDomainObjPrivatePtr priv; + bool need_job = false; + bool need_balloon = false; /* Flags checked by virDomainDefFormat */ @@ -6199,38 +6201,47 @@ static char *qemuDomainGetXMLDesc(virDomainPtr dom, if (virDomainGetXMLDescEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; + /* Need a job if we will talk to the monitor */ + if (virDomainObjIsActive(vm)) { + if (flags & VIR_DOMAIN_XML_BLOCK_INFO) + need_job = true; + if (vm->def->memballoon && + vm->def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_NONE && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT)) { + /* If we already need a job, then grab balloon. But if + * balloon is the only reason to query, don't delay if + * someone's using the monitor; existing most recent data + * is good enough. */ + need_job |= qemuDomainJobAllowed(priv, QEMU_JOB_QUERY); + need_balloon = need_job; + } + } + if (need_job) { + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + } + /* Refresh current memory based on balloon info if supported */ - if ((vm->def->memballoon != NULL) && - (vm->def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_NONE) && - !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT) && - (virDomainObjIsActive(vm))) { - /* Don't delay if someone's using the monitor, just use - * existing most recent data instead */ - if (qemuDomainJobAllowed(priv, QEMU_JOB_QUERY)) { - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) - goto cleanup; + if (need_balloon) { + qemuDomainObjEnterMonitor(driver, vm); + err = qemuMonitorGetBalloonInfo(priv->mon, &balloon); + qemuDomainObjExitMonitor(driver, vm); - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto endjob; - } + if (err < 0) + goto endjob; + if (err > 0) + vm->def->mem.cur_balloon = balloon; + /* err == 0 indicates no balloon support, so ignore it */ + } - qemuDomainObjEnterMonitor(driver, vm); - err = qemuMonitorGetBalloonInfo(priv->mon, &balloon); - qemuDomainObjExitMonitor(driver, vm); - - endjob: - if (!qemuDomainObjEndJob(driver, vm)) { - vm = NULL; - goto cleanup; - } - if (err < 0) - goto cleanup; - if (err > 0) - vm->def->mem.cur_balloon = balloon; - /* err == 0 indicates no balloon support, so ignore it */ - } + if (flags & VIR_DOMAIN_XML_BLOCK_INFO) { + /* FIXME run monitor commands to refresh block info */ } if ((flags & VIR_DOMAIN_XML_MIGRATABLE)) @@ -6238,6 +6249,10 @@ static char *qemuDomainGetXMLDesc(virDomainPtr dom, ret = qemuDomainFormatXML(driver, vm, flags); + endjob: + if (need_job && !qemuDomainObjEndJob(driver, vm)) + vm = NULL; + cleanup: if (vm) virObjectUnlock(vm); -- 1.9.3

In order for a future patch to actually implement the new VIR_DOMAIN_XML_BLOCK_INFO flag, we want to reuse the code for grabbing block information, including the use of a monitor command. But grabbing a job multiple times when getting XML is not wise, so it is easier to reuse the query code if it can always assume the job is already owned. For that to work, this patch refactors the job handling to have a larger scope when getting information on a single block. This patch results in grabbing a job in cases where one was not previously needed, but as it is a query job, it should not be noticeably slower. This patch touches the same code as the fix for CVE-2014-6458 (commit b799259); in that patch, we avoided hotplug changing a disk reference during the time of obtaining a monitor lock by copying all data we needed and no longer referencing disk; this patch goes the other way and ensures that by holding the job, the disk cannot be changed so we no longer need to worry about the disk being invalidated across the monitor lock. * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Rearrange job control to be outside of disk information, in order to make future patch for reusing disk information easier to manage. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 62 +++++++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9b77b00..cd70fde 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10950,7 +10950,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom, int format; int activeFail = false; virQEMUDriverConfigPtr cfg = NULL; - char *alias = NULL; char *buf = NULL; ssize_t len; @@ -10969,11 +10968,19 @@ qemuDomainGetBlockInfo(virDomainPtr dom, goto cleanup; } + /* Technically, we only need a job if we are going to query the + * monitor, which is only for active domains that are using + * non-raw block devices. But it is easier to share code if we + * always grab a job; furthermore, grabbing the job ensures that + * hot-plug won't change disk behind our backs. */ + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + /* Check the path belongs to this domain. */ if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) { virReportError(VIR_ERR_INVALID_ARG, _("invalid path %s not assigned to domain"), path); - goto cleanup; + goto endjob; } disk = vm->def->disks[idx]; @@ -10983,36 +10990,36 @@ qemuDomainGetBlockInfo(virDomainPtr dom, virReportError(VIR_ERR_INVALID_ARG, _("disk '%s' does not currently have a source assigned"), path); - goto cleanup; + goto endjob; } if ((fd = qemuOpenFile(driver, vm, disk->src->path, O_RDONLY, NULL, NULL)) == -1) - goto cleanup; + goto endjob; if (fstat(fd, &sb) < 0) { virReportSystemError(errno, _("cannot stat file '%s'"), disk->src->path); - goto cleanup; + goto endjob; } if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) { virReportSystemError(errno, _("cannot read header '%s'"), disk->src->path); - goto cleanup; + goto endjob; } } else { if (virStorageFileInitAs(disk->src, cfg->user, cfg->group) < 0) - goto cleanup; + goto endjob; if ((len = virStorageFileReadHeader(disk->src, VIR_STORAGE_MAX_HEADER, &buf)) < 0) - goto cleanup; + goto endjob; if (virStorageFileStat(disk->src, &sb) < 0) { virReportSystemError(errno, _("failed to stat remote file '%s'"), NULLSTR(disk->src->path)); - goto cleanup; + goto endjob; } } @@ -11024,17 +11031,17 @@ qemuDomainGetBlockInfo(virDomainPtr dom, virReportError(VIR_ERR_INTERNAL_ERROR, _("no disk format for %s and probing is disabled"), path); - goto cleanup; + goto endjob; } if ((format = virStorageFileProbeFormatFromBuf(disk->src->path, buf, len)) < 0) - goto cleanup; + goto endjob; } if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len, format, NULL))) - goto cleanup; + goto endjob; /* Get info for normal formats */ if (S_ISREG(sb.st_mode) || fd == -1) { @@ -11056,7 +11063,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom, if (end == (off_t)-1) { virReportSystemError(errno, _("failed to seek to end of %s"), path); - goto cleanup; + goto endjob; } info->physical = end; info->capacity = end; @@ -11084,35 +11091,24 @@ qemuDomainGetBlockInfo(virDomainPtr dom, if (!virDomainObjIsActive(vm)) { activeFail = true; ret = 0; - goto cleanup; + goto endjob; } - if (VIR_STRDUP(alias, disk->info.alias) < 0) - goto cleanup; + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorGetBlockExtent(priv->mon, + disk->info.alias, + &info->allocation); + qemuDomainObjExitMonitor(driver, vm); - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) - goto cleanup; - - if (virDomainObjIsActive(vm)) { - qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorGetBlockExtent(priv->mon, - alias, - &info->allocation); - qemuDomainObjExitMonitor(driver, vm); - } else { - activeFail = true; - ret = 0; - } - - if (!qemuDomainObjEndJob(driver, vm)) - vm = NULL; } else { ret = 0; } + endjob: + if (!qemuDomainObjEndJob(driver, vm)) + vm = NULL; cleanup: VIR_FREE(buf); - VIR_FREE(alias); virStorageSourceFree(meta); VIR_FORCE_CLOSE(fd); if (disk) -- 1.9.3

In a future patch, the implementation of VIR_DOMAIN_XML_BLOCK_INFO will use information stored in virStorageSource. In order to maximize code reuse, it is easiest if BlockInfo code also uses the same location for information. * src/util/virstoragefile.h (_virStorageSource): Add physical, to mirror virDomainBlockInfo. * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Store into storage source, then copy to block info. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 22 ++++++++++++++-------- src/util/virstoragefile.h | 3 ++- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cd70fde..aa24658 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11046,15 +11046,15 @@ qemuDomainGetBlockInfo(virDomainPtr dom, /* Get info for normal formats */ if (S_ISREG(sb.st_mode) || fd == -1) { #ifndef WIN32 - info->physical = (unsigned long long)sb.st_blocks * + disk->src->physical = (unsigned long long)sb.st_blocks * (unsigned long long)DEV_BSIZE; #else - info->physical = sb.st_size; + disk->src->physical = sb.st_size; #endif /* Regular files may be sparse, so logical size (capacity) is not same * as actual physical above */ - info->capacity = sb.st_size; + disk->src->capacity = sb.st_size; } else { /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should * be 64 bits on all platforms. @@ -11065,17 +11065,17 @@ qemuDomainGetBlockInfo(virDomainPtr dom, _("failed to seek to end of %s"), path); goto endjob; } - info->physical = end; - info->capacity = end; + disk->src->physical = end; + disk->src->capacity = end; } /* If the file we probed has a capacity set, then override * what we calculated from file/block extents */ if (meta->capacity) - info->capacity = meta->capacity; + disk->src->capacity = meta->capacity; /* Set default value .. */ - info->allocation = info->physical; + disk->src->allocation = disk->src->physical; /* ..but if guest is not using raw disk format and on a block device, * then query highest allocated extent from QEMU @@ -11097,13 +11097,19 @@ qemuDomainGetBlockInfo(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetBlockExtent(priv->mon, disk->info.alias, - &info->allocation); + &disk->src->allocation); qemuDomainObjExitMonitor(driver, vm); } else { ret = 0; } + if (ret == 0) { + info->capacity = disk->src->capacity; + info->allocation = disk->src->allocation; + info->physical = disk->src->physical; + } + endjob: if (!qemuDomainObjEndJob(driver, vm)) vm = NULL; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 2583e10..681e50a 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -252,8 +252,9 @@ struct _virStorageSource { virStoragePermsPtr perms; virStorageTimestampsPtr timestamps; - unsigned long long allocation; /* in bytes, 0 if unknown */ unsigned long long capacity; /* in bytes, 0 if unknown */ + unsigned long long allocation; /* in bytes, 0 if unknown */ + unsigned long long physical; /* in bytes, 0 if unknown */ size_t nseclabels; virSecurityDeviceLabelDefPtr *seclabels; -- 1.9.3

On 18/11/14 06:31 -0700, Eric Blake wrote:
In a future patch, the implementation of VIR_DOMAIN_XML_BLOCK_INFO will use information stored in virStorageSource. In order to maximize code reuse, it is easiest if BlockInfo code also uses the same location for information.
* src/util/virstoragefile.h (_virStorageSource): Add physical, to mirror virDomainBlockInfo. * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Store into storage source, then copy to block info.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 22 ++++++++++++++-------- src/util/virstoragefile.h | 3 ++- 2 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cd70fde..aa24658 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11046,15 +11046,15 @@ qemuDomainGetBlockInfo(virDomainPtr dom, /* Get info for normal formats */ if (S_ISREG(sb.st_mode) || fd == -1) { #ifndef WIN32 - info->physical = (unsigned long long)sb.st_blocks * + disk->src->physical = (unsigned long long)sb.st_blocks * (unsigned long long)DEV_BSIZE; #else - info->physical = sb.st_size; + disk->src->physical = sb.st_size; #endif /* Regular files may be sparse, so logical size (capacity) is not same * as actual physical above */ - info->capacity = sb.st_size; + disk->src->capacity = sb.st_size; } else { /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should * be 64 bits on all platforms. @@ -11065,17 +11065,17 @@ qemuDomainGetBlockInfo(virDomainPtr dom, _("failed to seek to end of %s"), path); goto endjob; } - info->physical = end; - info->capacity = end; + disk->src->physical = end; + disk->src->capacity = end; }
/* If the file we probed has a capacity set, then override * what we calculated from file/block extents */ if (meta->capacity) - info->capacity = meta->capacity; + disk->src->capacity = meta->capacity;
/* Set default value .. */ - info->allocation = info->physical; + disk->src->allocation = disk->src->physical;
/* ..but if guest is not using raw disk format and on a block device, * then query highest allocated extent from QEMU @@ -11097,13 +11097,19 @@ qemuDomainGetBlockInfo(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetBlockExtent(priv->mon, disk->info.alias, - &info->allocation); + &disk->src->allocation); qemuDomainObjExitMonitor(driver, vm);
} else { ret = 0; }
+ if (ret == 0) { + info->capacity = disk->src->capacity; + info->allocation = disk->src->allocation; + info->physical = disk->src->physical; + } + endjob: if (!qemuDomainObjEndJob(driver, vm)) vm = NULL; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 2583e10..681e50a 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -252,8 +252,9 @@ struct _virStorageSource {
virStoragePermsPtr perms; virStorageTimestampsPtr timestamps; - unsigned long long allocation; /* in bytes, 0 if unknown */ unsigned long long capacity; /* in bytes, 0 if unknown */ + unsigned long long allocation; /* in bytes, 0 if unknown */
(Pedantic and superficial) allocation dropped and re-added. Maybe you prefer it to appear after capacity in light of physical now being present?
+ unsigned long long physical; /* in bytes, 0 if unknown */ size_t nseclabels; virSecurityDeviceLabelDefPtr *seclabels;
-- 1.9.3
-- Adam Litke

On 11/18/2014 08:00 AM, Adam Litke wrote:
On 18/11/14 06:31 -0700, Eric Blake wrote:
In a future patch, the implementation of VIR_DOMAIN_XML_BLOCK_INFO will use information stored in virStorageSource. In order to maximize code reuse, it is easiest if BlockInfo code also uses the same location for information.
+++ b/src/util/virstoragefile.h @@ -252,8 +252,9 @@ struct _virStorageSource {
virStoragePermsPtr perms; virStorageTimestampsPtr timestamps; - unsigned long long allocation; /* in bytes, 0 if unknown */ unsigned long long capacity; /* in bytes, 0 if unknown */ + unsigned long long allocation; /* in bytes, 0 if unknown */
(Pedantic and superficial) allocation dropped and re-added. Maybe you prefer it to appear after capacity in light of physical now being present?
Just reordering fields so that they come in the same order as their public struct virDomainBlockInfo counterpart in libvirt-domain.h. Order doesn't strictly matter, if you want me to avoid the churn. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 18/11/14 09:35 -0700, Eric Blake wrote:
On 11/18/2014 08:00 AM, Adam Litke wrote:
On 18/11/14 06:31 -0700, Eric Blake wrote:
In a future patch, the implementation of VIR_DOMAIN_XML_BLOCK_INFO will use information stored in virStorageSource. In order to maximize code reuse, it is easiest if BlockInfo code also uses the same location for information.
+++ b/src/util/virstoragefile.h @@ -252,8 +252,9 @@ struct _virStorageSource {
virStoragePermsPtr perms; virStorageTimestampsPtr timestamps; - unsigned long long allocation; /* in bytes, 0 if unknown */ unsigned long long capacity; /* in bytes, 0 if unknown */ + unsigned long long allocation; /* in bytes, 0 if unknown */
(Pedantic and superficial) allocation dropped and re-added. Maybe you prefer it to appear after capacity in light of physical now being present?
Just reordering fields so that they come in the same order as their public struct virDomainBlockInfo counterpart in libvirt-domain.h.
I suspected there was a good reason for it :)
Order doesn't strictly matter, if you want me to avoid the churn.
Nope, please keep it the way you have it. -- Adam Litke

Create a helper function that can be reused to implement the new VIR_DOMAIN_XML_BLOCK_INFO flag. * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Split guts... (qemuStorageLimitsRefresh): ...into new helper function. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 287 ++++++++++++++++++++++++++----------------------- 1 file changed, 151 insertions(+), 136 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aa24658..7b1431b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6179,6 +6179,151 @@ qemuDomainObjRestore(virConnectPtr conn, } +/* Refresh the capacity and allocation limits of a given storage + * source. Assumes that the caller has already obtained a domain job. + * Set *activeFail to true if data cannot be obtained because a + * transient guest is no longer active. */ +static int +qemuStorageLimitsRefresh(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg, + virDomainObjPtr vm, virDomainDiskDefPtr disk, + const char *path, bool *activeFail) +{ + int ret = -1; + int fd = -1; + off_t end; + virStorageSourcePtr meta = NULL; + struct stat sb; + int format; + char *buf = NULL; + ssize_t len; + + if (virStorageSourceIsLocalStorage(disk->src)) { + if (!disk->src->path) { + virReportError(VIR_ERR_INVALID_ARG, + _("disk '%s' does not currently have a source assigned"), + path); + goto cleanup; + } + + if ((fd = qemuOpenFile(driver, vm, disk->src->path, O_RDONLY, + NULL, NULL)) == -1) + goto cleanup; + + if (fstat(fd, &sb) < 0) { + virReportSystemError(errno, + _("cannot stat file '%s'"), disk->src->path); + goto cleanup; + } + + if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) { + virReportSystemError(errno, _("cannot read header '%s'"), + disk->src->path); + goto cleanup; + } + } else { + if (virStorageFileInitAs(disk->src, cfg->user, cfg->group) < 0) + goto cleanup; + + if ((len = virStorageFileReadHeader(disk->src, VIR_STORAGE_MAX_HEADER, + &buf)) < 0) + goto cleanup; + + if (virStorageFileStat(disk->src, &sb) < 0) { + virReportSystemError(errno, _("failed to stat remote file '%s'"), + NULLSTR(disk->src->path)); + goto cleanup; + } + } + + /* 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 cleanup; + } + + if ((format = virStorageFileProbeFormatFromBuf(disk->src->path, + buf, len)) < 0) + goto cleanup; + } + + if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len, + format, NULL))) + goto cleanup; + + /* Get info for normal formats */ + if (S_ISREG(sb.st_mode) || fd == -1) { +#ifndef WIN32 + disk->src->physical = (unsigned long long)sb.st_blocks * + (unsigned long long)DEV_BSIZE; +#else + disk->src->physical = sb.st_size; +#endif + /* Regular files may be sparse, so logical size (capacity) is not same + * as actual physical above + */ + disk->src->capacity = sb.st_size; + } else { + /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should + * be 64 bits on all platforms. + */ + end = lseek(fd, 0, SEEK_END); + if (end == (off_t)-1) { + virReportSystemError(errno, + _("failed to seek to end of %s"), path); + goto cleanup; + } + disk->src->physical = end; + disk->src->capacity = end; + } + + /* If the file we probed has a capacity set, then override + * what we calculated from file/block extents */ + if (meta->capacity) + disk->src->capacity = meta->capacity; + + /* Set default value .. */ + disk->src->allocation = disk->src->physical; + + /* ..but if guest is not using raw disk format and on a block device, + * then query highest allocated extent from QEMU + */ + if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK && + format != VIR_STORAGE_FILE_RAW && + S_ISBLK(sb.st_mode)) { + qemuDomainObjPrivatePtr priv = vm->privateData; + + /* If the guest is not running, then success/failure return + * depends on whether domain is persistent + */ + if (!virDomainObjIsActive(vm)) { + *activeFail = true; + ret = 0; + goto cleanup; + } + + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorGetBlockExtent(priv->mon, + disk->info.alias, + &disk->src->allocation); + qemuDomainObjExitMonitor(driver, vm); + + } else { + ret = 0; + } + cleanup: + VIR_FREE(buf); + virStorageSourceFree(meta); + VIR_FORCE_CLOSE(fd); + virStorageFileDeinit(disk->src); + return ret; +} + + static char *qemuDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { @@ -10941,17 +11086,10 @@ qemuDomainGetBlockInfo(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; - int fd = -1; - off_t end; - virStorageSourcePtr meta = NULL; virDomainDiskDefPtr disk = NULL; - struct stat sb; int idx; - int format; - int activeFail = false; + bool activeFail = false; virQEMUDriverConfigPtr cfg = NULL; - char *buf = NULL; - ssize_t len; virCheckFlags(0, -1); @@ -10985,141 +11123,18 @@ qemuDomainGetBlockInfo(virDomainPtr dom, disk = vm->def->disks[idx]; - if (virStorageSourceIsLocalStorage(disk->src)) { - if (!disk->src->path) { - virReportError(VIR_ERR_INVALID_ARG, - _("disk '%s' does not currently have a source assigned"), - path); - goto endjob; - } - - if ((fd = qemuOpenFile(driver, vm, disk->src->path, O_RDONLY, - NULL, NULL)) == -1) - goto endjob; - - if (fstat(fd, &sb) < 0) { - virReportSystemError(errno, - _("cannot stat file '%s'"), disk->src->path); - goto endjob; - } - - if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) { - virReportSystemError(errno, _("cannot read header '%s'"), - disk->src->path); - goto endjob; - } - } else { - if (virStorageFileInitAs(disk->src, cfg->user, cfg->group) < 0) - goto endjob; - - if ((len = virStorageFileReadHeader(disk->src, VIR_STORAGE_MAX_HEADER, - &buf)) < 0) - goto endjob; - - if (virStorageFileStat(disk->src, &sb) < 0) { - virReportSystemError(errno, _("failed to stat remote file '%s'"), - NULLSTR(disk->src->path)); - goto endjob; - } - } - - /* 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(disk->src->path, - buf, len)) < 0) - goto endjob; - } - - if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len, - format, NULL))) + if ((ret = qemuStorageLimitsRefresh(driver, cfg, vm, disk, path, + &activeFail)) < 0) goto endjob; - /* Get info for normal formats */ - if (S_ISREG(sb.st_mode) || fd == -1) { -#ifndef WIN32 - disk->src->physical = (unsigned long long)sb.st_blocks * - (unsigned long long)DEV_BSIZE; -#else - disk->src->physical = sb.st_size; -#endif - /* Regular files may be sparse, so logical size (capacity) is not same - * as actual physical above - */ - disk->src->capacity = sb.st_size; - } else { - /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should - * be 64 bits on all platforms. - */ - end = lseek(fd, 0, SEEK_END); - if (end == (off_t)-1) { - virReportSystemError(errno, - _("failed to seek to end of %s"), path); - goto endjob; - } - disk->src->physical = end; - disk->src->capacity = end; - } - - /* If the file we probed has a capacity set, then override - * what we calculated from file/block extents */ - if (meta->capacity) - disk->src->capacity = meta->capacity; - - /* Set default value .. */ - disk->src->allocation = disk->src->physical; - - /* ..but if guest is not using raw disk format and on a block device, - * then query highest allocated extent from QEMU - */ - if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK && - format != VIR_STORAGE_FILE_RAW && - S_ISBLK(sb.st_mode)) { - qemuDomainObjPrivatePtr priv = vm->privateData; - - /* If the guest is not running, then success/failure return - * depends on whether domain is persistent - */ - if (!virDomainObjIsActive(vm)) { - activeFail = true; - ret = 0; - goto endjob; - } - - qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorGetBlockExtent(priv->mon, - disk->info.alias, - &disk->src->allocation); - qemuDomainObjExitMonitor(driver, vm); - - } else { - ret = 0; - } - - if (ret == 0) { - info->capacity = disk->src->capacity; - info->allocation = disk->src->allocation; - info->physical = disk->src->physical; - } + info->capacity = disk->src->capacity; + info->allocation = disk->src->allocation; + info->physical = disk->src->physical; endjob: if (!qemuDomainObjEndJob(driver, vm)) vm = NULL; cleanup: - VIR_FREE(buf); - virStorageSourceFree(meta); - VIR_FORCE_CLOSE(fd); - if (disk) - virStorageFileDeinit(disk->src); - /* If we failed to get data from a domain because it's inactive and * it's not a persistent domain, then force failure. */ -- 1.9.3

Thanks to the recent refactoring, it is now possible to easily update the virStorageSource associated with each disk, including making a monitor call if necessary. Of course, making one monitor call per disk is not ideal, but that can be improved in later patches. * src/qemu/qemu_driver.c (qemuStorageLimitsRefresh): Minor tweaks to allow NULL arguments. (qemuDomainGetXMLDesc): Get stats on all disks. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7b1431b..657cd91 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6197,11 +6197,17 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg, char *buf = NULL; ssize_t len; + if (!path) + path = disk->dst; if (virStorageSourceIsLocalStorage(disk->src)) { if (!disk->src->path) { - virReportError(VIR_ERR_INVALID_ARG, - _("disk '%s' does not currently have a source assigned"), - path); + if (activeFail) + virReportError(VIR_ERR_INVALID_ARG, + _("disk '%s' does not currently have a " + "source assigned"), + path); + else + ret = 0; goto cleanup; } @@ -6301,7 +6307,8 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg, * depends on whether domain is persistent */ if (!virDomainObjIsActive(vm)) { - *activeFail = true; + if (activeFail) + *activeFail = true; ret = 0; goto cleanup; } @@ -6386,7 +6393,17 @@ static char *qemuDomainGetXMLDesc(virDomainPtr dom, } if (flags & VIR_DOMAIN_XML_BLOCK_INFO) { - /* FIXME run monitor commands to refresh block info */ + size_t i; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + + for (i = 0; i < vm->def->ndisks; i++) { + if (qemuStorageLimitsRefresh(driver, cfg, vm, vm->def->disks[i], + NULL, NULL) < 0) { + virObjectUnref(cfg); + goto endjob; + } + } + virObjectUnref(cfg); } if ((flags & VIR_DOMAIN_XML_MIGRATABLE)) -- 1.9.3

This patch adds the common code for outputting drive sizing information, when a user has requested the new API flag. The output covers the data stashed into virStorageSource of each disk of the domain. * docs/schemas/domaincommon.rng (storageSourceExtra): New define. * src/conf/domain_conf.c (virDomainDiskDefFormat): Output sizing when flag is set. (DUMPXML_FLAGS): Add new flag. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/schemas/domaincommon.rng | 22 ++++++++++++++++++++++ src/conf/domain_conf.c | 16 +++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 6863ec6..2d551a9 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1280,6 +1280,28 @@ </element> </define> + <define name='storageSourceExtra' combine='choice'> + <!-- Override of storagecommon.rng to allow domain disks to list + extras without impacting snapshot disks --> + <interleave> + <optional> + <element name="capacity"> + <ref name="scaledInteger"/> + </element> + </optional> + <optional> + <element name="allocation"> + <ref name="scaledInteger"/> + </element> + </optional> + <optional> + <element name="physical"> + <ref name="scaledInteger"/> + </element> + </optional> + </interleave> + </define> + <define name="diskBackingChain"> <choice> <ref name="diskBackingStore"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5f4b9f6..41ad8cf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -108,7 +108,8 @@ typedef enum { (VIR_DOMAIN_XML_SECURE | \ VIR_DOMAIN_XML_INACTIVE | \ VIR_DOMAIN_XML_UPDATE_CPU | \ - VIR_DOMAIN_XML_MIGRATABLE) + VIR_DOMAIN_XML_MIGRATABLE | \ + VIR_DOMAIN_XML_BLOCK_INFO) verify(((VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | @@ -16344,6 +16345,19 @@ virDomainDiskDefFormat(virBufferPtr buf, flags) < 0) return -1; + if (flags & VIR_DOMAIN_XML_BLOCK_INFO) { + if (def->src->capacity) + virBufferAsprintf(buf, "<capacity unit='bytes'>%llu</capacity>\n", + def->src->capacity); + if (def->src->allocation) + virBufferAsprintf(buf, + "<allocation unit='bytes'>%llu</allocation>\n", + def->src->allocation); + if (def->src->physical) + virBufferAsprintf(buf, "<physical unit='bytes'>%llu</physical>\n", + def->src->physical); + } + /* Don't format backingStore to inactive XMLs until the code for * persistent storage of backing chains is ready. */ if (!(flags & VIR_DOMAIN_XML_INACTIVE) && -- 1.9.3

On Tue, Nov 18, 2014 at 06:31:46AM -0700, Eric Blake wrote:
v1 was here: https://www.redhat.com/archives/libvir-list/2014-September/msg01077.html
This adds the framework for exposing disk of all disks from a single dumpxml command. I'm still working on a followup patch to further list usage of disks in the backing chain, rather than just the top-level disk, but want to get review started on this code.
I'm not really convinced that exposing disk allocation/capacity information in the XML is appropriate. The XML is intended as a representation of the guest hardware API and the host configuration. The disk allocation/capacity info is neither of those things - it is a statistic about the current state of the system. THis is something that we'd usually report via dedicated APIs not the XML. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Tue, Nov 18, 2014 at 04:38:49PM +0000, Daniel P. Berrange wrote:
On Tue, Nov 18, 2014 at 06:31:46AM -0700, Eric Blake wrote:
v1 was here: https://www.redhat.com/archives/libvir-list/2014-September/msg01077.html
This adds the framework for exposing disk of all disks from a single dumpxml command. I'm still working on a followup patch to further list usage of disks in the backing chain, rather than just the top-level disk, but want to get review started on this code.
I'm not really convinced that exposing disk allocation/capacity information in the XML is appropriate. The XML is intended as a representation of the guest hardware API and the host configuration.
The disk allocation/capacity info is neither of those things - it is a statistic about the current state of the system. THis is something that we'd usually report via dedicated APIs not the XML.
eg, I'd expect this data to be reported by either virDomainListGetStats or virDomainBlockStatsFlags, not in the XML document. I think this would be better for programs wanting to use this data too. They are going to want to poll on this information reasonably frequently, so it makes no sense to incur the overhead of formatting & parsing the entire domain XML which will not be changing, just to get the disk stats which will be changing. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 11/18/2014 09:42 AM, Daniel P. Berrange wrote:
On Tue, Nov 18, 2014 at 04:38:49PM +0000, Daniel P. Berrange wrote:
On Tue, Nov 18, 2014 at 06:31:46AM -0700, Eric Blake wrote:
v1 was here: https://www.redhat.com/archives/libvir-list/2014-September/msg01077.html
This adds the framework for exposing disk of all disks from a single dumpxml command. I'm still working on a followup patch to further list usage of disks in the backing chain, rather than just the top-level disk, but want to get review started on this code.
I'm not really convinced that exposing disk allocation/capacity information in the XML is appropriate. The XML is intended as a representation of the guest hardware API and the host configuration.
The disk allocation/capacity info is neither of those things - it is a statistic about the current state of the system. THis is something that we'd usually report via dedicated APIs not the XML.
eg, I'd expect this data to be reported by either virDomainListGetStats or virDomainBlockStatsFlags, not in the XML document. I think this would be better for programs wanting to use this data too. They are going to want to poll on this information reasonably frequently, so it makes no sense to incur the overhead of formatting & parsing the entire domain XML which will not be changing, just to get the disk stats which will be changing.
Okay, I'll post a v3 that uses virDomainListGetStats as the mechanism for exposing the information. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Adam Litke
-
Daniel P. Berrange
-
Eric Blake