Re: [libvirt] [PATCHv1.5 16/27] snapshot: conf: Use common parsing and formatting functions for source

On 26.11.2013 17:48, Peter Krempa wrote: > Disk source elements for snapshots were using separate code from our > config parser. As snapshots can be stored on more than just regular > files, we will need the universal parser to allow us to expose a variety > of snapshot disk targets. This patch reuses the config parsers and > formatters to do the job. > > This initial support only changes the code without any visible XML > change. > --- > src/conf/snapshot_conf.c | 70 +++++++++++++++++++++++++++++++++--------------- > src/conf/snapshot_conf.h | 1 + > 2 files changed, 49 insertions(+), 22 deletions(-) > > diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c > index 94a74d2..7258daa 100644 > --- a/src/conf/snapshot_conf.c > +++ b/src/conf/snapshot_conf.c

> @@ -584,6 +601,13 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf, > if (disk->snapshot > 0) > virBufferAsprintf(buf, " snapshot='%s'", > virDomainSnapshotLocationTypeToString(disk->snapshot)); > + > + if (type < 0) > + type = VIR_DOMAIN_DISK_TYPE_FILE; > + else > + virBufferAsprintf(buf, " type='%s'", > + virDomainDiskTypeToString(type)); > + This is a very thin line between no XML change and outputting a new 'type' attribute into XML. Doesn't make much sense now, but I see this is to be changed in 25/27. Anyway, I think it would be better to save these small snippets up till then.

> if (!disk->file && disk->format == 0) { > virBufferAddLit(buf, "/>\n"); > return;

> ACK to the rest.