Re: [PATCH v3 3/5] domain_conf.c: revert auto-alignment of pSeries NVDIMM in post parse
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.