[virt-tools-list] [RFC virt-viewer 06/12] util: Replace virt_viewer_compare_version with _compare_buildid

Jonathon Jongsma jjongsma at redhat.com
Tue Jun 2 21:27:46 UTC 2015


On Tue, 2015-06-02 at 16:29 +0200, Christophe Fergeau wrote:
> This allows us to do a more accurate version check if the .vv file
> producer wants to allow builds newer than x.y-z because they contain an
> important bug fix.
> ---
>  man/remote-viewer.pod  |  3 +-
>  src/virt-viewer-file.c |  3 +-
>  src/virt-viewer-util.c | 86 ++++++++++++++++++++++++++++++++++++++++----------
>  src/virt-viewer-util.h |  2 +-
>  4 files changed, 74 insertions(+), 20 deletions(-)
> 
> diff --git a/man/remote-viewer.pod b/man/remote-viewer.pod
> index 8910bb0..ba12574 100644
> --- a/man/remote-viewer.pod
> +++ b/man/remote-viewer.pod
> @@ -141,7 +141,8 @@ protocol:
>  If remote-viewer version number isn't greater or equal to the required
>  version, an error is raised with the expected version.
>  
> -The version format accepted is a list of integers separated by '.'.
> +The version format accepted is a list of integers separated by '.'. It can
> +be followed by a dash '-' and an additional build number with the same format.
>  
>  Version comparison is done by comparing each integer from the list one by
>  one. If any of the component is not a number, the version comparison will fail
> diff --git a/src/virt-viewer-file.c b/src/virt-viewer-file.c
> index ce95e18..240a111 100644
> --- a/src/virt-viewer-file.c
> +++ b/src/virt-viewer-file.c
> @@ -796,8 +796,7 @@ virt_viewer_file_check_min_version(VirtViewerFile *self, GError **error)
>      if (min_version == NULL) {
>          return TRUE;
>      }
> -
> -    version_cmp = virt_viewer_compare_version(min_version, PACKAGE_VERSION);
> +    version_cmp = virt_viewer_compare_buildid(min_version, PACKAGE_VERSION BUILDID);
>  
>      if (version_cmp > 0) {
>          g_set_error(error,
> diff --git a/src/virt-viewer-util.c b/src/virt-viewer-util.c
> index aec3b77..e34336a 100644
> --- a/src/virt-viewer-util.c
> +++ b/src/virt-viewer-util.c
> @@ -1,7 +1,7 @@
>  /*
>   * Virt Viewer: A virtual machine console viewer
>   *
> - * Copyright (C) 2007-2012 Red Hat, Inc.
> + * Copyright (C) 2007-2015 Red Hat, Inc.
>   * Copyright (C) 2009-2012 Daniel P. Berrange
>   *
>   * This program is free software; you can redistribute it and/or modify
> @@ -439,41 +439,41 @@ spice_hotkey_to_gtk_accelerator(const gchar *key)
>      return accel;
>  }
>  
> -/**
> - * virt_viewer_compare_version:
> - * @s1: a version-like string
> - * @s2: a version-like string
> - *
> - * Compare two version-like strings: 1.1 > 1.0, 1.0.1 > 1.0, 1.10 > 1.7...
> - *
> - * String suffix (1.0rc1 etc) are not accepted, and will return 0.
> - *
> - * Returns: negative value if s1 < s2; zero if s1 = s2; positive value if s1 > s2.
> - **/
> -gint
> +static gint
>  virt_viewer_compare_version(const gchar *s1, const gchar *s2)
>  {
>      gint i, retval = 0;
>      gchar **v1, **v2;
>  
> -    g_return_val_if_fail(s1 != NULL, 0);
> -    g_return_val_if_fail(s2 != NULL, 0);
> +    g_return_val_if_fail(s1 != NULL, G_MAXINT);
> +    g_return_val_if_fail(s2 != NULL, G_MAXINT);

Hmm, I'm really not a big fan of using "signal" values like this. The
only thing you really gain from using this error signal is a little bit
of added context for the warning by printing a debug output statement in
the calling function. But that doesn't seem like such a big deal to me. 

I also don't see much benefit to normalizing the non-error return values
to -1, 0, or 1. But if you want to, that's fine.

>  
>      v1 = g_strsplit(s1, ".", -1);
>      v2 = g_strsplit(s2, ".", -1);
>  
> +    if ((v1[0] == NULL) || (v2[0] == NULL)) {
> +        retval = G_MAXINT;
> +        goto end;
> +    }
> +
>      for (i = 0; v1[i] && v2[i]; ++i) {
>          gchar *e1 = NULL, *e2 = NULL;
>          guint64 m1 = g_ascii_strtoull(v1[i], &e1, 10);
>          guint64 m2 = g_ascii_strtoull(v2[i], &e2, 10);
>  
>          retval = m1 - m2;
> -        if (retval != 0)
> +        if (retval > 0) {
> +            retval = 1;
>              goto end;
> +        } else if (retval < 0) {
> +            retval = -1;
> +            goto end;
> +        }
>  
>          g_return_val_if_fail(e1 && e2, 0);
>          if (*e1 || *e2) {
>              g_warning("the version string contains suffix");
> +            retval = G_MAXINT;
>              goto end;
>          }
>      }
> @@ -489,6 +489,60 @@ end:
>      return retval;
>  }
>  
> +/**
> + * virt_viewer_compare_buildid:
> + * @s1: a version-like string
> + * @s2: a version-like string
> + *
> + * Compare two buildid strings: 1.1-1 > 1.0-1, 1.0-2 > 1.0-1, 1.10 > 1.7...
> + *
> + * String suffix (1.0rc1 etc) are not accepted, and will return 0.
> + *
> + * Returns: negative value if s1 < s2; zero if s1 = s2; positive value if s1 > s2.
> + **/
> +gint
> +virt_viewer_compare_buildid(const gchar *s1, const gchar *s2)
> +{
> +    int ret = 0;
> +    GStrv split1 = NULL;
> +    GStrv split2 = NULL;
> +
> +    split1 = g_strsplit(s1, "-", 2);
> +    split2 = g_strsplit(s2, "-", 2);
> +    if ((split1 == NULL) || (split2 == NULL)) {
> +      goto end;
> +    }
> +    /* Compare versions */
> +    ret = virt_viewer_compare_version(split1[0], split2[0]);
> +    if (ret == G_MAXINT) {
> +        g_debug("Error comparing version '%s' and '%s''", s1, s2);
> +        ret = 0;
> +        goto end;
> +    } else if (ret != 0) {
> +        goto end;
> +    }
> +    if ((split1[0] == NULL) || (split2[0] == NULL)) {
> +      goto end;
> +    }

It seems a little bit strange to compare split1[0] and split2[0] for
NULL here after you just passed them to virt_viewer_compare_version().
Presumably this check should happen before calling _compare_version().
In any case, it will never be true in normal builds since
_compare_version() returns G_MAXINT if either of the args are NULL. 

Or were you trying to compare split1[1] to NULL here?

> +
> +    /* Compare -release */
> +    ret = virt_viewer_compare_version(split1[1], split2[1]);
> +    if (ret == G_MAXINT) {
> +        g_debug("Error comparing release '%s' and '%s''", s1, s2);
> +        ret = 0;
> +        goto end;
> +    } else if (ret != 0) {
> +        goto end;

These gotos are not strictly necessary since it immediately falls
through to that label anyway.

> +    }
> +
> +end:
> +    g_warn_if_fail((ret == -1) || (ret == 0) || (ret == 1));

Out of curiosity, what are you trying to prevent with this warning? Is
this to catch a (potential) case where you forgot to translate G_MAXINT
to 0? The calling code only checks whether the return value is greater
than 0, so it doesn't care whether the value is exactly -1, 0, or 1.

> +    g_strfreev(split1);
> +    g_strfreev(split2);
> +
> +    return ret;
> +}
> +
>  /* simple sorting of monitors. Primary sort left-to-right, secondary sort from
>   * top-to-bottom, finally by monitor id */
>  static int
> diff --git a/src/virt-viewer-util.h b/src/virt-viewer-util.h
> index 2226bf5..98badd2 100644
> --- a/src/virt-viewer-util.h
> +++ b/src/virt-viewer-util.h
> @@ -54,7 +54,7 @@ gulong virt_viewer_signal_connect_object(gpointer instance,
>                                           GConnectFlags connect_flags);
>  
>  gchar* spice_hotkey_to_gtk_accelerator(const gchar *key);
> -gint virt_viewer_compare_version(const gchar *s1, const gchar *s2);
> +gint virt_viewer_compare_buildid(const gchar *s1, const gchar *s2);
>  
>  /* monitor alignment */
>  void virt_viewer_align_monitors_linear(GdkRectangle *displays, guint ndisplays);





More information about the virt-tools-list mailing list