[patch] - bbsplash applet

[Michele Sanges]

Hello Denys, hello all,

Il giorno dom, 17/02/2008 alle 01.02 +0100, Denys Vlasenko ha scritto:

> +struct_system_configuration system_configuration;      ///< hold the system configuration data
> +int fbfd;              ///< descriptor of framebuffer device
> +FILE *theme_file;      ///< splash file
> +unsigned char *addr;   ///< pointer to framebuffer memory
> +struct fb_var_screeninfo screen_infovar;
> +struct fb_fix_screeninfo screen_infofix;
> 
> 
> This is a lot of static data -> bad for NOMMU targets.
> Take a look at "struct globals" trick done in other applets
> to avoid doing this.

Done.



> +static inline void fb_open(void)
> 
> Drop "inline" everywhere. gcc will inline as appropriate itself.

Done.
Yes, I know about gcc but...

Il giorno ven, 08/02/2008 alle 14.28 +0100, Bernhard Fischer ha scritto:
> >+      // initializes the application with the data taken from the
> configuration file
> >+      Init();
> 
> yea, just inline it here.

and

Il giorno ven, 08/02/2008 alle 14.28 +0100, Bernhard Fischer ha scritto:
> >+
> >+      fbOpen();
> >+      fbDrawImage();
> >+      fbDrawProgressBar(0);
> 
> Would be better if you inlined those here. Less files and easier to
> read and follow.

so, I didn't understood, can I leave the functions or must I put their
bodies inline?



> +{
> +       fbfd = open("/dev/fb0", O_RDWR);
> +       if (fbfd < 0)
> +       {
> +               DEBUG_MESSAGE("failed to open the frame buffer device... exit");
> +               exit(EXIT_FAILURE);
> +       }
> 
> Use xopen() instead, it will do complaining and exiting for you.

Done.



> +       // framebuffer properties
> +       if (ioctl(fbfd, FBIOGET_VSCREENINFO, &screen_infovar))
> +       {
> +               DEBUG_MESSAGE("failed to get the framebuffer var properties... exit");
> +               exit(EXIT_FAILURE);
> +       }
> 
> Same with xioctl

Done.



> +static inline bool enableCursor(bool bEnable)

Deleted and replaced with a printf.




> +static inline void fb_close(void)
> 
> Not needed. Please, just exit. Kernel will clean up for you.

Done.



> 
> +       for (x = nx1pos; x <= nx2pos; x++)
> +       {
> +               idx = (ny1pos * screen_infovar.xres + x) * (screen_infovar.bits_per_pixel / 8);
> +               *(DATA *)(addr + idx) = thispix;
> +       }
> +
> +       // bottom horizontal line
> +       for (x = nx1pos; x <= nx2pos; x++)
> +       {
> +               idx = (ny2pos * screen_infovar.xres  + x) * (screen_infovar.bits_per_pixel / 8);
> +               *(DATA *)(addr + idx) = thispix;
> +       }
> 
> Merge into one loop.

Done.


> +       sprintf(strinifile, "/etc/%s/%s.ini", PROGRAM, PROGRAM);
> 
> PROGRAM is a known string. Don't torture yourself by computing
> what you already know.

Done.



> You wait in select() on _one_ descriptor! Why?
> Just read from it. Read will block until data is available.

Done.


> +       sprintf(strfifo, "rm -f /dev/%s_fifo", PROGRAM);
> +       i = system(strfifo);
> 
> OMG... please execute "man 2 unlink" at the nearest shell prompt...

where is the unlink applet in busybox?.... oops, there is still the
source in the old 'e2fsprogs'.
Done.


I send the new patch (for busybox-1.9.1) for comments; the size is:

[root at pigreco busybox-1.9.1]# size busybox
   text    data     bss     dec     hex filename
 620209    2266    9500  631975   9a4a7 busybox

[root at pigreco busybox-1.9.1_bbsplash]# size busybox
   text    data     bss     dec     hex filename
 623462    2330    9500  635292   9b19c busybox


Thanks for your help.
Regards.

Michele Sanges

-------------- next part --------------
A non-text attachment was scrubbed...
Name: bbsplash.diff
Type: text/x-patch
Size: 16950 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20080219/c1c3d2be/attachment-0002.bin