Re: [libvirt] [PATCH 1 of 2] Add internal cgroup manipulation functions

BS> There is no support for permissions, is everything run as root? The LXC driver must run as root, yes. BS> Is 512 arbitrary? How do we know it is going to be sufficient? This should probably be a constant, but it's more than enough for any of the values we're setting. Defining it specifically will help make that clear. BS> So every routine calls cgroup_path_of(), reads the mounts entry BS> and find a entry for cgroup and returns it, why not do it just BS> once and use it. Because maintaining the (potentially stale) state isn't worth the small amount of work necessary to parse the mount table on each domain creation (IMHO). BS> I see a mix of open and fopen calls.I would prefer to stick to BS> just one, helps with readability. You mean because I use open() and getmntent_r() wants a FILE*? I think I'm willing to live with that :) BS> I don't think 512 bytes will be sufficient. THere are multi-line BS> files like memory.stat. We don't read memory.stat (or any other values at the moment). However: % for i in $(find /cgroup | grep memory.stat); do cat $i | wc -c; done 53 53 53 98 I don't see any point in allocating a huge amount of memory on the stack each time if we're not going to use it. If we need to read larger values in the future, we could move to an allocated string. BS> Why do you need access? mkdir will do that check anyway. Yep, I'll change it. BS> Why 0655? Why not use meaningful names see sys/stat.h I don't think that the meaningful names in sys/stat.h lead to better readability, personally. Octal permissions are used elsewhere int the code pretty extensively, so I'm comfortable with this as it is. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms(a)us.ibm.com