[libvirt] [PATCH 03/16] virconf: add typed value accessor methods

[Daniel P. Berrange]

On Thu, Jul 14, 2016 at 11:27:43AM +0200, Andrea Bolognani wrote:
> On Mon, 2016-07-11 at 10:45 +0100, Daniel P. Berrange wrote:
> > Currently many users of virConf APIs are defining the same
> > macros for calling virConfValue() and then doing type
> > checking. To remove this repeated code, add a set of
> > typesafe accessor methods.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> > ---
> >  src/libvirt_private.syms |  10 +
> >  src/util/virconf.c       | 500 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  src/util/virconf.h       |  34 +++-
> >  tests/virconftest.c      | 335 +++++++++++++++++++++++++++++++
> >  4 files changed, 873 insertions(+), 6 deletions(-)
> 
> [...]
> 
> > +/**
> > + * virConfGetValueSSizeT:
> > + * @conf: the config object
> > + * @setting: the config entry name
> > + * @value: pointer to hold integer value
> > + *
> > + * Get the integer value of the config name @setting, storing
> > + * it in @value. If the config entry is not present, then
> > + * @value will be unmodified.
> > + *
> > + * Reports an error if the config entry is set but has
> > + * an unexpected type, or if the value is outside the
> > + * range that can be stored in an 'ssize_t'
> > + *
> > + * Returns: 1 if the value was present, 0 if missing, -1 on error
> > + */
> > +int virConfGetValueSSizeT(virConfPtr conf,
> > +                          const char *setting,
> > +                          ssize_t *value)
> > +{
> > +    virConfValuePtr cval = virConfGetValue(conf, setting);
> > +
> > +    VIR_DEBUG("Get value ssize_t %p %d",
> > +              cval, cval ? cval->type : VIR_CONF_NONE);
> > +
> > +    if (!cval)
> > +        return 0;
> > +
> > +    if (cval->type != VIR_CONF_LONG &&
> > +        cval->type != VIR_CONF_ULONG) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                       _("%s: expected a signed integer for '%s' parameter"),
> > +                       conf->filename, setting);
> > +        return -1;
> > +    }
> > +
> > +    if (cval->l > SSIZE_MAX || cval->l < (-SSIZE_MAX - 1)) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                       _("%s: value for '%s' parameter must be in range %zd:%zd"),
> > +                       conf->filename, setting, -SSIZE_MAX - 1, SSIZE_MAX);
> > +        return -1;
> > +    }
> 
> This seems to have introduced a build failure on CI[1]:
> 
>   ../../src/util/virconf.c: In function 'virConfGetValueSSizeT':
>   ../../src/util/virconf.c:1267:5: error: logical 'or' of collectively
>     exhaustive tests is always true [-Werror=logical-op]

The line in question is

   if (cval->l > SSIZE_MAX || cval->l < (-SSIZE_MAX - 1)) {

If 'signed long long' ans 'ssize_t' are the same size, then
both of these conditions would always be false. So it seems
this is essentially if (0) in that case. The compiler error
message is a little misleading by making it sound as if it
were  if(1) :-)

I guess we need to have some pre-processor check in there
to skip the check when SSIZE_MAX == LONG_MAX.

> I can't reproduce it on my Debian sid builder, though.
> 
> Does this test even make sense on 64 bit architectures? cval->l
> is a long long (8 bytes) and ssize_t is 8 bytes as well, so I
> would expect the error above to pop up when compiling on x86_64,
> if anything.

I've sent a patch which clarifies the range checking by casting
cval->l to a 'unsigned long long' whenever type==VIR_CONF_ULONG.

> Suren, is the libvirt-debian host 32 bit or 64 bit? Can you
> maybe try updating it to rule out a since-solved compiler bug?

We should really aim to fix the warning regardless of compiler
bugs.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|