[PATCH v3 3/5] domain_conf.c: revert auto-alignment of pSeries NVDIMM in post parse

[Daniel Henrique Barboza]

On 12/4/20 3:15 PM, Andrea Bolognani wrote:
> On Fri, 2020-12-04 at 13:50 -0300, Daniel Henrique Barboza wrote:
>> On 12/4/20 12:13 PM, Andrea Bolognani wrote:
>>> I suggest you instead do a pseudo-revert, where you rename the
>>> function (without otherwise altering its signature) and move it to
>>> the part of qemu_domain.c where you are ultimately going to need it;
>>> in the commit message, you can still mention the commit that such a
>>> change is a "spiritual revert" of, but this way we avoid muddying the
>>> waters more than necessary.
>>
>> The change in signature was done to avoid using qemuDomainGetMemorySizeAlignment(def),
>> because that would be another function that would need to be moved up. I forgot to
>> mention that in patch 04.
> 
> Why would you want to avoid using it? It's exactly what that function
> is intended to do. Moving it around is no big deal, and by using it
> you can avoid open-coding the same value twice. See attached diff.

Fair enough.

> 
>> IIUC what you're suggesting can be implemented as follows:
>>
>> - go back to the first revert I was doing in v2 (remove the code from PostParse).
>> Single revert of d3f3c2c97f9b92;
>>
>> - apply the other 2 patches;
>>
>> - do an extra patch to rename/move the function that is now being used only
>> in QEMU files, but without a straight up revert of ace5931553c8. I can also
>> simplify the signature in this step.
> 
> Yes, except of course the other two patches should still include the
> changes that were implemented after my review, eg. they should be as
> seen in
> 
>    https://gitlab.com/abologna/libvirt/-/tree/ppc64-memalign
> 
> and not how they showed up initially on the list.

Yep, that's the idea.

> 
>> The order here is intentional - the double revert in patch 3 done in this series
>> needed to be followed up by changes in patches 4 and 5 (given that the function was
>> renamed in patch 3). In this order, I can pick the patches from your local branch
>> directly and just bother with the renaming in a single follow-up patch.
>>
>>
>> Sounds good?
> 
> Yeah, it sounds like a plan. You can consider all patches from the
> branch linked above
> 
>    Reviewed-by: Andrea Bolognani <abologna at redhat.com>
> 
> and push them at your leisure.


Alright! I'll do the adjustments and push with your R-b.


Thanks,


DHB

>