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 | 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.

> *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

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

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(a)redhat.com&gt; > 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 https://gitlab.com/libvirt/libvirt/-/jobs/497900982

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