[patch] - bbsplash applet

[Denys Vlasenko]

On Wednesday 13 February 2008 17:57, Michele Sanges wrote:
> Hello,
> 
> Can you take a look to the new version of bbsplash applet?

I'm afraid it looks depressing.

+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.


+static inline void fb_open(void)

Drop "inline" everywhere. gcc will inline as appropriate itself.

+{
+       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.

+       // 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

+static inline bool enableCursor(bool bEnable)
+{
+       int fd, ret;
+
+       fd = xopen ("/dev/tty0", O_RDWR);
+       if (fd < 0)
+               fd = xopen ("/dev/vc/0", O_RDWR);
+
+       if (fd < 0)

xopen never returns -1.

+       {
+               return false;
+       }
+       else

you can do without "else":

if (cond)
    return ...;
/* else: */
statement;


+       {
+               if (bEnable)
+               {
+                       if ((ret = write (fd, "\E[?25h\000", strlen ("\E[?25h\000"))) == -1)

unused "ret".

+                       {
+                               DEBUG_MESSAGE("failed to write on console");
+                       }
+               }
+               else
+               {
+                       if ((ret = write (fd, "\E[?25l\000", strlen ("\E[?25l\000"))) == -1)
+                       {
+                               DEBUG_MESSAGE("failed to write on console");
+                       }
+               }
+
+               close (fd);
+               return true;
+       }
+}


+static inline void fb_close(void)

Not needed. Please, just exit. Kernel will clean up for you.


+       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.

+       sprintf(strinifile, "/etc/%s/%s.ini", PROGRAM, PROGRAM);

PROGRAM is a known string. Don't torture yourself by computing
what you already know.

+       FD_ZERO(&rfds);
+       FD_SET(fd,&rfds);
+
+       fb_open();
+       fb_drawimage();
+       fb_drawprogressbar(0);
+
+       while(1)
+       {
+               int retval = select(fd+1, &rfds, NULL, NULL, NULL);
+
+               if (retval == -1)
+                       continue;
+
+               if (FD_ISSET(fd, &rfds))
+               {
+                       len = read(fd, buf, 255);
...
+               }
+       }

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

+       sprintf(strfifo, "rm -f /dev/%s_fifo", PROGRAM);
+       i = system(strfifo);

OMG... please execute "man 2 unlink" at the nearest shell prompt...
--
vda