Re: [PATCH v4 1/4] kern/vercmp: Add functionality to compare kernel versions

[Daniel Kiper]

On Wed, May 21, 2025 at 12:51:23PM +0000, Alec Brown wrote:
> Add functionality to compare alpha and numeric version segments for kernels.

I think this code applies not only for kernels.

> This can be useful in sorting newer from older kernels.

Where this code come from? Or maybe it is written from scratch. Anyway,
I think whatever it is it should be mentioned in the commit message.

> Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
> ---
>  Makefile.util.def        |  16 ++
>  grub-core/kern/vercmp.c  | 316 +++++++++++++++++++++++++++++++++++++++
>  include/grub/vercmp.h    |  35 +++++
>  tests/vercmp_unit_test.c |  65 ++++++++
>  4 files changed, 432 insertions(+)
>  create mode 100644 grub-core/kern/vercmp.c
>  create mode 100644 include/grub/vercmp.h
>  create mode 100644 tests/vercmp_unit_test.c
>
> diff --git a/Makefile.util.def b/Makefile.util.def
> index 038253b37..15be983f8 100644
> --- a/Makefile.util.def
> +++ b/Makefile.util.def
> @@ -1373,6 +1373,22 @@ program = {
>    ldadd = '$(LIBDEVMAPPER) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
>  };
>
> +program = {
> +  testcase = native;
> +  name = vercmp_unit_test;
> +  common = tests/vercmp_unit_test.c;
> +  common = tests/lib/unit_test.c;
> +  common = grub-core/kern/list.c;
> +  common = grub-core/kern/misc.c;
> +  common = grub-core/tests/lib/test.c;
> +  common = grub-core/kern/vercmp.c;
> +  ldadd = libgrubmods.a;
> +  ldadd = libgrubgcry.a;
> +  ldadd = libgrubkern.a;
> +  ldadd = grub-core/lib/gnulib/libgnu.a;
> +  ldadd = '$(LIBDEVMAPPER) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
> +};
> +
>  program = {
>    name = grub-menulst2cfg;
>    mansection = 1;
> diff --git a/grub-core/kern/vercmp.c b/grub-core/kern/vercmp.c
> new file mode 100644
> index 000000000..c2d69d7fd
> --- /dev/null
> +++ b/grub-core/kern/vercmp.c
> @@ -0,0 +1,316 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2025  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <grub/misc.h>
> +#include <grub/mm.h>
> +#include <grub/err.h>
> +#include <grub/vercmp.h>
> +
> +#define GOTO_RETURN(x) ({ *ret = (x); goto finish; })

You convert an enum to an int. Why? I think it can be simplified...

> +/*
> + * compare alpha and numeric segments of two versions
> + * return 1: a is newer than b
> + *        0: a and b are the same version
> + *       -1: a is older than b
> + */
> +grub_err_t
> +grub_vercmp (const char *a, const char *b, int *ret)

You can add GRUB_VERCMP_UNKNOWN/GRUB_VERCMP_ERR constant to "vercmp"
enum, return that type and drop ret from arguments. Then probably
GOTO_RETURN macro could be also dropped.

> +{
> +  char oldch1, oldch2;
> +  char *abuf, *bbuf;
> +  char *str1, *str2;
> +  char *one, *two;
> +  int rc;
> +  bool isnum;
> +
> +  if (ret == NULL)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("return parameter is not 
> set"));
> +  *ret = 0;
> +
> +  if (grub_strcmp (a, b) == 0)
> +    return GRUB_ERR_NONE;
> +
> +  abuf = grub_strdup (a);
> +  if (abuf == NULL)
> +    return grub_error (GRUB_ERR_OUT_OF_MEMORY, "couldn't duplicate string to 
> compare versions");
> +
> +  bbuf = grub_strdup (b);
> +  if (bbuf == NULL)
> +    {
> +      grub_free (abuf);
> +      return grub_error (GRUB_ERR_OUT_OF_MEMORY, "couldn't duplicate string 
> to compare versions");
> +    }

The const in the arguments suggests you do not need these grub_strdup() calls.

> +  str1 = abuf;
> +  str2 = bbuf;
> +
> +  one = str1;
> +  two = str2;

Could not we reduce number of variables here? If not it should be
explained which one is used for what.

> +  /* Loop through each version segment of str1 and str2 and compare them. */
> +  while (*one != '\0' || *two != '\0')
> +    {
> +      while (*one != '\0' && grub_isalnum (*one) == 0 && *one != '~' && *one 
> != '+')
> +        one++;
> +      while (*two != '\0' && grub_isalnum (*two) == 0 && *two != '~' && *two 
> != '+')
> +        two++;
> +
> +      /* Handle the tilde separator, it sorts before everything else. */
> +      if (*one == '~' || *two == '~')
> +        {
> +          if (*one != '~')
> +            GOTO_RETURN (GRUB_VERCMP_NEWER);
> +          if (*two != '~')
> +            GOTO_RETURN (GRUB_VERCMP_OLDER);
> +          one++;
> +          two++;
> +          continue;
> +        }
> +
> +      /*
> +       * Handle the plus separator. Concept is the same as tilde, except 
> that if
> +       * one of the strings ends (base version), the other is considered as 
> the
> +       * higher version.
> +       */
> +      if (*one == '+' || *two == '+')
> +        {
> +          if (*one == '\0')
> +            GOTO_RETURN (GRUB_VERCMP_OLDER);
> +          if (*two == '\0')
> +            GOTO_RETURN (GRUB_VERCMP_NEWER);
> +          if (*one != '+')
> +            GOTO_RETURN (GRUB_VERCMP_NEWER);
> +          if (*two != '+')
> +            GOTO_RETURN (GRUB_VERCMP_OLDER);
> +          one++;
> +          two++;
> +          continue;
> +        }
> +
> +      /* If we ran to the end of either, we are finished with the loop. */
> +      if (*one == '\0' || *two == '\0')
> +        break;
> +
> +      str1 = one;
> +      str2 = two;
> +
> +      /*
> +       * Grab the first completely alpha or completely numeric segment.
> +       * Leave one and two pointing to the start of the alpha or numeric
> +       * segment and walk str1 and str2 to end of segment.
> +       */
> +      if (grub_isdigit (*str1) == 1)
> +        {
> +          while (*str1 != '\0' && grub_isdigit (*str1) == 1)
> +            str1++;
> +          while (*str2 != '\0' && grub_isdigit (*str2) == 1)
> +            str2++;
> +          isnum = true;
> +        }
> +      else
> +        {
> +          while (*str1 != '\0' && grub_isalpha (*str1) == 1)
> +            str1++;
> +          while (*str2 != '\0' && grub_isalpha (*str2) == 1)
> +            str2++;
> +          isnum = false;
> +        }
> +
> +      /*
> +       * Save the character at the end of the alpha or numeric segment so 
> that
> +       * they can be restored after the comparison.
> +       */
> +      oldch1 = *str1;
> +      *str1 = '\0';
> +      oldch2 = *str2;
> +      *str2 = '\0';
> +
> +      /*
> +       * This cannot happen, as we previously tested to make sure that
> +       * the first string has a non-null segment.
> +       */
> +      if (one == str1)
> +        GOTO_RETURN (GRUB_VERCMP_OLDER); /* arbitrary */
> +
> +      /*
> +       * Take care of the case where the two version segments are different
> +       * types: one numeric, the other alpha (i.e. empty). Numeric segments 
> are
> +       * always newer than alpha segments.
> +       */
> +      if (two == str2)
> +        GOTO_RETURN (isnum ? GRUB_VERCMP_NEWER : GRUB_VERCMP_OLDER);
> +
> +      if (isnum == true)
> +        {
> +          grub_size_t onelen, twolen;
> +          /*
> +           * This used to be done by converting the digit segments to ints 
> using
> +           * atoi() - it's changed because long digit segments can overflow 
> an int -
> +           * this should fix that.
> +           */
> +
> +          /* Throw away any leading zeros - it's a number, right? */
> +          while (*one == '0')
> +            one++;
> +          while (*two == '0')
> +            two++;
> +
> +          /* Whichever number that has more digits wins. */
> +          onelen = grub_strlen (one);
> +          twolen = grub_strlen (two);

This could be calculated above and then you do not need to put NUL char
into strings.

> +          if (onelen > twolen)
> +            GOTO_RETURN (GRUB_VERCMP_NEWER);
> +          if (twolen > onelen)
> +            GOTO_RETURN (GRUB_VERCMP_OLDER);
> +        }
> +
> +      /*
> +       * grub_strcmp will return which one is greater - even if the two 
> segments
> +       * are alpha or if they are numeric. Don't return if they are equal
> +       * because there might be more segments to compare.
> +       */
> +      rc = grub_strcmp (one, two);

grub_strncmp() is probably your friend here.

> +      if (rc != 0)
> +        GOTO_RETURN (rc < 1 ? GRUB_VERCMP_OLDER : GRUB_VERCMP_NEWER);
> +
> +      /* Restore the character that was replaced by null above. */
> +      *str1 = oldch1;
> +      one = str1;
> +      *str2 = oldch2;
> +      two = str2;
> +    }
> +
> +  /*
> +   * This catches the case where all alpha and numeric segments have compared
> +   * identically but the segment separating characters were different.
> +   */
> +  if (*one == '\0' && *two == '\0')
> +    GOTO_RETURN (GRUB_VERCMP_SAME);
> +
> +  /* Whichever version that still has characters left over wins. */
> +  if (*one == '\0')
> +    GOTO_RETURN (GRUB_VERCMP_OLDER);
> +  else
> +    GOTO_RETURN (GRUB_VERCMP_NEWER);
> +
> + finish:
> +  grub_free (abuf);
> +  grub_free (bbuf);
> +  return GRUB_ERR_NONE;
> +}
> +
> +/*
> + * returns name/version/release
> + * NULL string pointer returned if nothing is found
> + */
> +void
> +grub_split_package_string (char *package_string, char **name,
> +                        char **version, char **release)
> +{
> +  char *package_version, *package_release;
> +
> +  /* Release */
> +  package_release = grub_strrchr (package_string, '-');
> +
> +  if (package_release != NULL)
> +      *package_release++ = '\0';
> +
> +  *release = package_release;
> +
> +  if (name == NULL)
> +    *version = package_string;
> +  else
> +    {
> +      /* Version */
> +      package_version = grub_strrchr (package_string, '-');
> +
> +      if (package_version != NULL)
> +        *package_version++ = '\0';
> +
> +      *version = package_version;
> +      /* Name */
> +      *name = package_string;
> +    }
> +
> +  /* Bubble up non-null values from release to name */
> +  if (name != NULL && *name == NULL)
> +    {
> +      *name = (*version == NULL ? *release : *version);
> +      *version = *release;
> +      *release = NULL;
> +    }
> +  if (*version == NULL)
> +    {
> +      *version = *release;
> +      *release = NULL;
> +    }
> +}
> +
> +/*
> + * return 1: nvr0 is newer than nvr1
> + *        0: nvr0 and nvr1 are the same version
> + *       -1: nvr1 is newer than nvr0
> + */
> +grub_err_t
> +grub_split_vercmp (const char *nvr0, const char *nvr1, bool has_name, int 
> *ret)

Same comments like for grub_vercmp().

> +{
> +  grub_err_t err = GRUB_ERR_NONE;
> +  char *str0, *name0, *version0, *release0;
> +  char *str1, *name1, *version1, *release1;
> +
> +  if (ret == NULL)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("return parameter is not 
> set"));
> +
> +  str0 = grub_strdup (nvr0);
> +  if (str0 == NULL)
> +    return grub_error (GRUB_ERR_OUT_OF_MEMORY, "couldn't duplicate BLS 
> filename to compare");
> +  str1 = grub_strdup (nvr1);
> +  if (str1 == NULL)
> +    {
> +      grub_free (str0);
> +      return grub_error (GRUB_ERR_OUT_OF_MEMORY, "couldn't duplicate BLS 
> filename to compare");
> +    }

Do we really need these grub_strdup() calls?

> +  grub_split_package_string (str0, has_name ? &name0 : NULL, &version0, 
> &release0);

s/has_name/(has_name == true)/

> +  grub_split_package_string (str1, has_name ? &name1 : NULL, &version1, 
> &release1);

Ditto.

> +  if (has_name == true)
> +    {
> +      err = grub_vercmp (name0 == NULL ? "" : name0, name1 == NULL ? "" : 
> name1, ret);

This suggests that grub_vercmp() should treat NULLs as empty strings.
Then the code should be nicer and safer.

> +      if (*ret != 0 || err != GRUB_ERR_NONE)
> +     {
> +       grub_free (str0);
> +       grub_free (str1);
> +       return err;
> +     }
> +    }
> +
> +  err = grub_vercmp (version0 == NULL ? "" : version0, version1 == NULL ? "" 
> : version1, ret);

Ditto.

> +  if (*ret != 0 || err != GRUB_ERR_NONE)
> +    {
> +      grub_free (str0);
> +      grub_free (str1);
> +      return err;
> +    }
> +
> +  err = grub_vercmp (release0 == NULL ? "" : release0, release1 == NULL ? "" 
> : release1, ret);

Ditto.

> +  grub_free (str0);
> +  grub_free (str1);
> +  return err;
> +}
> diff --git a/include/grub/vercmp.h b/include/grub/vercmp.h
> new file mode 100644
> index 000000000..e588ef530
> --- /dev/null
> +++ b/include/grub/vercmp.h
> @@ -0,0 +1,35 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2025  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#ifndef GRUB_VERCMP_H
> +#define GRUB_VERCMP_H        1
> +
> +enum
> +  {
> +    GRUB_VERCMP_OLDER        = -1,
> +    GRUB_VERCMP_SAME =  0,
> +    GRUB_VERCMP_NEWER        =  1,
> +  };
> +
> +grub_err_t grub_vercmp (const char *a, const char *b, int *ret);

extern EXPORT_FUNC (grub_vercmp) (const char *a, const char *b, int *ret);

> +
> +void grub_split_package_string (char *package_string, char **name,
> +                             char **version, char **release);
> +
> +grub_err_t grub_split_vercmp (const char *nvr0, const char *nvr1, bool 
> has_name, int *ret);

Do we really need to export these two functions? If yes then my comment
above applies here too.

Daniel