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.br...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
