| Message ID | 20130727195451.17627.62877.stgit@ray-controller |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, Jul 27, 2013 at 09:54:51PM +0200, Michael Braun wrote: > This hides away the details of the currently in-use VLAN model > and is preparing for adding tagged VLAN support later on. > Implementing this as inline functions lets the compiler create > as fast code as before the change. > src/common/vlan.h | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > create mode 100644 src/common/vlan.h Hmm.. This should likely be merged with some other commit(s) since on its own, adding a new header file doesn't do anything. > diff --git a/src/common/vlan.h b/src/common/vlan.h > +#define VLAN_NULL 0 > +typedef int vlan_t; Hmm.. I don't think I really like this. In general, there is desire to avoid typedefs and specially typedefs for structures (which is what this turns into further along in the series). Wouldn't it be simpler to just use struct vlan_description instead of vlan_t and #ifdef CONFIG_VLAN_TAGGED deciding which contents the structure has? > +static inline void vlan_alloc(vlan_t *dst, const int untagged) > +{ > + *dst = untagged; > +} It does not look necessary to make these inline functions either. VLAN management operations cannot really be that time critical that the comment about compiler producing as fast code as before would make a real difference. The versions added in 16/25 looks way too long to justify inline functions in a header file.
Am 04.08.2013 20:54, schrieb Jouni Malinen: > On Sat, Jul 27, 2013 at 09:54:51PM +0200, Michael Braun wrote: >> src/common/vlan.h | 53 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 53 insertions(+) >> create mode 100644 src/common/vlan.h > > Hmm.. This should likely be merged with some other commit(s) since on > its own, adding a new header file doesn't do anything. The problem was that the commit to add this header and actually use the new data type in one was too big (to be sent on this mailing list). So I splitted it up and one of the changes left alone was adding this header file. I don't really know which commit should be merged with this? > >> diff --git a/src/common/vlan.h b/src/common/vlan.h >> +#define VLAN_NULL 0 >> +typedef int vlan_t; > > Hmm.. I don't think I really like this. In general, there is desire to > avoid typedefs and specially typedefs for structures (which is what > this > turns into further along in the series). > > Wouldn't it be simpler to just use struct vlan_description instead of > vlan_t and #ifdef CONFIG_VLAN_TAGGED deciding which contents the > structure has? The beauty of having a kind of alias between vlan_t and int is that for the commits that migrate from int to vlan_t, the code after each single commit will still compile and run fine. This is usefull if somebody for example wants to bisect. A typedef is not really needed, a #define vlan_t int would work just as well, though that doesn't really make things better. So maybe have some commits after which the code doesn't compile just cannot be avoided. > >> +static inline void vlan_alloc(vlan_t *dst, const int untagged) >> +{ >> + *dst = untagged; >> +} > > It does not look necessary to make these inline functions either. VLAN > management operations cannot really be that time critical that the > comment about compiler producing as fast code as before would make a > real difference. The versions added in 16/25 looks way too long to > justify inline functions in a header file. I'll change that. Regards, M. Braun
On Sun, Aug 04, 2013 at 09:44:59PM +0200, michael-dev wrote: > The problem was that the commit to add this header and actually use > the new data type in one was too big (to be sent on this mailing > list). > So I splitted it up and one of the changes left alone was adding > this header file. > I don't really know which commit should be merged with this? Mailing list size limits do not prevent me from merging the patches together, so this is not too much of an issue. I was thinking of merging this with all the "VLAN: Use new VLAN data type in *" commits. It's fine to have them separate for review and mailing list posts, but I see no benefit from committing them separately. > >>diff --git a/src/common/vlan.h b/src/common/vlan.h > >>+#define VLAN_NULL 0 > >>+typedef int vlan_t; > >Wouldn't it be simpler to just use struct vlan_description instead of > >vlan_t and #ifdef CONFIG_VLAN_TAGGED deciding which contents the > >structure has? > > The beauty of having a kind of alias between vlan_t and int is that > for the commits that migrate from int to vlan_t, the code after each > single commit will still compile and run fine. This is usefull if > somebody for example wants to bisect. > A typedef is not really needed, a #define vlan_t int would work just > as well, though that doesn't really make things better. > So maybe have some commits after which the code doesn't compile just > cannot be avoided. Obviously compilation must work between each commit. That has nothing to do with whether there is a typedef or a struct with conditional contents. #define is even worse. It is that "vlan_t" that I would prefer not to see there. I was hoping to see something line this: struct vlan_description { #ifdef CONFIG_VLAN_TAGGED int untagged; unsigned int num_tagged; int *tagged; unsigned int* references; #ifdef DEBUG_VLAN_FREE /* enforce that copies cannot be freed by error */ struct vlan_description *self; #endif /* DEBUG_VLAN_FREE */ #else /* CONFIG_VLAN_TAGGED */ int vlan_id; #endif /* CONFIG_VLAN_TAGGED */ }; And _no_ typedef/#define vlan_t, but struct vlan_description * being passed to the functions. This can be added in two parts without breaking build, too, with the first phase looking like this: struct vlan_description { int vlan_id; };
Hi, Am 07.08.2013 10:21, schrieb Jouni Malinen: > Mailing list size limits do not prevent me from merging the patches > together, so this is not too much of an issue. I was thinking of > merging > this with all the "VLAN: Use new VLAN data type in *" commits. It's > fine > to have them separate for review and mailing list posts, but I see no > benefit from committing them separately. so I'll just leave the patches separate as they are and you'll merge them before committing? Or do I need to prepare them for that (i.e. use a different commit message)? > And _no_ typedef/#define vlan_t, but struct vlan_description * being > passed to the functions. This can be added in two parts without > breaking > build, too, with the first phase looking like this: > > struct vlan_description { > int vlan_id; > }; yep, but we cannot have half the files having a "int vlan_id" and the other half "struct vlan_description vlan_id" as one (atleast the gcc version I use) cannot cast between struct and non-struct data types. But if the transition from "int" to "struct vlan_description" is aggregated in a single commit, it will of course work. So I'll change that. Regards, M. Braun
diff --git a/src/common/vlan.h b/src/common/vlan.h new file mode 100644 index 0000000..9982d46 --- /dev/null +++ b/src/common/vlan.h @@ -0,0 +1,53 @@ +/* + * hostapd / VLAN definitions and helpers functions + * Copyright (c) 2013, Michael Braun <michael-dev@fami-braun.de> + * + * This software may be distributed under the terms of the BSD license. + * See README for more details. + */ + +#ifndef HOSTAPD_VLAN_H +#define HOSTAPD_VLAN_H + +#define VLAN_NULL 0 +typedef int vlan_t; + +static inline void vlan_alloc(vlan_t *dst, const int untagged) +{ + *dst = untagged; +} + +static inline void vlan_alloc_copy(vlan_t *dst, const vlan_t *src) +{ + *dst = *src; +} + +static inline void vlan_free(vlan_t *dst) +{ + *dst = 0; +} + +static inline int vlan_cmp(const vlan_t *a, const vlan_t *b) +{ + if (!a && !b) + return 1; + if (!a || !b) + return 0; + return (*a == *b); +} + +static inline int vlan_untagged(const vlan_t *a) +{ + if (!a) + return 0; + return *a; +} + +static inline int vlan_notempty(const vlan_t *a) +{ + if (!a) + return 0; + return *a; +} + +#endif