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

[Christophe Fergeau]

On Tue, Jun 02, 2015 at 04:27:46PM -0500, Jonathon Jongsma wrote:
> > 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.

Thanks for raising these various points, the -1/0/1 was something the
initial version of virt_viewer_compare_version() was doing, and I
initially had both _compare_version() and _compare_buildid() exported,
and did not want to change the behaviour of the former. Now that
_compare_version() is just an internal helper, the various weird bits
that you pointed out can indeed be cleaned up and made simpler.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20150603/ca2fd151/attachment.sig>