[patch] - bbsplash applet

[Michele Sanges]

Il giorno lun, 25/02/2008 alle 17.57 +0100, Denys Vlasenko ha scritto:
> Whitespace damage in many places; brace style ({}) is inconsistent
> with the rest of busybox.

OK


>  *      - 'bbsplash=on' this one is useful when you start the applet in the initramdisk and,
>  *              for some reasons, you want disable it.
> 
> I don't think abusing kernel command line like this is wise.
> You can do it in shell instead if you want:
> 
> grep -q "bbsplash=1" </proc/commandline && bbsplash <params>

OK, I removed the parsing of /proc/cmdline.


>  * - make the directory '/etc/bbsplash' and put in it the files 'bbsplash.ini' and the
>  *              splash image in .ppm format. Configure the applet by editing the .ini file.
>  * - start the daemon
>  * - send to the /dev/bbsplash_fifo the following messages:
> 
> Hardcoded paths - bad, we have command line parameters/options for such things.

They are simple configuration files.... why they can't stay to a fixed
place?


>         G.fbfd = xopen("/dev/fb0", O_RDWR);
> 
> And if user wants /dev/fb1?

This can be a useful parameter to pass to the applet.

In summary: can I leave the hardcoded path for /etc/bbsplash and read
the framebuffer device from the command line?



> Removing
> 
> typedef struct {
> ...
> } struct_system_configuration;
> 
> moving its members to struct globals and doing
> s/system_configuration.//g removes almost 1k of
> very long, repetitive names.

OK


>         char head[256]="";
> 
> Use memset! Why? This is why:
>    text    data     bss     dec     hex filename
>  801541     764    7484  809789   c5b3d busybox_old
>  801268     764    7484  809516   c5a2c busybox_unstripped

OK


>                 pvalue = strstr(buf, "=");      // pointer to character '='
> What if there is no '='? And how about strchr?
>                 npos = pvalue - buf;            // position of the character '='
>                 pvalue[strlen(pvalue)] = '\0';
> This is a joke, right??
>                 // parameter
>                 memcpy(strparam, buf, npos);
>                 strparam[npos]= '\0';
> 
> I'd replace this horror with just:
> 
>                 pvalue = strchrnul(buf, '=');   // pointer to character '='
>                 if (*pvalue) *pvalue++ = '\0';

Yes, I agree, is a horror :-)


>                                 G.logfile_fd = fopen_or_warn("/dev/bbsplash_log", "w");
>                                 if (G.logfile_fd == NULL) {
>                                         DEBUG_MESSAGE("failed to open the log file... exit");
>                                         exit(EXIT_FAILURE);
>                                 }
> 
> Just use xfopen if you want to exit if file can't be opened.

The idea is loggin the failures, if the applet is started from the
initrd you can lose the console message.


>         i = getopt32(argv, "c");
>         if (i == 1)
>                 bCursorOff = true;
> 
> How about just    bCursorOff = getopt32(argv, "c");    ?

OK


> Replaced some shorts with ints:
> 
> function                                             old     new   delta
> bbsplash_main                                       1385    1383      -2
> fb_drawfullrectangle                                 174     108     -66
> fb_drawprogressbar                                   520     402    -118
> ------------------------------------------------------------------------------
> (add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-186)           Total: -186 bytes
>    text    data     bss     dec     hex filename
>  801101     728    7484  809313   c5961 busybox_old
>  800915     728    7484  809127   c58a7 busybox_unstripped
> 
> They aren't so short in reality. ;)

Yes, I see :-)


> Making DEBUG_MESSAGE optional:
> 
> function                                             old     new   delta
> static.__FUNCTION__                                   22       -     -22
> bbsplash_main                                       1383    1110    -273
> ------------------------------------------------------------------------------
> (add/remove: 0/1 grow/shrink: 0/1 up/down: 0/-295)           Total: -295 bytes
>    text    data     bss     dec     hex filename
>  800915     728    7484  809127   c58a7 busybox_old
>  800432     728    7484  808644   c56c4 busybox_unstripped

Ok, in the coming hours, or perhaps it is better in the coming days :-),
I will retry to send the new patch.

Thanks for suggestions.
Regards
Michele Sanges