Re: [PATCH v3 3/5] domain_conf.c: revert auto-alignment of pSeries NVDIMM in post parse
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.
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.
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(a)redhat.com>
and push them at your leisure.
--
Andrea Bolognani / Red Hat / Virtualization
Attachments:
- align-size.diff (text/x-patch — 5.3 KB)