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

On Wed, Sep 12, 2018 at 03:18:26PM +0200, Michal Privoznik wrote: > 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. Both seem to follow cgroups to a point, so they're process-centric, not domain-centric. So if they're needed on lower (src/util) layers, separating them makes sense. (Not that domain_conf does not deserve to get both the parser and the formatter to be split out at this line count)

> 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. Such downstream branches should not interfere with upstream libvirt development. IOW: it's your opportunity as a maintainer to offer to merge this patch on top of your changes if merging them now would cause you too much trouble. ;)