[libvirt] [PATCH v4 07/13] Adapt to VIR_STRDUP and VIR_STRNDUP in src/rpc/*

[John Ferlan]

On 05/20/2013 01:55 PM, Michal Privoznik wrote:
> ---
>  src/rpc/gendispatch.pl       | 21 ++++--------
>  src/rpc/virnetclient.c       | 16 ++++-----
>  src/rpc/virnetmessage.c      | 27 +++++++++------
>  src/rpc/virnetsaslcontext.c  |  6 ++--
>  src/rpc/virnetserver.c       |  6 ++--
>  src/rpc/virnetserverclient.c | 10 ++----
>  src/rpc/virnetservermdns.c   |  6 ++--
>  src/rpc/virnetsocket.c       | 10 +++---
>  src/rpc/virnetsshsession.c   | 78 +++++++++++++++++++++-----------------------
>  src/rpc/virnettlscontext.c   | 26 +++++++--------
>  10 files changed, 92 insertions(+), 114 deletions(-)
> 
> diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
[...snip...]

> diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c
> index b2c6e5b..8483cd5 100644
> --- a/src/rpc/virnetmessage.c
> +++ b/src/rpc/virnetmessage.c
> @@ -29,6 +29,7 @@
>  #include "virlog.h"
>  #include "virfile.h"
>  #include "virutil.h"
> +#include "virstring.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_RPC
>  
> @@ -514,22 +515,28 @@ void virNetMessageSaveError(virNetMessageErrorPtr rerr)
>      if (verr) {
>          rerr->code = verr->code;
>          rerr->domain = verr->domain;
> -        if (verr->message && VIR_ALLOC(rerr->message) == 0)
> -            *rerr->message = strdup(verr->message);
> +        if (verr->message && VIR_ALLOC(rerr->message) == 0 &&
> +            VIR_STRDUP_QUIET(*rerr->message, verr->message) < 0)
> +            VIR_FREE(rerr->message);
>          rerr->level = verr->level;
> -        if (verr->str1 && VIR_ALLOC(rerr->str1) == 0)
> -            *rerr->str1 = strdup(verr->str1);
> -        if (verr->str2 && VIR_ALLOC(rerr->str2) == 0)
> -            *rerr->str2 = strdup(verr->str2);
> -        if (verr->str3 && VIR_ALLOC(rerr->str3) == 0)
> -            *rerr->str3 = strdup(verr->str3);
> +        if (verr->str1 && VIR_ALLOC(rerr->str1) == 0 &&
> +            VIR_STRDUP_QUIET(*rerr->str1, verr->str1) < 0)
> +            VIR_FREE(verr->str1);
> +        if (verr->str2 && VIR_ALLOC(rerr->str2) == 0 &&
> +            VIR_STRDUP_QUIET(*rerr->str2, verr->str2) < 0)
> +            VIR_FREE(verr->str2);
> +        if (verr->str3 && VIR_ALLOC(rerr->str3) == 0 &&
> +            VIR_STRDUP_QUIET(*rerr->str3, verr->str3) < 0)
> +            VIR_FREE(verr->str2);

Coverity has a complaint in here:
525  	        if (verr->str2 && VIR_ALLOC(rerr->str2) == 0 &&
526  	            VIR_STRDUP_QUIET(*rerr->str2, verr->str2) < 0)

(1) Event original: 	"verr->str2" looks like the original copy.
Also see events: 	[copy_paste_error]

527  	            VIR_FREE(verr->str2);
528  	        if (verr->str3 && VIR_ALLOC(rerr->str3) == 0 &&
529  	            VIR_STRDUP_QUIET(*rerr->str3, verr->str3) < 0)

(2) Event copy_paste_error: 	"str2" in "verr->str2" looks like a
copy-paste error. Should it say "str3" instead?
Also see events: 	[original]

530  	            VIR_FREE(verr->str2);
531  	        rerr->int1 = verr->int1;
532  	        rerr->int2 = verr->int2;



The complaint is only on the last VIR_FREE(verr->str2); as a cut-n-paste
of the one on line 527; however, it seems to me all those VIR_FREE()'s
should be on "rerr" and not "verr", right?

That is, there's a VIR_ALLOC(rerr->str#) for each section, then on
failure shouldn't it be VIR_FREE(rerr->str#)?

John


>          rerr->int1 = verr->int1;
>          rerr->int2 = verr->int2;
>      } else {
>          rerr->code = VIR_ERR_INTERNAL_ERROR;
>          rerr->domain = VIR_FROM_RPC;
> -        if (VIR_ALLOC(rerr->message) == 0)
> -            *rerr->message = strdup(_("Library function returned error but did not set virError"));
> +        if (VIR_ALLOC(rerr->message) == 0 &&
> +            VIR_STRDUP_QUIET(*rerr->message,
> +                             _("Library function returned error but did not set virError")) < 0)
> +            VIR_FREE(rerr->message);
>          rerr->level = VIR_ERR_ERROR;
>      }
>  }
[...snip...]