diff mbox series

[v2,4/5] x86: Use getopt to handle command line args

Message ID 20240416091546.11622-5-fouad.hilly@cloud.com (mailing list archive)
State Superseded
Headers show
Series x86/xen-ucode: Introduce --force option | expand

Commit Message

Fouad Hilly April 16, 2024, 9:15 a.m. UTC
Use getopt_long() to handle command line arguments.
Introduce ext_err for common exit with errors.

[v2]
1- Removed unnecessary NULL initialization.
2- Used static at the beginning of type struct declaration.
3- Corrected switch\case indentations.
4- Removed redundant checks.
5- Corrected label indentation.

Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
---
 tools/misc/xen-ucode.c | 43 ++++++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 12 deletions(-)

Comments

Anthony PERARD April 22, 2024, 5:48 p.m. UTC | #1
On Tue, Apr 16, 2024 at 10:15:45AM +0100, Fouad Hilly wrote:
> diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> index 0c0b2337b4ea..e3c1943e3633 100644
> --- a/tools/misc/xen-ucode.c
> +++ b/tools/misc/xen-ucode.c
> @@ -11,6 +11,7 @@
>  #include <sys/stat.h>
>  #include <fcntl.h>
>  #include <xenctrl.h>
> +#include <getopt.h>
>  
>  static xc_interface *xch;
>  
> @@ -22,7 +23,8 @@ static void usage(const char *name)
>      printf("%s: Xen microcode updating tool\n"
>             "Usage: %s [microcode file] [options]\n"
>             "Options:\n"
> -           "show-cou-info   show CPU information and exit\n",
> +           "  -h, --help            display this help and exit\n"
> +           "  -s, --show-cpu-info   show CPU information and exit\n",
>             name, name);
>  }
>  
> @@ -86,6 +88,13 @@ int main(int argc, char *argv[])
>      char *filename, *buf;
>      size_t len;
>      struct stat st;
> +    int opt;
> +
> +    static const struct option options[] = {
> +        {"help", no_argument, NULL, 'h'},
> +        {"show-cpu-info", no_argument, NULL, 's'},
> +        {NULL, no_argument, NULL, 0}
> +    };
>  
>      xch = xc_interface_open(NULL, NULL, 0);
>      if ( xch == NULL )
> @@ -95,19 +104,22 @@ int main(int argc, char *argv[])
>          exit(1);
>      }
>  
> -    if ( argc < 2 )
> -    {
> -        fprintf(stderr,
> -                "xen-ucode: Xen microcode updating tool\n"
> -                "Usage: %s [<microcode file> | show-cpu-info]\n", argv[0]);
> -        show_curr_cpu(stderr);
> -        exit(2);
> -    }
> +    if ( argc != 2 )
> +        goto ext_err;

Why only two arguments allowed? And why check `argc` before calling
getopt_long() ?


>  
> -    if ( !strcmp(argv[1], "show-cpu-info") )
> +    while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 )
>      {
> -        show_curr_cpu(stdout);
> -        return 0;
> +        switch (opt)
> +        {
> +        case 'h':
> +            usage(argv[0]);
> +            exit(EXIT_SUCCESS);
> +        case 's':
> +            show_curr_cpu(stdout);
> +            exit(EXIT_SUCCESS);
> +        default:
> +            goto ext_err;
> +        }
>      }
>  
>      filename = argv[1];

So, after calling getopt_long(), the variable `optind` point to the next
argument that getopt_long() didn't recognize as an argument. It would be
a good time to start using it, and check that the are actually argument
left on the command line, even if in the current patch the only possible
outcome is that argv[1] has something that isn't an option.

> @@ -152,4 +164,11 @@ int main(int argc, char *argv[])
>      close(fd);
>  
>      return 0;
> +
> + ext_err:
> +    fprintf(stderr,
> +            "xen-ucode: Xen microcode updating tool\n"
> +            "Usage: %s [microcode file] [options]\n", argv[0]);

Isn't there a usage() function that we could use?

> +    show_curr_cpu(stderr);

Why call show_curr_cpu() on the error path?

> +    exit(STDERR_FILENO);

Still not an exit value.

