Re: [libvirt] [PATCH 20/53] vircgroup: register cgroup v2 backend

On 10/04/2018 06:10 PM, Pavel Hrdina wrote: > On Thu, Oct 04, 2018 at 05:26:32PM +0200, Michal Privoznik wrote: >> On 10/02/2018 10:44 AM, Pavel Hrdina wrote: >>> All mandatory callbacks are implemented for cgroup v2 backend so we >>> can register it now. >>> >>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; >>> --- >>> src/util/vircgroupbackend.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/src/util/vircgroupbackend.c b/src/util/vircgroupbackend.c >>> index 79fe6cb73d..7ee39ac8ca 100644 >>> --- a/src/util/vircgroupbackend.c >>> +++ b/src/util/vircgroupbackend.c >>> @@ -52,6 +52,7 @@ virCgroupBackendRegister(virCgroupBackendPtr backend) >>> static void >>> virCgroupBackendOnceInit(void) >>> { >>> + virCgroupV2Register(); >>> virCgroupV1Register(); >>> } >>> >>> >> >> Well, the callbacks are implemented, but that's about it. No API that >> involves reading/setting a value from CGroup will work at this point, oui? >> >> For instance: >> >> virsh blkiotune fedora >> error: Operation not supported: operation 'getBlkioWeight' not supported >> >> ACK if you move this to be the very last patch (or somewhere before the >> tests - I haven't gotten that far yet, so I don't know how tests are >> implemented, whether they need the v2 backend to be registered or not). > > This was intentional to register the backend right after all mandatory > callbacks are implemented. Controller specific callbacks are optional > because some of the controllers that are available in cgroup v1 will > never exist in cgroup v2. Sure, but some of them will. Just like I demonstrated above. > > I can move this patch after the all controllers in this series are > implemented, but there is still devices controller missing and is not > even implemented in kernel and most likely will never exist in v2. Hold on, so how are we supposed to limit what devices qemu has access to if there is no devices cgroup? If not adding new layers of security we should not remove them at least.

> > So technically there is no point of moving it. I think there is. Because right now, around these patches none of the APIs that involve CGroup work. It's okay to add code for new feature and enable it only at the very last moment. I think it's even in our coding guidelines.