[libvirt] [libvirt PATCH v2 0/4] Share cgroup code that is duplicated between QEMU and LXC

[Michal Privoznik]

On 09/12/2018 12:46 PM, Pavel Hrdina wrote:
> On Wed, Sep 12, 2018 at 10:57:32AM +0200, Fabiano Fidêncio wrote:
>> virLXCCgroupSetupBlkioTune() and qemuSetupBlkioCgroup() and
>> virLXCCgroupSetupCpuTune() and qemuSetupCpuCgroup() are the most similar
>> functions between QEMU and LXC code.
>>
>> Let's move their common code to virCgroup.
>>
>> Mind that the first two patches are basically preparing the ground for
>> the changes introduced in the last two patches.
> 
> Hi, definitely good idea to remove code duplication!
> 
> We have similar issue with the virDomainSetBlkioParameters for QEMU and
> LXC drivers.  The code to set cgroup values is the same.
> 
> Since the common object is domain how about introducing
> virDomainSetupBlkioTune and virDomainSetupMemTune and move it into
> domain_conf.c, that way we don't have to extract domain specific data
> into src/util.

I'd rather remove stuff from doman_conf.c than add something there. It's
already the biggest file by far margin:

libvirt.git $ find . -type f -iname \*.c -exec du -h {} \; | sort -h
416K    ./src/qemu/qemu_domain.c
696K    ./src/qemu/qemu_driver.c
956K    ./src/conf/domain_conf.c

And moving code from domain_conf is something we do from time to time.
Just look at all those virnet* includes at the beginning of domain_conf.h.

Another reason for moving the code out is that blkio tune is not domain
specific. In the future, we might want to limit say iohelper and thus
move it into its specific cgroup.

I'm not that convinced about 2/4 to be honest. Memory specification is
pretty much domain centric. But it follows my first point - moving code
out from domain_conf.c. And it de-duplicates the code.

> 
> Another benefit is that it will not cause me merge conflicts because I'm
> rewriting cgroup code and adding support for cgroup v2.

I don't think that code movement should cause that big of a trouble.
You'll get some context conflicts but that's always the case with 30+
unmerged patches.

Michal