Thanks,
Fouad Hilly April 23, 2024, 3:16 p.m. UTC | #2
On Mon, Apr 22, 2024 at 6:48 PM Anthony PERARD <anthony.perard@cloud.com> wrote:
>
> On Tue, Apr 16, 2024 at 10:15:45AM +0100, Fouad Hilly wrote:
> > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> > index 0c0b2337b4ea..e3c1943e3633 100644
> > --- a/tools/misc/xen-ucode.c
> > +++ b/tools/misc/xen-ucode.c
> > @@ -11,6 +11,7 @@
> >  #include <sys/stat.h>
> >  #include <fcntl.h>
> >  #include <xenctrl.h>
> > +#include <getopt.h>
> >
> >  static xc_interface *xch;
> >
> > @@ -22,7 +23,8 @@ static void usage(const char *name)
> >      printf("%s: Xen microcode updating tool\n"
> >             "Usage: %s [microcode file] [options]\n"
> >             "Options:\n"
> > -           "show-cou-info   show CPU information and exit\n",
> > +           "  -h, --help            display this help and exit\n"
> > +           "  -s, --show-cpu-info   show CPU information and exit\n",
> >             name, name);
> >  }
> >
> > @@ -86,6 +88,13 @@ int main(int argc, char *argv[])
> >      char *filename, *buf;
> >      size_t len;
> >      struct stat st;
> > +    int opt;
> > +
> > +    static const struct option options[] = {
> > +        {"help", no_argument, NULL, 'h'},
> > +        {"show-cpu-info", no_argument, NULL, 's'},
> > +        {NULL, no_argument, NULL, 0}
> > +    };
> >
> >      xch = xc_interface_open(NULL, NULL, 0);
> >      if ( xch == NULL )
> > @@ -95,19 +104,22 @@ int main(int argc, char *argv[])
> >          exit(1);
> >      }
> >
> > -    if ( argc < 2 )
> > -    {
> > -        fprintf(stderr,
> > -                "xen-ucode: Xen microcode updating tool\n"
> > -                "Usage: %s [<microcode file> | show-cpu-info]\n", argv[0]);
> > -        show_curr_cpu(stderr);
> > -        exit(2);
> > -    }
> > +    if ( argc != 2 )
> > +        goto ext_err;
>
> Why only two arguments allowed? And why check `argc` before calling
> getopt_long() ?
At this stage of the patch series, xen-ucode expects either firmware
file or a single argument, that is why argc should be 2.
If there is no argument or many arguments that are not supposed to be,
we exit with error other than try to parse the arguments.
>
>
> >
> > -    if ( !strcmp(argv[1], "show-cpu-info") )
> > +    while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 )
> >      {
> > -        show_curr_cpu(stdout);
> > -        return 0;
> > +        switch (opt)
> > +        {
> > +        case 'h':
> > +            usage(argv[0]);
> > +            exit(EXIT_SUCCESS);
> > +        case 's':
> > +            show_curr_cpu(stdout);
> > +            exit(EXIT_SUCCESS);
> > +        default:
> > +            goto ext_err;
> > +        }
> >      }
> >
> >      filename = argv[1];
>
> So, after calling getopt_long(), the variable `optind` point to the next
> argument that getopt_long() didn't recognize as an argument. It would be
> a good time to start using it, and check that the are actually argument
> left on the command line, even if in the current patch the only possible
> outcome is that argv[1] has something that isn't an option.
>
> > @@ -152,4 +164,11 @@ int main(int argc, char *argv[])
> >      close(fd);
> >
> >      return 0;
> > +
> > + ext_err:
> > +    fprintf(stderr,
> > +            "xen-ucode: Xen microcode updating tool\n"
> > +            "Usage: %s [microcode file] [options]\n", argv[0]);
>
> Isn't there a usage() function that we could use?
As there is an error, stderr should be used, which was a comment on V1.
>
> > +    show_curr_cpu(stderr);
>
> Why call show_curr_cpu() on the error path?
Informative, but could be removed.
>
> > +    exit(STDERR_FILENO);
>
> Still not an exit value.
>
> Thanks,
>
> --
> Anthony PERARD

Thanks,

