On 04/13/2012 05:23 AM, Jiri Denemark wrote:
> On Mon, Apr 09, 2012 at 21:52:16 -0600, Eric Blake wrote:
>> QUESTION: should we parse and ignore <mirror> on input, rather than
>> rejecting it? By rejecting it, I can't add a unit test, since the
>> unit test framework currently doesn't expose a way to trigger
>> internal parsing.
>>
>> In order to track a block copy job across libvirtd restarts, we
>> need to save internal XML that tracks the name of the file
>> holding the mirror. Displaying this name in dumpxml might also
>> be useful to the user, even if we don't yet have a way to (re-)
>> start a domain with mirroring enabled up front. This is done
>> with a new <mirror> sub-element to <disk>, as in:
>>
>> <disk type='file' device='disk'>
>> <driver name='qemu' type='raw'/>
>> <source file='/var/lib/libvirt/images/original.img'/>
>> <mirror file='/var/lib/libvirt/images/copy.img' format='qcow2'/>
>> ...
>> </disk>
>
> However, this would mean, the XML from virDomainGetXMLDesc() cannot be
> directly used for defining new domain. I think there are two possible ways to
> go. Either output <mirror> the way you did it and ignore it on input
> (similarly to what we do with aliases or SELinux labels) or put the mirror
> element out of domain XML and put it into state XML only.
I like seeing the output (it makes it obvious that a mirror is still
running, and what --pivot will switch to). I'll redo this patch to
ignore rather than reject <mirror> on input for inactive domains, at
which point I can then add an xml2xml round-trip testcase (and remove
the QUESTION paragraph from my commit message).
>> @@ -3453,6 +3457,22 @@ virDomainDiskDefParseXML(virCapsPtr caps,
>> ioeventfd = virXMLPropString(cur, "ioeventfd");
>> event_idx = virXMLPropString(cur, "event_idx");
>> copy_on_read = virXMLPropString(cur, "copy_on_read");
>> + } else if ((mirror == NULL) &&
>> + (xmlStrEqual(cur->name, BAD_CAST "mirror"))) {
>
> You could have also removed those extra () when copy&pasting the code :-)
Copy-and-paste strikes again :)
>>
>> + char *mirror;
>> + char *mirrorFormat;
>> + bool mirroring;
>> +
>> virDomainBlockIoTuneInfo blkdeviotune;
>>
>> char *serial;
>> @@ -2125,6 +2129,7 @@ VIR_ENUM_DECL(virDomainDiskIo)
>> VIR_ENUM_DECL(virDomainDiskSecretType)
>> VIR_ENUM_DECL(virDomainDiskSnapshot)
>> VIR_ENUM_DECL(virDomainDiskTray)
>> +VIR_ENUM_DECL(virDomainDiskMirrorStage)
>> VIR_ENUM_DECL(virDomainIoEventFd)
>> VIR_ENUM_DECL(virDomainVirtioEventIdx)
>> VIR_ENUM_DECL(virDomainDiskCopyOnRead)
>
> This last hunk looks weired. Should it go into another patch perhaps?
Good catch. Junk left over from an earlier revision (back when qemu
didn't keep the job once in the mirroring stage, so I had to track the
job manually via additional xml state). I'll delete it for v5.
--
Eric Blake eblake@xxxxxxxxxx +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