Libguestfs - May 2016 - Re: [PATCH 1/2] src: start unifying version handling

On Tue, May 17, 2016 at 03:41:10PM +0200, Pino Toscano
wrote:
> +extern bool guestfs_int_version_is (const struct version *v, int maj, int
min, int mic);
I think calling this function "is" is a bit misleading.  I think
we should have _ge and _le functions (cf my patch).

This comparison for example is wrong:
>    /* qemu 1.1 claims to support virtio-scsi but in reality it's
broken. */
> -  if (data->qemu_version_major == 1 &&
data->qemu_version_minor < 2)
> +  if (!guestfs_int_version_is (&data->qemu_version, 1, 2, 0))
>      return 1;
However the idea is sound.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Tuesday 17 May 2016 15:45:42 Richard W.M. Jones
wrote:
> On Tue, May 17, 2016 at 03:41:10PM +0200, Pino Toscano wrote:
> > +extern bool guestfs_int_version_is (const struct version *v, int maj,
int min, int mic);
> 
> I think calling this function "is" is a bit misleading.  I think
> we should have _ge and _le functions (cf my patch).
Makes sense, renamed to _ge.
> This comparison for example is wrong:
> 
> >    /* qemu 1.1 claims to support virtio-scsi but in reality it's
broken. */
> > -  if (data->qemu_version_major == 1 &&
data->qemu_version_minor < 2)
> > +  if (!guestfs_int_version_is (&data->qemu_version, 1, 2, 0))
> >      return 1;
The older code was basically checking for versions in the interval
[1, 2[, while the change turns that into a [0, 2[.  Considering the
qemu version supported is >= 1.0 anyway, the final effect is the same.
Am I missing anything?

Thanks,
-- 
Pino Toscano

On Wed, May 18, 2016 at 12:27:56PM +0200, Pino Toscano
wrote:
> On Tuesday 17 May 2016 15:45:42 Richard W.M. Jones wrote:
> > On Tue, May 17, 2016 at 03:41:10PM +0200, Pino Toscano wrote:
> > > +extern bool guestfs_int_version_is (const struct version *v, int
maj, int min, int mic);
> > 
> > I think calling this function "is" is a bit misleading.  I
think
> > we should have _ge and _le functions (cf my patch).
> 
> Makes sense, renamed to _ge.
> 
> > This comparison for example is wrong:
> > 
> > >    /* qemu 1.1 claims to support virtio-scsi but in reality
it's broken. */
> > > -  if (data->qemu_version_major == 1 &&
data->qemu_version_minor < 2)
> > > +  if (!guestfs_int_version_is (&data->qemu_version, 1, 2,
0))
> > >      return 1;
> 
> The older code was basically checking for versions in the interval
> [1, 2[, while the change turns that into a [0, 2[.  Considering the
> qemu version supported is >= 1.0 anyway, the final effect is the same.
> Am I missing anything?
No, I was missing the negation.

Is there a v2 of this patch?  I'm keen to get it reviewed and applied
upstream so I can work on the qemu memoization changes on top of it.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org