[BusyBox] grep -B and -A support

[Mark Whitley]

On Fri, Feb 02, 2001 at 05:03:10PM -0500, Richard June wrote:
> Yup, it's an ugly hack, but it works, and i should be able to clean it 
> up and make the buffer work w/ anything else that needs it. thoughts?

Yes, several. :-)


1) Please make the name of the feature BB_FEATURE_GREP_CONTEXT_LINES, instead
of BB_FEATURE_LINES_BA.


2) I noticed that you're using a single global variable called 'lines' that is
used by both the -A and -B options to determine context lines. Whether these
lines are before or after is controlled by the 'state' variable. This is
problematic for two reasons:

 - First problem:

	$ cat grepme 
	foo
	bar
	foolish
	baz
	barfight

	(Using GNU grep)

	$ grep -A 1 -B 2 foolish grepme
	foo
	bar
	foolish
	baz

	(Using Busybox grep w/ your patch)

	$ ./busybox grep -A 1 -B 2 foolish grepme
	foo
	bar
	foolish

	(It didn't print any lines after, even though I told it to.)


 - Second problem: With support for A and B in place, it should make it easy
   to add support for C, but as demonstrated above, it can't be done. Here's
   how I suggest you do it: Use two global variables, one called lines_before,
   the other called lines_after; don't make one variable do double-duty. This
   way we can support -C easily like so:

	(at top of file)
	#ifdef BB_FEATURE_GREP_CONTEXT_LINES
	static int lines             = 0;
	static int state             = 0;
	char **buffer                = NULL;
	#endif /* BB_FEATURE_GREP_CONTEXT_LINES

	(then, down in the option parsing)
	#ifdef BB_FEATURE_GREP_CONTEXT_LINES
			case 'A':
				lines_before = strtoul(optarg, &junk, 10) + 1;
				if(*junk != '\0')
					err_msg_and_die("invalid context length argument");
			break;
			case 'B':
				lines_after = (strtoul(optarg, &junk, 10) + 1);
				if(*junk != '\0')
					err_msg_and_die("invalid context length argument");
			break;
			case 'C':
				lines_before = lines_after = (strtoul(optarg, &junk, 10) + 1);
				if(*junk != '\0')
					err_msg_and_die("invalid context length argument");
			break;
	#endif /* BB_FEATURE_GREP_CONTEXT_LINES
			}
		}

	#ifdef BB_FEATURE_GREP_CONTEXT_LINES
		/* see if we need to set up a circular buffer */
		if (lines_before) {
			buffer = (char **)malloc(sizeof(char *) * (lines + 1));
			memset(buffer, 0, sizeof(char *) * (lines + 1));
			state = 2;
		}
	#endif /* BB_FEATURE_GREP_CONTEXT_LINES


3) I like the way you're doing the circular buffer. This kind of thing:

	if(buffer[index])
		free(buffer[index]);
	buffer[index] = strdup(line);
	index++;
	if(index > lines)
		index = 0;

Is exactly what I had in mind. Kudos.


4) You can save yourself a line when mallocing the buffer.

	Instead of this:

	buffer = (char **)malloc(sizeof(char *) * (lines + 1));
	memset(buffer, 0, sizeof(char *) * (lines + 1));

	Do this:

	buffer = (char **)calloc(lines + 1, sizeof(char *));

The calloc() function will automatically zero the memory you are allocating.

Also, always use xmalloc() and xcalloc() (found in utility.c) instead of just
straight malloc() and calloc() because the x- versions do error checking.


5) Is there any reason why these lines were commented out in the 'A' and 'B'
getopt handling?

	/*
		if(*junk != '\0')
			err_msg_and_die("invalid context length argument");
	*/

Along the same lines, it should be 'error_msg_and_die', not 'err_msg_and_die'.
I uncommented them, made that change, it compiled cleanly, and produced the
appropriate error message when I did something like:

	./busybox grep -Aflfl foo grepme
	grep: invalid context length argument

So please, uncomment them.


6) At the top of the option parsing loop you're doing this:

		/* do normal option parsing */
		while ((opt = getopt(argc, argv, "iHhnqvsc"
	#ifdef BB_FEATURE_LINES_BA
	"A:B:"
	#endif
	)) > 0) {

Can I suggest you do this instead:

		char options[] = "iHhnqvsc"
	#ifdef BB_FEATURE_GREP_CONTEXT_LINES
		"ABC"       
	#endif          
		;       
					
		/* do normal option parsing */
		while ((opt = getopt(argc, argv, options)) > 0) {

It's a tiny bit cleaner.


7) Some other general guidelines:

	- Always test busybox grep against GNU grep and make sure the behavior /
	  output is identical between the two.

	- Try several different permutations and  combinations of the features
	  you're adding and make sure they all work (i.e. the problem with -B and
	  -A being mutually exclusive above).

	- Make sure you test compiling against the source both with the feature
	  turn on and turned off and make sure it compiles cleanly both ways.



And lastly, thanks for the work you're doing. It's appreciated.


Mark Whitley
markw at lineo.com