On Mon, Jul 01, 2019 at 02:13:16PM +0100, David Howells wrote: > Christian Brauner <christian@xxxxxxxxxx> wrote: > > > > +config FSINFO > > > > Hm, any reason why we would hide that syscalls under a config option? > > Rasmus Villemoes asked for it to be made conditional. Ah, ok. I guess this is another case of "what about embedded users". Fair enough. > > https://lore.kernel.org/lkml/f3646774-ee9e-d5b7-8a11-670012034d59@xxxxxxxxxxxxxxxxxx/ > > > Do we, not have any dumb helpers for scenarios like this?: > > > > #define strlen_literal(x) (sizeof(""x"") - 1) > > #define strlen_array(x) (sizeof(x) - 1) > > git grep doesn't find them under this name. Yeah, than we don't have that. Might be worth having such helpers at some point. > > > > + while (!signal_pending(current)) { > > > + params->usage = 0; > > > + ret = fsinfo(path, params); > > > + if (IS_ERR_VALUE((long)ret)) > > > + return ret; /* Error */ > > > + if ((unsigned int)ret <= params->buf_size) > > > > if ((size_t)ret ...? Just for the sake of clarity if for nothing else. > > > > > + return ret; /* It fitted */ > > > > Ok, a little confused here, tbh. params->buf_size is size_t > > It's "unsigned int". Ok, good. > > > and this function returns an int. Forgot whether you mentioned this before, > > buf_size exceed can't exceed INT_MAX? > > It's mentioned in the documentation (ie. fsinfo.rst). I'll mention it in the > comments adjacent to the attribute definition table also. Thanks! I missed that apparently. > > > Is it really wort it to have this code generating stuff in there? > > From a readability PoV, yes, tabulation is awesome, IMO;-). Up to 5 lines per > attribute is too much vertical space and expanding it makes the whole thing > much less readable. Add to that that not all attributes will be the same > number of lines. > > It would be easier if the I could get away with making the constant names > lower case, but the thou-shalt-capitalise-constantists dislike that, so, given > that I don't know of a way to make the C preprocessor change the case of a > symbol, I have to include both parts. > > I have four pieces of information: type, depth, constant name, struct name (if > applicable), and I can fit them on one line this way. > > You really find this: > > static const struct fsinfo_attr_info fsinfo_buffer_info[FSINFO_ATTR__NR] = { > [FSINFO_ATTR_STATFS] = { > .type = __FSINFO_STRUCT, > .flags = __FSINFO_SINGLE, > .size = sizeof(struct fsinfo_statfs) > }, > [FSINFO_ATTR_FSINFO] = { > .type = __FSINFO_STRUCT, > .flags = __FSINFO_SINGLE, > .size = sizeof(struct fsinfo_fsinfo) > }, > [FSINFO_ATTR_IDS] = { > .type = __FSINFO_STRUCT, > .flags = __FSINFO_SINGLE, > .size = sizeof(struct fsinfo_ids) > }, > [FSINFO_ATTR_LIMITS] = { > .type = __FSINFO_STRUCT, > .flags = __FSINFO_SINGLE, > .size = sizeof(struct fsinfo_limits) > }, > [FSINFO_ATTR_CAPABILITIES] = { > .type = __FSINFO_STRUCT, > .flags = __FSINFO_SINGLE, > .size = sizeof(struct fsinfo_capabilities) > }, > [FSINFO_ATTR_SUPPORTS] = { > .type = __FSINFO_STRUCT, > .flags = __FSINFO_SINGLE, > .size = sizeof(struct fsinfo_supports) > }, > [FSINFO_ATTR_TIMESTAMP_INFO] = { > .type = __FSINFO_STRUCT, > .flags = __FSINFO_SINGLE, > .size = sizeof(struct fsinfo_timestamp_info) > }, > [FSINFO_ATTR_VOLUME_ID] = { > .type = __FSINFO_STRING, > .flags = __FSINFO_SINGLE, > }, > [FSINFO_ATTR_VOLUME_UUID] = { > .type = __FSINFO_STRUCT, > .flags = __FSINFO_SINGLE, > .size = sizeof(struct fsinfo_volume_uuid) > }, > [FSINFO_ATTR_VOLUME_NAME] = { > .type = __FSINFO_STRING, > .flags = __FSINFO_SINGLE, > }, > [FSINFO_ATTR_NAME_ENCODING] = { > .type = __FSINFO_STRING, > .flags = __FSINFO_SINGLE, > }, > [FSINFO_ATTR_NAME_CODEPAGE] = { > .type = __FSINFO_STRING, > .flags = __FSINFO_SINGLE, > }, > [FSINFO_ATTR_PARAM_DESCRIPTION] = { > .type = __FSINFO_STRUCT, > .flags = __FSINFO_SINGLE, > .size = sizeof(struct fsinfo_param_description) > }, > [FSINFO_ATTR_PARAM_SPECIFICATION] = { > .type = __FSINFO_STRUCT, > .flags = __FSINFO_N, > .size = sizeof(struct fsinfo_param_specification) > }, > [FSINFO_ATTR_PARAM_ENUM] = { > .type = __FSINFO_STRUCT, > .flags = __FSINFO_N, > .size = sizeof(struct fsinfo_param_enum) > }, > [FSINFO_ATTR_PARAMETERS] = { > .type = __FSINFO_OPAQUE, > .flags = __FSINFO_SINGLE, > }, > [FSINFO_ATTR_LSM_PARAMETERS] = { > .type = __FSINFO_OPAQUE, > .flags = __FSINFO_SINGLE, > }, > [FSINFO_ATTR_SERVER_NAME] = { > .type = __FSINFO_STRING, > .flags = __FSINFO_N, > }, > [FSINFO_ATTR_SERVER_ADDRESS] = { > .type = __FSINFO_STRUCT, > .flags = __FSINFO_NM, > .size = sizeof(struct fsinfo_server_address) > }, > [FSINFO_ATTR_AFS_CELL_NAME] = { > .type = __FSINFO_STRING, > .flags = __FSINFO_SINGLE, > }, > [FSINFO_ATTR_MOUNT_INFO] = { > .type = __FSINFO_STRUCT, > .flags = __FSINFO_SINGLE, > .size = sizeof(struct fsinfo_mount_info) > }, > [FSINFO_ATTR_MOUNT_DEVNAME] = { > .type = __FSINFO_STRING, > .flags = __FSINFO_SINGLE, > }, > [FSINFO_ATTR_MOUNT_CHILDREN] = { > .type = __FSINFO_STRUCT_ARRAY, > .flags = __FSINFO_SINGLE, > .size = sizeof(struct fsinfo_mount_child) > }, > [FSINFO_ATTR_MOUNT_SUBMOUNT] = { > .type = __FSINFO_STRING, > .flags = __FSINFO_N, > }, > }; > > is easier to read than this?: Yes, very much so imho. :) > > static const struct fsinfo_attr_info fsinfo_buffer_info[FSINFO_ATTR__NR] = { > FSINFO_STRUCT (STATFS, statfs), > FSINFO_STRUCT (FSINFO, fsinfo), > FSINFO_STRUCT (IDS, ids), > FSINFO_STRUCT (LIMITS, limits), > FSINFO_STRUCT (CAPABILITIES, capabilities), > FSINFO_STRUCT (SUPPORTS, supports), > FSINFO_STRUCT (TIMESTAMP_INFO, timestamp_info), > FSINFO_STRING (VOLUME_ID), > FSINFO_STRUCT (VOLUME_UUID, volume_uuid), > FSINFO_STRING (VOLUME_NAME), > FSINFO_STRING (NAME_ENCODING), > FSINFO_STRING (NAME_CODEPAGE), > FSINFO_STRUCT (PARAM_DESCRIPTION, param_description), > FSINFO_STRUCT_N (PARAM_SPECIFICATION, param_specification), > FSINFO_STRUCT_N (PARAM_ENUM, param_enum), > FSINFO_OPAQUE (PARAMETERS), > FSINFO_OPAQUE (LSM_PARAMETERS), > FSINFO_STRING_N (SERVER_NAME), > FSINFO_STRUCT_NM (SERVER_ADDRESS, server_address), > FSINFO_STRING (AFS_CELL_NAME), > FSINFO_STRUCT (MOUNT_INFO, mount_info), > FSINFO_STRING (MOUNT_DEVNAME), > FSINFO_STRUCT_ARRAY (MOUNT_CHILDREN, mount_child), > FSINFO_STRING_N (MOUNT_SUBMOUNT), > }; > > The latter also has the advantage that I can take this and drop it into the > test program and change the helper macros to make it do other things. With > the fully expanded code, that isn't possible. > > One thing I will grant you, though, I can simplify: > > #define __FSINFO_STRUCT 0 > #define __FSINFO_STRING 1 > #define __FSINFO_OPAQUE 2 > #define __FSINFO_STRUCT_ARRAY 3 > #define __FSINFO_0 0 > #define __FSINFO_N 0x0001 > #define __FSINFO_NM 0x0002 > > #define _Z(T, F, S) { .type = __FSINFO_##T, .flags = __FSINFO_##F, .size = S } > #define FSINFO_STRING(X) [FSINFO_ATTR_##X] = _Z(STRING, 0, 0) > #define FSINFO_STRUCT(X,Y) [FSINFO_ATTR_##X] = _Z(STRUCT, 0, sizeof(struct fsinfo_##Y)) > #define FSINFO_STRING_N(X) [FSINFO_ATTR_##X] = _Z(STRING, N, 0) > #define FSINFO_STRUCT_N(X,Y) [FSINFO_ATTR_##X] = _Z(STRUCT, N, sizeof(struct fsinfo_##Y)) > #define FSINFO_STRING_NM(X) [FSINFO_ATTR_##X] = _Z(STRING, NM, 0) > #define FSINFO_STRUCT_NM(X,Y) [FSINFO_ATTR_##X] = _Z(STRUCT, NM, sizeof(struct fsinfo_##Y)) > #define FSINFO_OPAQUE(X) [FSINFO_ATTR_##X] = _Z(OPAQUE, 0, 0) > #define FSINFO_STRUCT_ARRAY(X,Y) [FSINFO_ATTR_##X] = _Z(STRUCT_ARRAY, 0, sizeof(struct fsinfo_##Y)) > > a bit: > > #define __FSINFO_STRUCT 0 > #define __FSINFO_STRING 1 > #define __FSINFO_OPAQUE 2 > #define __FSINFO_STRUCT_ARRAY 3 > #define __FSINFO_N 0x01 > #define __FSINFO_NM 0x02 > > #define _Z(T, S) { .type = __FSINFO_##T, .flags = 0, .size = S } > #define _Z_N(T, S) { .type = __FSINFO_##T, .flags = __FSINFO_N, .size = S } > #define _Z_NM(T, S) { .type = __FSINFO_##T, .flags = __FSINFO_NM, .size = S } > #define FSINFO_STRING(X) [FSINFO_ATTR_##X] = _Z(STRING, 0) > #define FSINFO_STRUCT(X,Y) [FSINFO_ATTR_##X] = _Z(STRUCT, sizeof(struct fsinfo_##Y)) > #define FSINFO_STRING_N(X) [FSINFO_ATTR_##X] = _Z_N(STRING, 0) > #define FSINFO_STRUCT_N(X,Y) [FSINFO_ATTR_##X] = _Z_N(STRUCT, sizeof(struct fsinfo_##Y)) > #define FSINFO_STRING_NM(X) [FSINFO_ATTR_##X] = _Z_NM(STRING, 0) > #define FSINFO_STRUCT_NM(X,Y) [FSINFO_ATTR_##X] = _Z_NM(STRUCT, sizeof(struct fsinfo_##Y)) > #define FSINFO_OPAQUE(X) [FSINFO_ATTR_##X] = _Z(OPAQUE, 0) > #define FSINFO_STRUCT_ARRAY(X,Y) [FSINFO_ATTR_##X] = _Z(STRUCT_ARRAY, sizeof(struct fsinfo_##Y)) > > > I urge you to think about git grep users. For them this is an absolute > > nightmare. :) > > That's a valid point, but it's a problem all over the kernel. We use > macroisation everywhere. See all the declaration and define macros that nest > layers deep. Well maybe we can stop doing it (at least for some stuff). :) > > If that's your main worry, The attribute type name could be fully expanded in > the table, eg.: > > FSINFO_STRUCT (FSINFO_ATTR_CAPABILITIES, capabilities), > FSINFO_STRING_N (FSINFO_ATTR_MOUNT_SUBMOUNT), > > > > + unsigned int result_size; > > > > Wouldn't it be better if this could be a size_t? > > Why? size_t takes more space on a 64-bit system, but I'm not allowing the > filesystem to return that much data, mainly because I don't really want to be > allocating a >2G buffer. > > In fact, for large objects there's something to be said for writing directly > to userspace rather than going through a buffer, but for the fact that I want > to hold, say, the RCU readlock across the entire transaction in some > instances. > > > > + if (!user_buffer || !user_buf_size) { > > > > Maybe we could be a little more strict and require both be set to their > > respective zero values, i.e. only support reporting the size if > > !user_buffer && user_buf_size = 0 for that to work. If only one of them > > is set to their zero value we report EINVAL. > > That's an option, certainly. Ok, up to you. I find my suggestion a little cleaner. > > > Hm, I'm not sure that "capabilities" is a good name here. This is > > potentially misleading because of other uses of "capabilities" we > > already have. Like, I don't want thes capabilities to pop up when I do > > git grep capabilities. Just a short way until someone also speaks of > > "fscaps" or "fsinfocaps" and then confusion is basically guaranteed. :) > > > > Maybe "features" would be better? > > Yeah - that's probably better. The only issue is that it doesn't have a nice > short hypocoristicon like "cap", though I could use "feat" I guess. FEAT is probably ok. Not pretty but "cap" isn't either. > > > > +#define _ATFILE_SOURCE > > > > nit: Defining fsinfoat() implicitly or what's that supposed to do? If that's > > the case wouldn't it be nicer to just explicitly declare fsinfoat() > > Um... fsinfo() takes AT_* flags. It's fsinfoat(), ffsinfo() and lfsinfo() > all rolled into one, plus a couple of extra bits. It doesn't really need an > at-suffix on the name as there's no at-less original. Ah, seems like a very ancient macro...
