Patchwork [v4,05/25] VLAN: Create new data type for VLAN description.

login
register
mail settings
Submitter michael-dev@fami-braun.de
Date July 27, 2013, 7:54 p.m.
Message ID <20130727195451.17627.62877.stgit@ray-controller>
Download mbox | patch
Permalink /patch/262507/
State Superseded
Headers show

Comments

michael-dev@fami-braun.de - July 27, 2013, 7:54 p.m.
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.

Signed-hostap: Michael Braun <michael-dev@fami-braun.de>
---
 src/common/vlan.h |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 src/common/vlan.h
Jouni Malinen - Aug. 4, 2013, 6:54 p.m.
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.
michael-dev@fami-braun.de - Aug. 4, 2013, 7:44 p.m.
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
Jouni Malinen - Aug. 7, 2013, 8:21 a.m.
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;
};
michael-dev@fami-braun.de - Aug. 8, 2013, 12:47 p.m.
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

Patch

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