[BusyBox] [patch] New applet, "count".

Rob Landley rob at landley.net
Sat Oct 4 06:07:02 UTC 2003


On Friday 03 October 2003 20:29, Manuel Novoa III wrote:
> Hello Rob,
>
> On Fri, Oct 03, 2003 at 12:33:37AM -0500, Rob Landley wrote:
> > This is something I've used a lot, it's a quick and dirty progress
> > indicator
>
> I'm afraid the emphasis seems to be on "dirty".  There are a number
> of problems, as explained below.

Part of the reason I did this is as a learning experience.  (How to add an 
applet, etc.)

I'm under the vague impression that there needs to be some kind of global 
declared at the top of the file with the applet's name in it.  I think I saw 
it in one of the other files...

> > #include <stdio.h>
> > #include <unistd.h>
> > #include "busybox.h"
> >
> > /* Pipes work in system page size chunks. */
> > #define BUFFER_SIZE 4096
> > char *message="%ld bytes%c";
>
> static const char message[] = "%ld bytes%c";
>
> would have been better here for both size and namespace reasons.
> But you don't really need to reference message twice (see below).

The thing is, with only one call, if there's no data the loop never gets 
called, so there's no "0 bytes" message printed for an empty pipeline.

But if that's acceptable behavior, life is good...

> In any case, it is always advisable to run both 'size' and 'nm'
> on an applet's object file to look for potential problems.
>
> > int count_main(int argc, char **argv)
> > {
> >     long bytes=0;
>
> Just as easy to use an unsigned long here, and it might be worth making
> using unsigned long long an option since you don't know how much data
> might be passing through.

Promoting it to unsigned is good (that's what I had in my original version, I 
just slapped a new implementation together off the top of my head adding it 
to busybox...)

As for long long, I'm not against doing it but I am against additional config 
options that serve very little purpose.  How about instead of making it a new 
option, make it part of long file support, which we already have a config 
option for. :)

> >     char buf[BUFFER_SIZE];
>
> Busybox has configureable buffer allocation policies.  See busybox.h
> for RESERVE_CONFIG_BUFFER and RELEASE_CONFIG_BUFFER defines.  Some
> environments (notably uClinux) have limited stack space.

Cool.  I have learned something new. :)

> >     for(;;) {
> >         int len=read(0,buf,BUFSIZ);
>
> You want BUFFER_SIZE here.

Ahem, yes.  My bad.

> >         if(len) {
> >             bytes+=len;
> >             write(1,buf,len);
>
> And if read fails, or write fails or is short?  There really should be
> some error checking.  Otherwise, using this in an archiving pipeline
> is dangerous.

Good point.

> >             fprintf(stderr,message,bytes,'\r');
> >         } else break;
> >     }
> >
> >     fprintf(stderr,message,bytes,'\n');
>
> You can replace the two fprintf calls with one fprintf and one fputc,
> as illustrated below.

At the expense of no message for 0 bytes written.  But that's not the end of 
the world.

Actually, if the increment were inside the curly brackets but the fprintf were 
outside, the behavior would be correct for 0 bytes because of the do { } 
while(test)...

> >     return 0;
> > }
>
> Consider the rewritten version below.  It is slightly larger, but does
> proper error checking and has better busybox integration.

It's a distinct improvement over mine.  Only two suggestions: llu based on 
CONFIG_LFS, and moving the curly brackets so "0 bytes" is printed for a 
stream that had no data pass through it.

(Okay, and I couldn't resist a very minor cleanup to the return logic, which 
probably broke something because I have no caffiene in front of me at the 
moment.  And I turned the spaces back into tabs.)

I would like to point out that "safe_read" does not have a bb_ appended to the 
front of it.  I fixed that as well (well, ok, I made it compile.  I take no 
position about whether or not it SHOULD have a bb_ on the front of it, but 
that's a much bigger patch...)

All I can say is that the following file compiled without errors.  (Ah, 
quality control... :)

Rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: count.c
Type: text/x-csrc
Size: 1103 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20031004/a9cb8e55/attachment.c 


More information about the busybox mailing list