[libvirt] [PATCH] conf: rework code around 'append' attribute to make coverity happy

[John Ferlan]

On 12/26/2015 12:18 PM, Dmitry Mishin wrote:
> Signed-off-by: Dmitry Mishin <dim at virtuozzo.com>
> ---
>  src/conf/domain_conf.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f210c0b..8895e10 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1723,10 +1723,11 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
>  
>      switch (src->type) {
>      case VIR_DOMAIN_CHR_TYPE_FILE:
> -        dest->data.file.append = src->data.file.append;
>      case VIR_DOMAIN_CHR_TYPE_PTY:
>      case VIR_DOMAIN_CHR_TYPE_DEV:
>      case VIR_DOMAIN_CHR_TYPE_PIPE:
> +        if (src->type == VIR_DOMAIN_CHR_TYPE_FILE)
> +            dest->data.file.append = src->data.file.append;
>          if (VIR_STRDUP(dest->data.file.path, src->data.file.path) < 0)
>              return -1;
>          break;
> @@ -9386,12 +9387,12 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>  
>                  switch ((virDomainChrType) def->type) {
>                  case VIR_DOMAIN_CHR_TYPE_FILE:
> -                    if (!append)
> -                        append = virXMLPropString(cur, "append");
>                  case VIR_DOMAIN_CHR_TYPE_PTY:
>                  case VIR_DOMAIN_CHR_TYPE_DEV:
>                  case VIR_DOMAIN_CHR_TYPE_PIPE:
>                  case VIR_DOMAIN_CHR_TYPE_UNIX:
> +                    if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE)
> +                        append = virXMLPropString(cur, "append");
>                      /* PTY path is only parsed from live xml.  */
>                      if (!path  &&
>                          (def->type != VIR_DOMAIN_CHR_TYPE_PTY ||
> @@ -9476,15 +9477,15 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>          break;
>  
>      case VIR_DOMAIN_CHR_TYPE_FILE:
> +    case VIR_DOMAIN_CHR_TYPE_PTY:
> +    case VIR_DOMAIN_CHR_TYPE_DEV:
> +    case VIR_DOMAIN_CHR_TYPE_PIPE:
>          if (append &&

Wouldn't this last one be :

           if (append && def->type == VIR_DOMAIN_CHR_TYPE_FILE &&

>              (def->data.file.append = virTristateSwitchTypeFromString(append)) <= 0) {


NB:

Other uses of def->data.file.append could have used the
virTristateSwitch constants (VIR_TRISTATE_SWITCH_ABSENT,
VIR_TRISTATE_SWITCH_ON, and VIR_TRISTATE_SWITCH_OFF).

That is rather than "if (dev->data.file.append)", it could be "if
(dev->data.file.append != VIR_TRISTATE_SWITCH_ABSENT).  It's just
"safer" that way. Does adding ",append=off" make much sense? Or do you
expect some day in the future that append=on becomes the "default"?

You could have added an example where append='off' - it shouldn't
matter, but it is a legal option and the right XML should be generated
and the right .args would have append=off added to the command line.

John

>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("Invalid append attribute value '%s'"), append);
>              goto error;
>          }
> -    case VIR_DOMAIN_CHR_TYPE_PTY:
> -    case VIR_DOMAIN_CHR_TYPE_DEV:
> -    case VIR_DOMAIN_CHR_TYPE_PIPE:
>          if (!path &&
>              def->type != VIR_DOMAIN_CHR_TYPE_PTY) {
>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>