On 03/29/14 23:20, Eric Blake wrote:
> Now that we have a common struct, it's time to start using it!
> Since external snapshots make a longer backing chain, it is
> only natural to use the same struct for the file created by
> the snapshot as what we use for <domain> disks.
>
> * src/conf/snapshot_conf.h (_virDomainSnapshotDiskDef): Use common
> struct instead of open-coded duplicate fields.
> * src/conf/snapshot_conf.c (virDomainSnapshotDiskDefClear)
> (virDomainSnapshotDiskDefParseXML, virDomainSnapshotAlignDisks)
> (virDomainSnapshotDiskDefFormat)
> (virDomainSnapshotDiskGetActualType): Adjust clients.
> * src/qemu/qemu_conf.c (qemuTranslateSnapshotDiskSourcePool):
> Likewise.
> * src/qemu/qemu_driver.c (qemuDomainSnapshotDiskGetSourceString)
> (qemuDomainSnapshotCreateInactiveExternal)
> (qemuDomainSnapshotPrepareDiskExternalOverlayActive)
> (qemuDomainSnapshotPrepareDiskExternal)
> (qemuDomainSnapshotPrepare)
> (qemuDomainSnapshotCreateSingleDiskActive): Likewise.
> * src/storage/storage_driver.c
> (virStorageFileInitFromSnapshotDef): Likewise.
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> src/conf/snapshot_conf.c | 68 +++++++++++++++++------------------
> src/conf/snapshot_conf.h | 8 ++---
> src/qemu/qemu_conf.c | 2 +-
> src/qemu/qemu_driver.c | 86 ++++++++++++++++++++++----------------------
> src/storage/storage_driver.c | 8 ++---
> 5 files changed, 85 insertions(+), 87 deletions(-)
>
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index ebb3bb4..3bd4732 100644
> @@ -632,16 +632,16 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf,
> virBufferAsprintf(buf, " type='%s'>\n", virStorageTypeToString(type));
> virBufferAdjustIndent(buf, 2);
>
> - if (disk->format > 0)
> + if (disk->src.format > 0)
> virBufferEscapeString(buf, "<driver type='%s'/>\n",
> - virStorageFileFormatTypeToString(disk->format));
> + virStorageFileFormatTypeToString(disk->src.format));
> virDomainDiskSourceDefFormatInternal(buf,
Hmm, now that we have a common struct, we can unify the source formatter
and have it accept the common struct instead of splitting out separate
fields ...
> type,
> - disk->file,
> + disk->src.path,
> 0,
> - disk->protocol,
> - disk->nhosts,
> - disk->hosts,
> + disk->src.protocol,
> + disk->src.nhosts,
> + disk->src.hosts,
> 0, NULL, NULL, 0);
>
> virBufferAdjustIndent(buf, -2);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9d48a46..9a2de12 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -12028,10 +12028,10 @@ qemuDomainSnapshotDiskGetSourceString(virDomainSnapshotDiskDefPtr disk,
> *source = NULL;
>
> return qemuGetDriveSourceString(virDomainSnapshotDiskGetActualType(disk),
same here. This function can now be refactored to take the struct
instead of the fields separately.
> - disk->file,
> - disk->protocol,
> - disk->nhosts,
> - disk->hosts,
> + disk->src.path,
> + disk->src.protocol,
> + disk->src.nhosts,
> + disk->src.hosts,
> NULL,
> NULL,
> source);
>
> @@ -12874,9 +12876,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
>
> disk->src.path = newsource;
> disk->src.format = format;
> - disk->src.type = snap->type;
> - disk->src.protocol = snap->protocol;
> - disk->src.nhosts = snap->nhosts;
> + disk->src.type = snap->src.type;
> + disk->src.protocol = snap->src.protocol;
> + disk->src.nhosts = snap->src.nhosts;
> disk->src.hosts = newhosts;
And here we can introduce a function to copy the source instead of doing
it manually.
>
> newsource = NULL;
> @@ -12889,9 +12891,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
>
> persistDisk->src.path = persistSource;
> persistDisk->src.format = format;
> - persistDisk->src.type = snap->type;
> - persistDisk->src.protocol = snap->protocol;
> - persistDisk->src.nhosts = snap->nhosts;
> + persistDisk->src.type = snap->src.type;
> + persistDisk->src.protocol = snap->src.protocol;
> + persistDisk->src.nhosts = snap->src.nhosts;
> persistDisk->src.hosts = persistHosts;
>
> persistSource = NULL;
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index cdb8536..6fca3d2 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -2802,10 +2802,10 @@ virStorageFilePtr
> virStorageFileInitFromSnapshotDef(virDomainSnapshotDiskDefPtr disk)
> {
> return virStorageFileInitInternal(virDomainSnapshotDiskGetActualType(disk),
> - disk->file,
> - disk->protocol,
> - disk->nhosts,
> - disk->hosts);
> + disk->src.path,
> + disk->src.protocol,
> + disk->src.nhosts,
> + disk->src.hosts);
> }
>
>
ACK to the patch. The proposed changes are definitely separable and
better to review separately.
Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list