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

[Daniel Henrique Barboza]

On 12/4/20 12:13 PM, Andrea Bolognani wrote:
> On Fri, 2020-12-04 at 15:55 +0100, Andrea Bolognani wrote:
>> On Thu, 2020-12-03 at 22:05 -0300, Daniel Henrique Barboza wrote:
>>> This reverts commit d3f3c2c97f9b92c982ff809479495f44614edb88
>>> (domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()).
>>>
>>> This reverts commit ace5931553c87beebb6b3cd994061742b3f88238
>>> (conf, qemu: move qemuDomainNVDimmAlignSizePseries to domain_conf.c).
>>
>> Please revert the two commits separately.
> 
> Actually, looking at the rest of the series, I think it's better if
> you do *not* go ahead with a straight revert, since that results in
> 
>    1) returning the function to its old place and signature;
>    2) having to change the signature back;
>    3) having to move the function because you need it to be usable
>       earlier in the file.
> 
> That's a whole lot of churn for arguably very little benefit.
> 
> 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.

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.


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?



DHB


> 
> Sorry for not realizing earlier that my initial suggestion would have
> these consequences.
>