| Message ID | 1313625029-19546-5-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | RFC |
| Headers | show |
On Mon, Aug 22, 2011 at 1:50 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote: >> +typedef enum { >> + MTD_OOB_PLACE, >> + MTD_OOB_AUTO, >> + MTD_OOB_RAW, >> +} mtd_oob_mode_t; > > Could we get rid of this typedef and use anonymous enum instead: > > enum { > A, > B, > }; > > Indeed, we do not need it, and we do not want to pollute the user-space > namespaces unnecessarily. Well, we do *use* the typedef in a few kernel structs, and we will use it in user space as well. So we may run into issues with 32-bit vs. 64-bit integers if we just stick with an int-based type, right? Also, I'm pondering the question: do we need to worry about the underlying numbering for such an enum? I believe there it is theoretically possible for different compilers to choose a different numbering scheme, potentially making user-compiled software incompatible with the internal kernel binary. Perhaps to be safe we could just do: enum { MTD_OOB_PLACE = 0, MTD_OOB_AUTO = 1, MTD_OOB_RAW = 2, }; Or instead, maybe we should just switch to a __u32 type and some #defines... Sorry if this is a lot of thinking out loud here :) Brian
On Mon, 2011-08-22 at 14:43 -0700, Brian Norris wrote: > On Mon, Aug 22, 2011 at 1:50 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > > On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote: > >> +typedef enum { > >> + MTD_OOB_PLACE, > >> + MTD_OOB_AUTO, > >> + MTD_OOB_RAW, > >> +} mtd_oob_mode_t; > > > > Could we get rid of this typedef and use anonymous enum instead: > > > > enum { > > A, > > B, > > }; > > > > Indeed, we do not need it, and we do not want to pollute the user-space > > namespaces unnecessarily. > > Well, we do *use* the typedef in a few kernel structs, and we will use > it in user space as well. So we may run into issues with 32-bit vs. > 64-bit integers if we just stick with an int-based type, right? I just find typedefs unreadable. Every time you see it - you need to look at the definition, find it, distract. And C does not do any type checking anway, so typedefs for enums really make little sense. If you really do not want to use int, let's use 'enum blah' type, which is at least readable - when you see it - you know it is just an integer, and you do not have to find the definition. Anonymous enums is my preference. They are just like macro definition, but with assumed type, and potentially debugger-friendly. So, to conclude: 1. typedefs for enums make no sense a all - C does not do any strict type-checking anyway, and typedefs only make reading code more difficult. 2. using 'enum blah' is more sensible - it does not hurt readability too much at least. 3. IMHO, and I even dare to say that many kernel gurus would agree, if we are talking about a simple enum with just few constants inside - anonymous enum is the best - the code is simpler and more readable. It is not that difficult to remove in-kernel usage of typedefs, I think. VS user-space - the only user we care about - mtd-utils - has its own copy of the headers, and if we update the header files there, we can amend mtd-utils. 32/64 problems - what do you mean? enumeration = 'int' which is always 32 bits in all architectures Linux supports. References: 1. C99 standard, section 6.4.4.3: enumeration = int 2. C99 section 6.2.5, marker 20: about enum = just definition of a constant and _not_ a typed form. So, I'd be happy with 3, can live with 2, and dislike 1 very much :-) > Also, I'm pondering the question: do we need to worry about the > underlying numbering for such an enum? I believe there it is > theoretically possible for different compilers to choose a different > numbering scheme, potentially making user-compiled software > incompatible with the internal kernel binary. Perhaps to be safe we > could just do: > > enum { > MTD_OOB_PLACE = 0, > MTD_OOB_AUTO = 1, > MTD_OOB_RAW = 2, > }; I cannot refer to a section in a standard, but I am sure unitialized tags will start with 0 reliably. But yes, it is much less error-prone to initialize the tags explicitly, because it makes it a bit more difficult to make a stupid mistake by adding a new constant and change values of other constants. > Or instead, maybe we should just switch to a __u32 type and some #defines... Anonymous enum is almost that, you can just treat it as int32_t. > Sorry if this is a lot of thinking out loud here :) Yeah, least significant topics usually cause most discussions - I was told recently by a colleague that this is called "bike shedding". Notice how much I wrote comparing to more important posts of yours :-)
On Mon, Aug 22, 2011 at 10:30 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > 32/64 problems - what do you mean? enumeration = 'int' which is always > 32 bits in all architectures Linux supports. Oops, got things mixed up here (ints and pointers). Thanks for the correction. My worry was unfounded... > 3. IMHO, and I even dare to say that many kernel gurus would agree, if > we are talking about a simple enum with just few constants inside - > anonymous enum is the best - the code is simpler and more readable. ... > So, I'd be happy with 3, can live with 2, and dislike 1 very much :-) 3 sounds perfect, now that I've been straightened out. > I cannot refer to a section in a standard, but I am sure unitialized > tags will start with 0 reliably. But yes, it is much less error-prone to > initialize the tags explicitly, because it makes it a bit more difficult > to make a stupid mistake by adding a new constant and change values of > other constants. It's also useful if we have to kill an old option at some point. I'll probably include initializers. >> Sorry if this is a lot of thinking out loud here :) > > Yeah, least significant topics usually cause most discussions - I was > told recently by a colleague that this is called "bike shedding". Notice > how much I wrote comparing to more important posts of yours :-) I actually learned/recalled a few things here; I had pieces of the puzzle but not the whole picture. And the "bike shed" is an important addition to my vocabulary. Thanks, Brian
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index 46682ac..e089bce 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -68,20 +68,6 @@ struct mtd_erase_region_info { unsigned long *lockmap; /* If keeping bitmap of locks */ }; -/* - * oob operation modes - * - * MTD_OOB_PLACE: oob data are placed at the given offset - * MTD_OOB_AUTO: oob data are automatically placed at the free areas - * which are defined by the ecclayout - * MTD_OOB_RAW: mode to read oob and data without doing ECC checking - */ -typedef enum { - MTD_OOB_PLACE, - MTD_OOB_AUTO, - MTD_OOB_RAW, -} mtd_oob_mode_t; - /** * struct mtd_oob_ops - oob operation operands * @mode: operation mode diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h index 2f7d45b..f850d9a 100644 --- a/include/mtd/mtd-abi.h +++ b/include/mtd/mtd-abi.h @@ -45,6 +45,20 @@ struct mtd_oob_buf64 { __u64 usr_ptr; }; +/* + * oob operation modes + * + * MTD_OOB_PLACE: oob data are placed at the given offset + * MTD_OOB_AUTO: oob data are automatically placed at the free areas + * which are defined by the ecclayout + * MTD_OOB_RAW: mode to read oob and data without doing ECC checking + */ +typedef enum { + MTD_OOB_PLACE, + MTD_OOB_AUTO, + MTD_OOB_RAW, +} mtd_oob_mode_t; + #define MTD_ABSENT 0 #define MTD_RAM 1 #define MTD_ROM 2
We will want to use the MTD_OOB_{PLACE,AUTO,RAW} modes in user-space applications through the introduction of new ioctls, so we should make this enum a shared type. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- include/linux/mtd/mtd.h | 14 -------------- include/mtd/mtd-abi.h | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 14 deletions(-)