On Mon, Apr 6, 2020 at 4:39 PM Daniel P. Berrangé <berra...@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é <berra...@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 > > https://gitlab.com/libvirt/libvirt/-/jobs/497900982 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 :| > >