Adding --set to videoinfo to retrieve current video resolution

[Markus Scholz]

Dear Grub Developers,

recently we faced the problem the author in this old thread faced:
- https://lists.gnu.org/archive/html/grub-devel/2012-10/msg00009.html

Ie. our theme wouldn't display properly due to different screen resolutions.
Hence, I wanted to change specifically the font size of our theme given the
current resolution.
While the videoinfo module provides info about the current resolution I
wasn't able to
discover any way to get the info from the booted grub.

Following the aforementioned thread, I therefore patched the videoinfo mod
to add the ability
of a -set switch just like commands like "smbinfo" or "search" have.

Ie. the documentation of the videoinfo call could change as follows:

Command: videoinfo [--set var | [WxH]xD]

    Retrieve available video modes. If --set is given, assign the currently
active resolution to var.
    If --set is not provided the available video modes are listed.
    If resolution is given, show only matching modes. 
    
Attached is my patch proposal for the module; I probably made a lot mistakes
wrt.
coding style and guidelines - please bear with me.

I would be grateful regarding feedback what I could do / how I could improve
the patch to make it part of grub? If it possible at all with this approach?

Thank you for your reply & best,
Markus

--- 

--- grub-mod/grub-core/commands/videoinfo.c     2022-10-12
13:30:54.992451000 +0000
+++ grub/grub-core/commands/videoinfo.c 2022-10-12 13:31:37.715512000 +0000
@@ -30,7 +30,6 @@ GRUB_MOD_LICENSE ("GPLv3+");
 struct hook_ctx
 {
   unsigned height, width, depth;
-  unsigned char set_variable; 
   struct grub_video_mode_info *current_mode;
 };
 
@@ -39,9 +38,6 @@ hook (const struct grub_video_mode_info
 {
   struct hook_ctx *ctx = hook_arg;
 
-  if (ctx->set_variable) /* if variable should be set don't print all modes
*/
-    return 0;
-
   if (ctx->height && ctx->width && (info->width != ctx->width ||
info->height != ctx->height))
     return 0;
 
@@ -136,40 +132,31 @@ grub_cmd_videoinfo (grub_command_t cmd _
   grub_video_adapter_t adapter;
   grub_video_driver_id_t id;
   struct hook_ctx ctx;
-  
-  ctx.height = ctx.width = ctx.depth = ctx.set_variable = 0;
-  
+
+  ctx.height = ctx.width = ctx.depth = 0;
   if (argc)
     {
-      if (argc == 2 && grub_memcmp(args[0], "--set", sizeof ("--set") - 1)
== 0 )
-  ctx.set_variable = 1;
-      else
-        {
       const char *ptr;
       ptr = args[0];
       ctx.width = grub_strtoul (ptr, &ptr, 0);
       if (grub_errno)
-  return grub_errno;
+       return grub_errno;
       if (*ptr != 'x')
-  return grub_error (GRUB_ERR_BAD_ARGUMENT,
-        N_("invalid video mode specification `%s'"),
-        args[0]);
+       return grub_error (GRUB_ERR_BAD_ARGUMENT,
+                          N_("invalid video mode specification `%s'"),
+                          args[0]);
       ptr++;
       ctx.height = grub_strtoul (ptr, &ptr, 0);
       if (grub_errno)
-  return grub_errno;
+       return grub_errno;
       if (*ptr == 'x')
-  {
-    ptr++;
-    ctx.depth = grub_strtoul (ptr, &ptr, 0);
-    if (grub_errno)
-      return grub_errno;
-  }
-        }
-
+       {
+         ptr++;
+         ctx.depth = grub_strtoul (ptr, &ptr, 0);
+         if (grub_errno)
+           return grub_errno;
+       }
     }
-  
-
 
 #ifdef GRUB_MACHINE_PCBIOS
   if (grub_strcmp (cmd->name, "vbeinfo") == 0)
@@ -177,23 +164,20 @@ grub_cmd_videoinfo (grub_command_t cmd _
 #endif
 
   id = grub_video_get_driver_id ();
-  if (!ctx.set_variable)
-    {
-grub_puts_ (N_("List of supported video modes:"));
-grub_puts_ (N_("Legend: mask/position=red/green/blue/reserved"));
-    }
-  
+
+  grub_puts_ (N_("List of supported video modes:"));
+  grub_puts_ (N_("Legend: mask/position=red/green/blue/reserved"));
+
   FOR_VIDEO_ADAPTERS (adapter)
   {
     struct grub_video_mode_info info;
     struct grub_video_edid_info edid_info;
-    if (!ctx.set_variable)
-  grub_printf_ (N_("Adapter `%s':\n"), adapter->name);
+
+    grub_printf_ (N_("Adapter `%s':\n"), adapter->name);
 
     if (!adapter->iterate)
       {
-        if (!ctx.set_variable)
-    grub_puts_ (N_("  No info available"));
+       grub_puts_ (N_("  No info available"));
        continue;
       }
 
@@ -202,18 +186,7 @@ grub_puts_ (N_("Legend: mask/position=re
     if (adapter->id == id)
       {
        if (grub_video_get_info (&info) == GRUB_ERR_NONE)
-    {
-      ctx.current_mode = &info;
-      if (ctx.set_variable)
-        {
-    /* set grub env var to current mode */
-    char screen_wxh[20];
-    unsigned int width = info.width;
-    unsigned int height = info.height;
-    grub_snprintf (screen_wxh, 20, "%ux%u", width, height);
-    grub_env_set (args[1], screen_wxh);
-        }
-    }
+         ctx.current_mode = &info;
        else
          /* Don't worry about errors.  */
          grub_errno = GRUB_ERR_NONE;
@@ -263,26 +236,19 @@ GRUB_MOD_INIT(videoinfo)
                               /* TRANSLATORS: "x" has to be entered in,
                                  like an identifier, so please don't
                                  use better Unicode codepoints.  */
-                              N_("[--set var | [WxH]xD]"),
-                              N_("Retrieve available video modes. "
-             "--set is given, assign the currently"
-             "active resolution to var. If --set "
-             "is not provided the available video "
-             "modes are listed. If resolution is "
-             "given, show only matching modes."));
-
+                              N_("[WxH[xD]]"),
+                              N_("List available video modes. If "
+                                    "resolution is given show only modes"
+                                    " matching it."));
 #ifdef GRUB_MACHINE_PCBIOS
   cmd_vbe = grub_register_command ("vbeinfo", grub_cmd_videoinfo,
                                   /* TRANSLATORS: "x" has to be entered in,
                                      like an identifier, so please don't
                                      use better Unicode codepoints.  */
-                                  N_("[--set var | [WxH]xD]"),
-           N_("Retrieve available video modes. "
-           "--set is given, assign the currently"
-           "active resolution to var. If --set "
-           "is not provided the available video "
-           "modes are listed. If resolution is "
-           "given, show only matching modes."));
+                                  N_("[WxH[xD]]"),
+                                  N_("List available video modes. If "
+                                     "resolution is given show only modes"
+                                     " matching it."));
 #endif
 }