grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/1] feat: add --set argument to videoinfo to write current r


From: Zhang Boyang
Subject: Re: [PATCH 1/1] feat: add --set argument to videoinfo to write current resolution to grub env var
Date: Tue, 13 Dec 2022 19:33:09 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.1

Hi,

On 2022/12/6 23:48, Markus Scholz wrote:
---
  grub-core/commands/videoinfo.c | 92 +++++++++++++++++++++++-----------
  1 file changed, 63 insertions(+), 29 deletions(-)


Commit message shouldn't be empty. You need to write something about your changes. You also need to add your Signed-off-by tags.

diff --git a/grub-core/commands/videoinfo.c b/grub-core/commands/videoinfo.c
index 5eb969748..75e3b4e9f 100644
--- a/grub-core/commands/videoinfo.c
+++ b/grub-core/commands/videoinfo.c
@@ -30,6 +30,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
  struct hook_ctx
  {
    unsigned height, width, depth;
+  unsigned char set_variable;

`bool` can be used instead of `unsigned char` here.

    struct grub_video_mode_info *current_mode;
  };
@@ -38,6 +39,9 @@ hook (const struct grub_video_mode_info *info, void *hook_arg)
  {
    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;
@@ -132,31 +136,40 @@ grub_cmd_videoinfo (grub_command_t cmd __attribute__ ((unused)),
    grub_video_adapter_t adapter;
    grub_video_driver_id_t id;
    struct hook_ctx ctx;
-
-  ctx.height = ctx.width = ctx.depth = 0;
+
+  ctx.height = ctx.width = ctx.depth = ctx.set_variable = 0;
+
    if (argc)
      {
+      if (argc == 2 && grub_memcmp(args[0], "--set", sizeof ("--set") - 1) == 
0 )

grub_strcmp() is more appropriate here?

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

There are white space issues in your patch. Please use GNU coding style.

        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)
@@ -164,20 +177,23 @@ grub_cmd_videoinfo (grub_command_t cmd __attribute__ 
((unused)),
  #endif
id = grub_video_get_driver_id ();
-
-  grub_puts_ (N_("List of supported video modes:"));
-  grub_puts_ (N_("Legend: mask/position=red/green/blue/reserved"));
-
+  if (!ctx.set_variable)
+    {
+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;
-
-    grub_printf_ (N_("Adapter `%s':\n"), adapter->name);
+    if (!ctx.set_variable)
+  grub_printf_ (N_("Adapter `%s':\n"), adapter->name);
if (!adapter->iterate)
        {
-       grub_puts_ (N_("  No info available"));
+        if (!ctx.set_variable)
+    grub_puts_ (N_("  No info available"));
        continue;
        }
@@ -186,7 +202,18 @@ grub_cmd_videoinfo (grub_command_t cmd __attribute__ ((unused)),
      if (adapter->id == id)
        {
        if (grub_video_get_info (&info) == GRUB_ERR_NONE)
-         ctx.current_mode = &info;
+    {
+      ctx.current_mode = &info;
+      if (ctx.set_variable)
+        {
+    /* set grub env var to current mode */
+    char screen_wxh[20];

20 is not sufficient (theoretically) because 32 bit unsigned integer can be as large as 4294967296 (10 digits). You might find grub_xasprintf() useful.

+    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);
+        }
+    }
        else
          /* Don't worry about errors.  */
          grub_errno = GRUB_ERR_NONE;
@@ -236,19 +263,26 @@ GRUB_MOD_INIT(videoinfo)
                               /* TRANSLATORS: "x" has to be entered in,
                                  like an identifier, so please don't
                                  use better Unicode codepoints.  */
-                              N_("[WxH[xD]]"),
-                              N_("List available video modes. If "
-                                    "resolution is given show only modes"
-                                    " matching it."));
+                              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."));
+

The docs in docs/grub.texi also needs updates.

  #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_("[WxH[xD]]"),
-                                  N_("List available video modes. If "
-                                     "resolution is given show only modes"
-                                     " matching it."));
+                                  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."));
  #endif
  }

Best Regards,
Zhang Boyang



reply via email to

[Prev in Thread] Current Thread [Next in Thread]