a tricky expression
Mark
Trying to understand some application dealing with VLAN and can't figure out
what this piece of code does:
#define MAX_PORTS_IN_UNIT 28
#define MAX_VLAN_ID 4095
#define VLAN_BITMAP_ARRAY_SIZE (MAX_VLAN_ID/32+1)
unsigned long vlan_member_array[VLAN_BITMAP_ARRAY_SIZE];
int sw_is_set_vlan_bit(unsigned long *VlanBitArray, int vlan_id)
{
return (VlanBitArray[(vlan_id - 1) / 32] & (1 << ((vlan_id - 1) % 32)));
}
...
int port;
for (port = 0; port < MAX_PORTS_IN_UNIT; port++) {
...
/* Set PVID for each port */
if (!sw_is_set_vlan_bit(vlan_member_array, pvid)) {
printf("PVID Error!\n");
} else {
....
}
...
}
Here is a loop running through the ports (let's say 24) and calling
sw_is_set_vlan_bit() for each port:
The code is huge, I hope the snippet I'm providing will suffice. So my
questions are:
1) why to define an array of (MAX_VLAN_ID/32+1) size. What does it indicate?
2) what exactly does the function do?
Thanks!
--
Mark
Thomas Maier-Komor
The function returns 0 if the bit vlan_id in VlanBitArray is not set, if
it is set some value != 0 is returned.
The first portion (i.e. VlanBitArray[(vlan_id - 1) / 32]) selects the
responsible 32bit word in the array, where the relevant bit is located.
The second part (i.e. & (1 << ((vlan_id - 1) % 32))) uses a and mask to
mask out all bits except the relevant one.
The +1 in MAX_VLAN_ID/32+1 is necessary to create a large enough array.
if MAX_VLAN_ID is 16, division by 32 is 0, i.e. the array would be 0
size. Therefore, it is necessary to add one, in case MAX_VLAN_ID is not
a multiple of 32.
HTH,
Thomas
Jens Thoms Toerring
> Hi
> unsigned long vlan_member_array[VLAN_BITMAP_ARRAY_SIZE];
It's not completely clear what all this is meant for, but certain
things look rather obvious to me: the author wanted an array of
bits for each VLAN ID (whatever that is) to be able to store a
single bit of information for each on. Since there are no bit
arrays in C (s)he opted for instead using an array of longs and
using each bit in each long in the array for a different VLAN ID.
So the bit for VLAN ID 1 (the numbering seems to start a 1, not
at 0) would be the least significant bit in 'vlan_member_array[0]',
the one for VLAN ID 2 would be the next higher bit etc. The bit
for VLAN id 33 is then the least significant bit in
'vlan_member_array[1]' etc.
But there seems to be a bug here:
> #define MAX_VLAN_ID 4095
> #define VLAN_BITMAP_ARRAY_SIZE (MAX_VLAN_ID/32+1)
> unsigned long vlan_member_array[VLAN_BITMAP_ARRAY_SIZE];
Instead of as 'MAX_VLAN_ID/32+1' one needs
#define VLAN_BITMAP_ARRAY_SIZE ((MAX_VLAN_ID+1)/32)
to have enough room in the array. And, of course, MAX_VLAN_ID
must be one less than an integer multiple of 32 for this to
work correctly (otherwise you will end up with less array
elements than are required).
There's also an assumption made here that an unsigned long
has (at least) 32 bits. That's correct if meant for "at
least" but may waste space on machines where a long has 8
bytes.
The function
> int sw_is_set_vlan_bit(unsigned long *VlanBitArray, int vlan_id)
> {
> return (VlanBitArray[(vlan_id - 1) / 32] & (1 << ((vlan_id - 1) % 32)));
> }
simple tells if the bit for a certain VLAN ID is set in the
first argument array. The first part
VlanBitArray[(vlan_id - 1) / 32]
gives you the value of the element of the array where the bit
for 'vlan_id' is stored (with the assuption that 'vlan_bit' runs
from 1 to MAX_VLAN_ID+1). For 'vlan_id' from 1 to 32 this gives
the first element of the array, for 'vlan_id' from 33 to 64 its
the second element etc.
The second part
1 << ((vlan_id - 1) % 32)
results in a value with just a single bit set, which one depen-
ding on 'vlan_id'. For 'vlan_id' values of 1, 33, 65 etc. it's
the lowest bit (i.e. a value of 1), for 'vlan_id' values of 2,
34, 66 etc. its the second lowest bit etc. (i.e. a value of 2)
and for 'vlan_id' 32, 64, 96 etc. its the highest bit (i.e. a
value of 2^31). (Nit-pick: I would use '1UL' instead if '1'
for the first '1' in this expression since we want to end up
with an unsigned long value.)
This second value is now used to mask out the correct bit
from the first value using the '&' operator. So the function
returns 0 if the bit for the VLAN ID isn't set and a non-zero
value otherwise.
Regards, Jens
--
\ Jens Thoms Toerring ___ j...@toerring.de
\__________________________ http://toerring.de
Jens Thoms Toerring
> But there seems to be a bug here:
> > #define MAX_VLAN_ID 4095
> > #define VLAN_BITMAP_ARRAY_SIZE (MAX_VLAN_ID/32+1)
> > unsigned long vlan_member_array[VLAN_BITMAP_ARRAY_SIZE];
> Instead of as 'MAX_VLAN_ID/32+1' one needs
> #define VLAN_BITMAP_ARRAY_SIZE ((MAX_VLAN_ID+1)/32)
> to have enough room in the array. And, of course, MAX_VLAN_ID
> must be one less than an integer multiple of 32 for this to
> work correctly (otherwise you will end up with less array
> elements than are required).
Sorry, scratch that. The way it's written is correct (and works
for arbitrary values of MAX_VLAN_ID) and my "replacement" would
get you in trouble! I guess I need more coffee...
Rainer Weikusat
> Trying to understand some application dealing with VLAN and can't
> figure out what this piece of code does:
>
> #define MAX_PORTS_IN_UNIT 28
> #define MAX_VLAN_ID 4095
> #define VLAN_BITMAP_ARRAY_SIZE (MAX_VLAN_ID/32+1)
>
> unsigned long vlan_member_array[VLAN_BITMAP_ARRAY_SIZE];
>
> int sw_is_set_vlan_bit(unsigned long *VlanBitArray, int vlan_id)
> {
> return (VlanBitArray[(vlan_id - 1) / 32] & (1 << ((vlan_id - 1) % 32)));
> }
[...]
> 2) what exactly does the function do?
A VLAN ID, as indicated by this function, is a particular encoding of
a two element vector as int. This vector describes a position in a
128x32 space. This 128x32 space is implemented as array of unsigned
long values (y), each supplying (at least) 32 'data bits'
(x). The y-coordinate comes from (0, 127), indicating an index into
the array, and the x-coordindate from (0, 31), selecting the bit whose
value is 2 to the xth power. The VLAN ID is encoded as
((y << 5) | x) + 1
that is, the y-coordinate is shifted five bits to the left, such that
the five lower bits can be used to hold the x-coordinate. There is no
particular reason for the addition[*]. The two expressions
above decode the encoded value, cleverly disgusing the bit shift and
and operations as integer division and integer division remainder,
presumably, because the useles subtraction was deemed to not be
confusing enough. The recovered bit index is used to create an integer
with exactly this bit set and a &-operation is used to test if the
corresponding bit in the space-array is set. The result is non-zero if
it was (and numerically identical to value of the 1 << expression),
otherwise, zero.
[*] It 'encodes' the author lack of understanding of the C
language, where a[n] is defined as *(a + n), ie, the so-called 'array
index' is, in fact, the distance of the addressed field from
the start of the array, at the expense of confusing potential
readers and forcing the computer to do useless work.
This is further emphasized by the fact that the MAX_VLAN_ID
macros is not defined to be the numerically highest VLAN ID
but the largest valid index for addressing elements of the
array and because of this, another correction is necessary
(that's the MAX_VLAN_ID/32 + 1).
John Kelly
<rwei...@mssgmbh.com> wrote:
> There is no particular reason for the addition[*]
> [*] It 'encodes' the author lack of understanding of the C language
Spotting a bug is one thing. Knowing what the author had in mind is
another. I've been accused of not understanding C due to this use of
sizeof:
char ts[512];
if (sizeof (ts) < 3) {
errno = EOVERFLOW;
perror ("failure specifying North pipe storage");
exit (EXIT_FAILURE);
}
I know the test will always be false. I use it like an assert. Mine
executes, but since the execution penalty is infinitesimal, who cares.
It provides a self documenting reminder of the minimum usable size for
the buffer, also described in a comment at the top of the program.
I don't mind criticism when I can learn something from it. There should
be more code auditing. The accounting profession has auditors, but many
programmers just cross their fingers and hope their code won't explode.
I have a program you looked at once. I've revised it extensively since
then; it's due for another inspection.
There's no need to discuss the two gotos I used. They prevent needless
indentation of large blocks, and they keep certain lines near each other
right where I want them.
There's also no need to discuss further functional decomposition. I
like the large main() the way it is. The tedious repetition of error
checking and exit (EXIT_FAILURE) is a self debugging style. Many of the
errors will never happen unless there is some systemic failure beyond
control of the programmer, but if that happens, the error message will
help the programmer understand the nature of it.
I don't purport to be a C expert. I'm just a workman with some work to
do. Expert advice is welcome:
http://www.beewyz.com/~jar/etcetera/computer/programming/project/dh/
--
Web mail, POP3, and SMTP
http://www.beewyz.com/freeaccounts.php
Rainer Weikusat
[...]
> This is further emphasized by the fact that the MAX_VLAN_ID
> macros is not defined to be the numerically highest VLAN ID
> but the largest valid index for addressing elements of the
> array and because of this, another correction is necessary
> (that's the MAX_VLAN_ID/32 + 1).
Clarification: 4095 is not the largest valid array index (that would
be 127) but the largest possible non-transformed VLAN ID, array index
127, bit index 31.
Rainer Weikusat
> On Fri, 27 Nov 2009 13:52:41 +0100, Rainer Weikusat
> <rwei...@mssgmbh.com> wrote:
>
>> There is no particular reason for the addition[*]
>
>> [*] It 'encodes' the author lack of understanding of the C language
>
> Spotting a bug is one thing. Knowing what the author had in mind is
> another.
In this particular case the range of VLAN IDs is changed from (0,
4095) to (1, 4096). This complicates the code for no particular reason
except 'let the first be 1', this being a traditional grudge some
people have with C: Access to field elements is 0-based, not
1-based. But since the 'index' is actually a distance, that's
perfectly sensible in itself.
> I've been accused of not understanding C due to this use of
> sizeof:
>
> char ts[512];
>
> if (sizeof (ts) < 3) {
> errno = EOVERFLOW;
> perror ("failure specifying North pipe storage");
> exit (EXIT_FAILURE);
> }
>
>
> I know the test will always be false. I use it like an assert. Mine
> executes, but since the execution penalty is infinitesimal, who
> cares.
Well, you obviously don't.
[...]
> I have a program you looked at once. I've revised it extensively since
> then; it's due for another inspection.
There is some code I have to read because of my job. Presently, this
is clamav. And there is code I fortunately don't have to read, like
'useless tools produced by people who abhor writing easily readable
text'. I hope this is clear enough.
John Kelly
<rwei...@mssgmbh.com> wrote:
>there is code I fortunately don't have to read, like useless tools
if (You != everyone) tool = notuseless;
>produced by people who abhor writing easily readable text
You can read minds too!
> I hope this is clear enough.
Funny that.
Rainer Weikusat
> On Fri, 27 Nov 2009 19:45:15 +0100, Rainer Weikusat
> <rwei...@mssgmbh.com> wrote:
>
>>there is code I fortunately don't have to read, like useless tools
>
> if (You != everyone) tool = notuseless;
I didn't expect your opinion on this to be identical to my opinion,
for obvious reasons.
>>produced by people who abhor writing easily readable text
>
> You can read minds too!
There is no reason to 'read a mind' when it communicates about itself
in sufficiently detailed ways. Code which has no purpose confuses the
reader (see original posting), you wrote that you like to write code
without purpose because you use it instead of proper documentation.
Further, you stated that you wouldn't like to structure texts into
sensible and easily intelligible, independent units, but rather just
talk along in however ways you presently feel like doing (that was the
'large function' statement) and at least when I was in school, taking
a that cavalier approach towards composition of non-fictional texts
intended to be read by others would solely have resulted in a bad
grade.
You are free to write F-level technical prose. Nobody has a reason to
care for that.
John Kelly
<rwei...@mssgmbh.com> wrote:
>Further, you stated that you wouldn't like to structure texts into
>sensible and easily intelligible, independent units, but rather just
>talk along in however ways you presently feel like doing (that was the
>'large function' statement) and at least when I was in school, taking
>a that cavalier approach towards composition of non-fictional texts
>intended to be read by others would solely have resulted in a bad
>grade.
Since graduating, I have no need to please professors.
>You are free to write F-level technical prose.
My code may perplex academics, but professionals need a higher level of
comprehension to succeed in the messy real world.
You can leave this argument any time now; I won't mind.
Scott Lurndal
Given that the valid universe of VLAN ID's is [1..4095], perhaps the
author knew what he was doing after all.
Rainer Weikusat
> On Fri, 27 Nov 2009 21:06:35 +0100, Rainer Weikusat
> <rwei...@mssgmbh.com> wrote:
>
>>Further, you stated that you wouldn't like to structure texts into
>>sensible and easily intelligible, independent units, but rather just
>>talk along in however ways you presently feel like doing (that was the
>>'large function' statement) and at least when I was in school, taking
>>a that cavalier approach towards composition of non-fictional texts
>>intended to be read by others would solely have resulted in a bad
>>grade.
>
> Since graduating, I have no need to please professors.
The point of these exercises (and this would have been in highschool in
the US) is not that the pupil 'somehow' manages to get a degree, but
that he or she actually learns something. For instance, how to
structure a text sensibly. And a program is a text.
>>You are free to write F-level technical prose.
>
> My code may perplex academics, but professionals need a higher level of
> comprehension to succeed in the messy real world.
I am just in the process of writing another in-place parser for e-mail
headers. And that is a messy data format which was apparently defined
by 'the Russians' in order to order to hold back western software
technology or something like that. Dealing with messy problems
requires orderly approaches, or, to put it differently, the more
complicated the code to solve a simple problem already is, the simpler
the problems which can still be solved will become.
John Kelly
I didn't demand anyone read my code. I only offered it. If you didn't
want to review it, you could have remained silent.
But you didn't. You replies are bombast.
Rainer Weikusat
That's actually even more awful, because it amounts to doing one or
two subtractions (depending on the capabilities of the compiler) in
order to save a bit at the beginning of the array which is necessarily
'wasted' at the end of it. There is no reason to deal with this in the
code at all. A VLAN ID is a 12-bit value and the range of twelve bit
values is (0, 4095). The number of all possible 12-bit values is 1 <<
12.