[Projekt-wlan] [PATCH v4 05/25] VLAN: Create new data type for VLAN description.

[Jouni Malinen]

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;
};

-- 
Jouni Malinen                                            PGP id EFC895FA