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@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 :|