Quoting Serge E. Hallyn (serue@xxxxxxxxxx): > Quoting Andrew Morgan (morgan@xxxxxxxxxx): > > -----BEGIN PGP SIGNED MESSAGE----- > > Hash: SHA1 > > > > Serge E. Hallyn wrote: > > > Andrew, this version follows all of your suggestions. Definately nicer > > > userspace interface. thanks > > [...] > > > > > > /* Allow ioperm/iopl access */ > > > @@ -314,6 +314,10 @@ typedef struct kernel_cap_struct { > > > > > > #define CAP_SETFCAP 31 > > > > > > +#define CAP_NUM_CAPS 32 > > > + > > > +#define cap_valid(x) ((x) >= 0 && (x) < CAP_NUM_CAPS) > > > + > > > > Could you change the name of CAP_NUM_CAPS? There is some libcap building > > code that does the following to automatically build the "cap_*" names > > for libcap, and this new define above messes that up! :-( > > > > sed -ne '/^#define[ \t]CAP[_A-Z]\+[ \t]\+[0-9]\+/{s/^#define \([^ > > \t]*\)[ \t]*\([^ \t]*\)/ \{ \2, \"\1\" > > \},/;y/ABCDEFGHIJKLMNOPQRSTUVWXYZ/abcdefghijklmnopqrstuvwxyz/;p;}' < > > $(KERNEL_HEADERS)/linux/capability.h | fgrep -v 0x > cap_names.sed > > > > Something like: > > > > #define CAP_NUM_CAPS (CAP_SETFCAP+1) > > > > will save me some hassle. :-) > > Gotcha. Will change that. > > I worry that what you have is just a *touch* too busy so whoever adds > capability #32 might forget to update CAP_NUM_CAPS, but it looks like > > #define CAP_LAST_CAP CAP_SETFCAP > > #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP) > > should also be ok for libcap. > > > [...] > > > > > /* > > > * Bit location of each capability (used by user-space library and kernel) > > > */ > > > @@ -350,6 +354,17 @@ typedef struct kernel_cap_struct { > > > > > > #define CAP_INIT_INH_SET CAP_EMPTY_SET > > > > > > > Its kind of a pity to put a kernel config ifdef in a header file. Could > > you put the ifdef code in the c-files that uses these definitions? > > Hmm, now that you mention it, I notice that the exact same block of > code is still in commoncap.c. I must have lost the patch hunk dropping > that some time ago... > > But at this point CAP_INIT_BSET is only used in > include/linux/init_task.h. And I'd really rather not put the definition > in there. > > Note that the conditional is under a #ifdef __KERNEL__, so applications > shouldn't be looking at it anyway. Does that help? > > > > +#ifdef CONFIG_SECURITY_FILE_CAPABILITIES > > > > In my experience when headers define things differently based on > > configuration #defines, other users of header files (apps, kernel > > modules etc.), never quite know what the current define is. If we can > > avoid conditional code like this in this header file, I'd be happier. > > > > > +#ifdef CONFIG_SECURITY_FILE_CAPABILITIES > > > > ditto. > > For this I really can't, because that is the recommended way to handle > functions with different behavior per CONFIG_ variables. #ifdefs are to > be kept out of .c files to improve their readability, and helper > functions called in .c files are to have their definition in .h files > depend on the CONFIG_ variables. On second thought, I'm going to do exactly what you suggest, because removing CONFIG_SECURITY_FILE_CAPABILITIES checks severaly reduces the amount of recompilation when you switch between CONFIG_SECURITY_FILE_CAPABILITIES=y and n. thanks, -serge _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers