Code review comment for lp:~stgraber/ubuntu/trusty/systemd/logind-cgmanager

Hey Stéphane,

great work! This isn't upstreamable anyway (as upstream moved to a completely different cgroup management architecture), so I don't think all that careful HAVE_CGMANAGER etc. is actually needed. But if you need it for some kind of non-Ubuntu usage, this bit in Makefile.am probably ought to be conditional on ENABLE_CGMANAGER as well?

682 src/shared/cgmanager.c \
683 src/shared/cgmanager.h \

Same with e. g. in logind.c or src/shared/cgroup-util.c

41 #include "cgmanager.h"

108 Suggests: cgmanager

Out of interest, will cgmanager installed by default on trusty hosts/containers? (I suppose you only need it on the host?)

I'm a bit surprised by the addition of src/shared/cgmanager.[hc]. Shouldn't those be in libcgmanager-dev/libcgmanager0?

src/shared/cgroup-util.c:

55 if (cgm_dbus_connect()) {
56 template = strdup("/tmp/.cgmanager-logind.XXXXXX");

"template" leaks here, both with successful operation as well as in various error paths. It does not need to be initialized that early, so you can move it further down to cut some error paths. But actually this is an excellent place for using _cleanup_free_, so you don't need to worry about all the error paths.

Same problem further down (line 112 ff.)

Is there a bug report for this? I suppose this needs an FFE and proper testing at this point of the release cycle.

LGTM otherwise, thanks!