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@xxxxxxxxxx>
> > ---
> > 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list