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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxx>

and push them at your leisure.


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


Thanks,


DHB






[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux