Re: [libvirt] [PATCH v2 4/6] tests: utils: Add PreFormat callback for CompareXML2XML helper
On Tue, Feb 09, 2016 at 03:28:59PM -0500, Cole Robinson wrote:
On 02/09/2016 03:13 PM, Laine Stump wrote:
> On 02/09/2016 10:59 AM, Cole Robinson wrote:
>> This allows individual driver tests to hook in their own code before
>> the def is formatted and compared.
>>
>> We will eventually use this in the qemuxml2xml
>> ---
>
> Oops. I ACKed 5/6 before this one (after looking ahead to see how this would
> be used).
>
> This looks okay, although I'm wondering why you call just the PCI address
> assignment rather than a full postparse callback. Other things can happen in
> there, e.g. auto-adding controllers. We may as well test the whole thing.
>
PostParse is automatically called by DomainDefParse (I said DomainDefFormat in
a previously mail, that's incorrect), so we don't need to call it explicitly.
The qemu specific controller additions are triggered via that.
The generic domain_conf AddImplicitControllers though isn't tested here, but I
have previously posted patches that move that call into the generic PostParse
function, so it will be tested after that. I'll be reposting those as part of
another series after this is merged.
The other series also puts AssignAddresses into qemu's PostParse callback, so
the immediate need for this patch will be gone. But there are other bits like
AssignAliases we should be testing here as well
Yes, there are still more things that need to be done, especially if
we're going to check active XML. However the problem of our codebase is
that we don't have dedicated places to modify:
a) persistent definition before starting the domain (adding defaults,
controllers, whatever) -- this could be added to PortParse later
since we now pass qemuCaps into that function.
b) live definition before the domain is started -- there are various
places where such information is changed and because some of them
are in BuildCommandLine and some are not, and our tests only call
that function, we don't have a check for those modified in
ProcessLaunch, for example.
I don't know whether there's an ongoing effort to do this, but I
overheard some chatter about that. Not a problem of this patch, of
course, just needed to let the steam out somewhere ;)
> Still this is a good start at that, so ACK.
>
>
Thanks, pushed now
- Cole
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)