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

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