On Mon, Apr 6, 2020 at 4:39 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Mon, Apr 06, 2020 at 04:16:48PM +0530, Mishal Shah wrote:
On Mon, Apr 6, 2020 at 3:57 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Mon, Apr 06, 2020 at 03:49:36PM +0530, Mishal Shah wrote:
Fix libvirt build by taking care of NULL string handling
What build problems are you seeing without this patch ? Our automated CI is all working correctly and I don't think the changes make sense.
../../src/openvz/openvz_driver.c: In function 'openvzDomainMigratePrepare3Params': ../../src/util/glibcompat.h:33:26: error: '%s' directive argument is null [-Werror=format-overflow=] 33 | # define g_strdup_printf vir_g_strdup_printf ../../src/openvz/openvz_driver.c:2152:16: note: in expansion of macro 'g_strdup_printf' 2152 | *uri_out = g_strdup_printf("ssh://%s", hostname); | ^~~~~~~~~~~~~~~
This is the error that comes on compiling build. I'm using Ubuntu 18.04 & GCC version 9.2.1
Have you made any local changes to libvirt code, or are you passing any special flags to configure, or custom env ?
We test the build on Ubuntu 18.04 and don't see these warnings
eg this is the latest build log
No, I haven't passed any special flags. The issue is caused because of the GCC version and not Ubuntu version IMO. I found two links to support: - Similar issue: https://bugzilla.redhat.com/show_bug.cgi?id=1670377 - Fix: https://gitlab.gnome.org/GNOME/glib/-/merge_requests/626
--- src/openvz/openvz_driver.c | 6 +++++- tests/sockettest.c | 8 ++++++-- 2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 1a189dbbe7..46f117cd00 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -2084,7 +2084,7 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn, virDomainDefPtr def = NULL; virDomainObjPtr vm = NULL; char *my_hostname = NULL; - const char *hostname = NULL; + char *hostname = NULL;
This string is just a copy of anoter string, so shouldn't have const removed IMHO.
virURIPtr uri = NULL; int ret = -1;
@@ -2149,6 +2149,10 @@
openvzDomainMigratePrepare3Params(virConnectPtr
dconn,
} }
+ if (hostname == NULL) { + goto error; + }
I don't think this is right either - what's missing is that in the previous if() block we should have a "hostname = my_hostname;" assignment.
Yes, I think this assignment should have been there, otherwise, the string hostname stays NULL and throws an error in the build.
*uri_out = g_strdup_printf("ssh://%s", hostname);
ret = 0; diff --git a/tests/sockettest.c b/tests/sockettest.c index 29a565de40..f0a0815b88 100644 --- a/tests/sockettest.c +++ b/tests/sockettest.c @@ -188,8 +188,12 @@ static int testMaskNetwork(const char *addrstr, return -1;
if (STRNEQ(networkstr, gotnet)) { - VIR_FREE(gotnet); - fprintf(stderr, "Expected %s, got %s\n", networkstr,
gotnet);
+ if (gotnet == NULL) { + fprintf(stderr, "Expected %s, got empty string\n", networkstr); + } else { + fprintf(stderr, "Expected %s, got %s\n", networkstr, gotnet); + VIR_FREE(gotnet); + } return -1; }
This makese no sense either, since a few lines above we have
if (!(gotnet = virSocketAddrFormat(&network))) return -1;
so it is impossible for "gotnet == NULL" to be true
Yes, but I think because VIR_FREE was called before the fprintf statement, gotnet became NULL and was throwing an error in the build.
I can make the necessary changes if required.
Oh yes, the order needs to be reversed
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|