Re: [Qemu-devel] [PATCH] Drop superfluous conditionals around g_strdup()

[Fam Zheng]

On Thu, 12/04 11:39, Markus Armbruster wrote:
> Fam Zheng <address@hidden> writes:
> 
> > On Thu, 12/04 10:26, Markus Armbruster wrote:
> >> Signed-off-by: Markus Armbruster <address@hidden>
> >> ---
> >>  backends/rng-random.c    |  6 +-----
> >>  hw/tpm/tpm_passthrough.c |  4 +---
> >>  util/uri.c               | 43 +++++++++++++++++--------------------------
> >>  3 files changed, 19 insertions(+), 34 deletions(-)
> >> 
> >> diff --git a/backends/rng-random.c b/backends/rng-random.c
> >> index 601d9dc..4f85a8e 100644
> >> --- a/backends/rng-random.c
> >> +++ b/backends/rng-random.c
> >> @@ -88,11 +88,7 @@ static char *rng_random_get_filename(Object *obj, Error 
> >> **errp)
> >>  {
> >>      RndRandom *s = RNG_RANDOM(obj);
> >>  
> >> -    if (s->filename) {
> >> -        return g_strdup(s->filename);
> >> -    }
> >> -
> >> -    return NULL;
> >> +    return g_strdup(s->filename);
> >>  }
> >>  
> >>  static void rng_random_set_filename(Object *obj, const char *filename,
> >> diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
> >> index 56e9e0f..2bf3c6f 100644
> >> --- a/hw/tpm/tpm_passthrough.c
> >> +++ b/hw/tpm/tpm_passthrough.c
> >> @@ -400,9 +400,7 @@ static int tpm_passthrough_handle_device_opts(QemuOpts 
> >> *opts, TPMBackend *tb)
> >>      const char *value;
> >>  
> >>      value = qemu_opt_get(opts, "cancel-path");
> >> -    if (value) {
> >> -        tb->cancel_path = g_strdup(value);
> >> -    }
> >> +    tb->cancel_path = g_strdup(value);
> >>  
> >>      value = qemu_opt_get(opts, "path");
> >>      if (!value) {
> >> diff --git a/util/uri.c b/util/uri.c
> >> index e348c17..bbf2832 100644
> >> --- a/util/uri.c
> >> +++ b/util/uri.c
> >> @@ -1736,24 +1736,21 @@ uri_resolve(const char *uri, const char *base) {
> >>    goto done;
> >>      if ((ref->scheme == NULL) && (ref->path == NULL) &&
> >>    ((ref->authority == NULL) && (ref->server == NULL))) {
> >> -  if (bas->scheme != NULL)
> >> -      res->scheme = g_strdup(bas->scheme);
> >> +        res->scheme = g_strdup(bas->scheme);
> >>    if (bas->authority != NULL)
> >>        res->authority = g_strdup(bas->authority);
> >>    else if (bas->server != NULL) {
> >> -      res->server = g_strdup(bas->server);
> >> -      if (bas->user != NULL)
> >> -          res->user = g_strdup(bas->user);
> >> -      res->port = bas->port;
> >> +            res->server = g_strdup(bas->server);
> >> +            res->user = g_strdup(bas->user);
> >> +            res->port = bas->port;

You fixed indentation for res->server and res->port ...

> >>    }
> >> -  if (bas->path != NULL)
> >> -      res->path = g_strdup(bas->path);
> >> -  if (ref->query != NULL)
> >> +        res->path = g_strdup(bas->path);
> >> +        if (ref->query != NULL) {
> >>        res->query = g_strdup (ref->query);

... but not for res->query.

> >> +        } else {
> >> +            res->query = g_strdup(bas->query);
> >> +        }
> >> +        res->fragment = g_strdup(ref->fragment);
> >>    goto step_7;
> >>      }
> >>  
> >> @@ -1767,13 +1764,10 @@ uri_resolve(const char *uri, const char *base) {
> >>    val = uri_to_string(ref);
> >>    goto done;
> >>      }
> >> -    if (bas->scheme != NULL)
> >> -  res->scheme = g_strdup(bas->scheme);
> >> +    res->scheme = g_strdup(bas->scheme);
> >>  
> >> -    if (ref->query != NULL)
> >> -  res->query = g_strdup(ref->query);
> >> -    if (ref->fragment != NULL)
> >> -  res->fragment = g_strdup(ref->fragment);
> >> +    res->query = g_strdup(ref->query);
> >> +    res->fragment = g_strdup(ref->fragment);
> >>  
> >>      /*
> >>       * 4) If the authority component is defined, then the reference is a
> >> @@ -1787,20 +1781,17 @@ uri_resolve(const char *uri, const char *base) {
> >>        res->authority = g_strdup(ref->authority);
> >>    else {
> >>        res->server = g_strdup(ref->server);
> >> -      if (ref->user != NULL)
> >> -          res->user = g_strdup(ref->user);
> >> +            res->user = g_strdup(ref->user);
> >>              res->port = ref->port;
> >>    }
> >> -  if (ref->path != NULL)
> >> -      res->path = g_strdup(ref->path);
> >> +        res->path = g_strdup(ref->path);
> >>    goto step_7;
> >>      }
> >>      if (bas->authority != NULL)
> >>    res->authority = g_strdup(bas->authority);
> >>      else if (bas->server != NULL) {
> >> -  res->server = g_strdup(bas->server);
> >> -  if (bas->user != NULL)
> >> -      res->user = g_strdup(bas->user);
> >> +        res->server = g_strdup(bas->server);
> >> +        res->user = g_strdup(bas->user);
> >>    res->port = bas->port;

and not for res->port.

> >>      }
> >>  
> >> -- 
> >> 1.9.3
> >> 
> >> 
> >
> > Very confusing tab/whitespace mixture. Code is better, format is worse. I'm 
> > not
> > sure it's a win. :)
> 
> As per standard operating procedure, I expanded tabs in the lines I
> touched.  No visual difference, except in patches.
> 
> What do you want me to do?
> 
> 1. Don't expand tabs, ignore checkpatch.pl whining
> 
> 2. Expand tabs in touched lines (current patch)
> 
> 3. Expand all tabs in uri_resolve() (in a separate patch, of course)
> 
> 4. Expand all tabs in util/uri.c (in a separate patch, of course)
> 

Well, I never know what to do with legacy tabs, perhaps it's not worth
polluting git blame. The code looks good, as a reviewer I'm happy to add my

Reviewed-by: Fam Zheng <address@hidden>

if maintainers are happy with the indentation.

Fam