Fouad
Anthony PERARD April 23, 2024, 4:50 p.m. UTC | #3
On Tue, Apr 23, 2024 at 04:16:10PM +0100, Fouad Hilly wrote:
> On Mon, Apr 22, 2024 at 6:48 PM Anthony PERARD <anthony.perard@cloud.com> wrote:
> > On Tue, Apr 16, 2024 at 10:15:45AM +0100, Fouad Hilly wrote:
> > > +    if ( argc != 2 )
> > > +        goto ext_err;
> >
> > Why only two arguments allowed? And why check `argc` before calling
> > getopt_long() ?
> At this stage of the patch series, xen-ucode expects either firmware
> file or a single argument, that is why argc should be 2.
> If there is no argument or many arguments that are not supposed to be,
> we exit with error other than try to parse the arguments.

Right, but what's the point of introducing getopt_long() if we are not
going to use it? The patch tries to make it nicer to introduce more
options to the tool but then put an extra check that make this actually
harder. This condition is even change in the next patch, that's one more
reason that says that it's a wrong check.

You can let getopt_long() parse all the options, deal with them, then
you can deal with the nonoptions after that, and check that's there's
only one nonoption.

> > > + ext_err:
> > > +    fprintf(stderr,
> > > +            "xen-ucode: Xen microcode updating tool\n"
> > > +            "Usage: %s [microcode file] [options]\n", argv[0]);
> >
> > Isn't there a usage() function that we could use?
> As there is an error, stderr should be used, which was a comment on V1.
> >
> > > +    show_curr_cpu(stderr);
> >
> > Why call show_curr_cpu() on the error path?
> Informative, but could be removed.

We are on the error path, it's looks like the usage message is printed
before the cpu information, so if the error is due to wrong options
then that information is lost. We should print why there's an error, not
much else would be useful. Error messages should be as late as possible
or they are getting lost in the scroll of the terminal. So yes, it's
probably best to remove.

Thanks,
diff mbox series

Patch

diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
index 0c0b2337b4ea..e3c1943e3633 100644
--- a/tools/misc/xen-ucode.c
+++ b/tools/misc/xen-ucode.c
@@ -11,6 +11,7 @@ 
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <xenctrl.h>
+#include <getopt.h>
 
 static xc_interface *xch;
 
@@ -22,7 +23,8 @@  static void usage(const char *name)
     printf("%s: Xen microcode updating tool\n"
            "Usage: %s [microcode file] [options]\n"
            "Options:\n"
-           "show-cou-info   show CPU information and exit\n",
+           "  -h, --help            display this help and exit\n"
+           "  -s, --show-cpu-info   show CPU information and exit\n",
            name, name);
 }
 
@@ -86,6 +88,13 @@  int main(int argc, char *argv[])
     char *filename, *buf;
     size_t len;
     struct stat st;
+    int opt;
+
+    static const struct option options[] = {
+        {"help", no_argument, NULL, 'h'},
+        {"show-cpu-info", no_argument, NULL, 's'},
+        {NULL, no_argument, NULL, 0}
+    };
 
     xch = xc_interface_open(NULL, NULL, 0);
     if ( xch == NULL )
@@ -95,19 +104,22 @@  int main(int argc, char *argv[])
         exit(1);
     }
 
-    if ( argc < 2 )
-    {
-        fprintf(stderr,
-                "xen-ucode: Xen microcode updating tool\n"
-                "Usage: %s [<microcode file> | show-cpu-info]\n", argv[0]);
-        show_curr_cpu(stderr);
-        exit(2);
-    }
+    if ( argc != 2 )
+        goto ext_err;
 
-    if ( !strcmp(argv[1], "show-cpu-info") )
+    while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 )
     {
-        show_curr_cpu(stdout);
-        return 0;
+        switch (opt)
+        {
+        case 'h':
+            usage(argv[0]);
+            exit(EXIT_SUCCESS);
+        case 's':
+            show_curr_cpu(stdout);
+            exit(EXIT_SUCCESS);
+        default:
+            goto ext_err;
+        }
     }
 
     filename = argv[1];
@@ -152,4 +164,11 @@  int main(int argc, char *argv[])
     close(fd);
 
     return 0;
+
+ ext_err:
+    fprintf(stderr,
+            "xen-ucode: Xen microcode updating tool\n"
+            "Usage: %s [microcode file] [options]\n", argv[0]);
+    show_curr_cpu(stderr);
+    exit(STDERR_FILENO);
 }