[Merge] lp:~stgraber/ubuntu/trusty/systemd/logind-cgmanager into lp:ubuntu/systemd

Martin Pitt martin.pitt at ubuntu.com
Wed Mar 19 06:51:48 UTC 2014


Review: Needs Fixing

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!
-- 
https://code.launchpad.net/~stgraber/ubuntu/trusty/systemd/logind-cgmanager/+merge/211649
Your team Ubuntu branches is requested to review the proposed merge of lp:~stgraber/ubuntu/trusty/systemd/logind-cgmanager into lp:ubuntu/systemd.



More information about the Ubuntu-reviews mailing list