lkml.org 
[lkml]   [2004]   [Jan]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: seperator error in __mask_snprintf_len
> This patch captures what I am looking for in bitmap display and input.

Interesting. I appear to have provoked Joe into a burst of coding.
Now, if I had any smarts, I would stand aside and let Joe own this,
just as Bill Irwin did when I posted my initial lib/mask.c patch a
couple months ago.


Andrew:

If you find Joe's coding more to your liking than my "Gad" style,
I will bless this, and after tossing in a few parting shots, will
stand aside. It meets my essential needs, which were:
- chunked output (a comma every 16 or 32 bits),
- symmetric input and output formats, and
- display and parsing code generic to diverse bitmap sizes.

My actual recommendation, if however you are still undecided, is:
- my patch of last night (with the M32X() 64 bit big endian fix),
- Joe's recommended format, zero-filling to chunksize each word,
- Joe's renaming/refactoring from lib/mask.c to lib/bitmap.h, and
- a chunksize of 16 rather than 32 (Joe likes 16, I don't care).

The essential differences between Joe's and my proposals that I see are:
- Joe's has more code, especially in the parsing routine,
- Joe's bitmap size resolution is bits, not words, and
- I use an implied alloca of 4 x sizeof(mask) bytes.


Comments on Joe's patch:

> ChangeLog:

Good job of summarizing for the Changelog the changes.

> o move into the bitmap.h family, rename and refactor interface to match

I think I like this - good.

> o bitmap size resolution changed from byte to bit

Why? This adds a fair bit of complexity to the code, I suspect.
I am not aware of a need for this, but if there is one, ok.

> o chunking (digits between commas) changed from 8 to 4 digits

Ok - either 8 or 4 works for me. This detail should be decided
by those working on 16 to 32 cpu systems, who will notice this
choice the most. Those of us on larger or smaller systems are
going to see, or not see, separators in either case.

> o display no longer affected by sizeof(unsigned long).

A bit of a misstatement. The display was only affected by the
chunksize, one of 32 (sizeof(u32)*8) or 16 (CHUNKSZ).

> o no alloca usage

True. Though on the other hand, you need to roll your own parsing
code of comma-separated chunks, instead of getting by with using
strsep(). So you trade alloca usage for code complexity. Either
way works - coders choice. I agree that we disagree on this tradeoff.

> o works correctly independent of the size of an unsigned long.

And my version doesn't?

> o works on big and little endian machine.

With my M32X() eor-1 fix, so did mine.

> + * lib/bitmask.c - bitmask manipulation routines too big to go into bitmask.h

Typo? Did you mean bitmap.c and bitmap.h, not bitmask?

> +int bitmap_parse(const char __user *ubuf, unsigned int ubuflen,
> + unsigned long *maskp, unsigned int nmaskbits)

This routine has quite a bit more code detail than my corresponding
parsing routine. I hope that:
(1) providing bit-level resolution, and
(2) removing the implied alloca
justifies this increase in code detail.

> + if (isspace(c))
> + continue;

So a space embedded in a hex number is skipped? That is, your code
parses "dead,beef" and "de a d, bee f" the same? This seems strange.
Perhaps you would prefer to suppress only leading spaces in each chunk:

if (!n && isspace(c))
continue;
> + for (j = 0; j < 32; j++) {

What's this "32", an unrepentant CHUNKSZ?

--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <pj@sgi.com> 1.650.933.1373
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:00    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site