Re: [psplash][PATCH] Add fbdev option to set the proper /dev/fbX.

[Richard Leitner <richard.leitner@skidata.com>]

Hi Julien,
comments on the code are below.
On 05/22/2016 07:46 PM, Julien Gueytat wrote:
> It works exactly the same way than the angle parameter:
>  * --angle <-> /etc/rotation file
>  * --fbdev <-> /etc/fbdev file
> 
> Signed-off-by: Julien Gueytat <contact@jgueytat.fr>
> ---
>  psplash-fb.c | 16 ++++++++++------
>  psplash-fb.h |  4 ++--
>  psplash.c    | 57 ++++++++++++++++++++++++++++++++-------------------------
>  3 files changed, 44 insertions(+), 33 deletions(-)
> 
> diff --git a/psplash-fb.c b/psplash-fb.c
> index 8daaf6f..d344e5a 100644
> --- a/psplash-fb.c
> +++ b/psplash-fb.c
> @@ -99,18 +99,20 @@ attempt_to_change_pixel_format (PSplashFB *fb,
>  }
>  
>  PSplashFB*
> -psplash_fb_new (int angle)
> +psplash_fb_new (int angle, int fbdev_id)
>  {
>    struct fb_var_screeninfo fb_var;
>    struct fb_fix_screeninfo fb_fix;
>    int                      off;
> -  char                    *fbdev;
> +  char                     fbdev[9] = "/dev/fb0";
>  
>    PSplashFB *fb = NULL;
>  
> -  fbdev = getenv("FBDEV");
> -  if (fbdev == NULL)
> -    fbdev = "/dev/fb0";
> +  if (fbdev_id > 0 && fbdev_id < 10)
> +    {
> +        // Conversion from integer to ascii.
> +        fbdev[7] = fbdev_id + 48;
> +    }

You removed the possiblity to get the fb device from FBDEV!
Please don't to that as people may rely on that feature!

If you like to add a fbdev commandline option make sure it can co-exist
with the already available methods for setting this option.
And don't forget to add documentation!

>  
>    if ((fb = malloc (sizeof(PSplashFB))) == NULL)
>      {

...

> @@ -205,35 +205,41 @@ int
>  main (int argc, char** argv) 
>  {
>    char      *tmpdir;
> -  int        pipe_fd, i = 0, angle = 0, ret = 0;
> +  int        pipe_fd, i = 0, angle = 0, fbdev_id = 0, ret = 0;
>    PSplashFB *fb;
>    bool       disable_console_switch = FALSE;
> -  
> +
>    signal(SIGHUP, psplash_exit);
>    signal(SIGINT, psplash_exit);
>    signal(SIGQUIT, psplash_exit);
>  
> -  while (++i < argc)
> -    {
> -      if (!strcmp(argv[i],"-n") || !strcmp(argv[i],"--no-console-switch"))
> -        {
> -	  disable_console_switch = TRUE;
> -	  continue;
> -	}
> +  while (++i < argc) {
> +    if (!strcmp(argv[i],"-n") || !strcmp(argv[i],"--no-console-switch"))
> +      {
> +        disable_console_switch = TRUE;
> +        continue;
> +      }
> +
> +    if (!strcmp(argv[i],"-a") || !strcmp(argv[i],"--angle"))
> +      {
> +        if (++i >= argc) goto fail;
> +        angle = atoi(argv[i]);
> +        continue;
> +      }
> +
> +    if (!strcmp(argv[i],"-f") || !strcmp(argv[i],"--fbdev"))
> +      {
> +        if (++i >= argc) goto fail;
> +        fbdev_id = atoi(argv[i]);
> +        continue;
> +      }
>  
> -      if (!strcmp(argv[i],"-a") || !strcmp(argv[i],"--angle"))
> -        {
> -	  if (++i >= argc) goto fail;
> -	  angle = atoi(argv[i]);
> -	  continue;
> -	}
> -      
>      fail:
>        fprintf(stderr, 
> -	      "Usage: %s [-n|--no-console-switch][-a|--angle <0|90|180|270>]\n", 
> -	      argv[0]);
> +              "Usage: %s [-n|--no-console-switch][-a|--angle <0|90|180|270>][-f|--fbdev <0..9>]\n", 
> +              argv[0]);
>        exit(-1);
> -    }
> +  }

Please use the same coding style everywhere in psplash.

Furthermore please avoid/remove trailing whitespaces in new/changed code

>  
>    tmpdir = getenv("TMPDIR");
>  
> @@ -245,10 +251,10 @@ main (int argc, char** argv)
>    if (mkfifo(PSPLASH_FIFO, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP))
>      {
>        if (errno!=EEXIST) 
> -	{
> -	  perror("mkfifo");
> -	  exit(-1);
> -	}
> +	    {
> +	      perror("mkfifo");
> +	      exit(-1);
> +	    }
>      }

This change is irrelevant for new fbdev option.
Please send this whitespace fix as a separate patch.

>  
>    pipe_fd = open (PSPLASH_FIFO,O_RDONLY|O_NONBLOCK);
> @@ -262,10 +268,11 @@ main (int argc, char** argv)
>    if (!disable_console_switch)
>      psplash_console_switch ();
>  
> -  if ((fb = psplash_fb_new(angle)) == NULL) {
> +  if ((fb = psplash_fb_new(angle,fbdev_id)) == NULL)
> +    {
>  	  ret = -1;
>  	  goto fb_fail;
> -  }
> +    }
>  
>    /* Clear the background with #ecece1 */
>    psplash_fb_draw_rect (fb, 0, 0, fb->width, fb->height,
> 

IMHO in this case fixing the whitespace is fine because the if clause
needs to be adopted for the new option.