[libvirt] [PATCH] Ignore listen attribute of <graphics> for type network listens

[Laine Stump]

On 02/26/2015 08:53 AM, Ján Tomko wrote:
> Commit 6992994 started filling the listen attribute
> of the parent <graphics> elements from type='network' listens.
>
> When this XML is passed to UpdateDevice, parsing fails:
> XML error: graphics listen attribute 10.20.30.40 must match
> address attribute of first listen element (found none)


Note that the listen attribute of <graphics> won't be filled in if you
request the *inactive* xml, and so there will be no error when it is fed
back to updatedevice. I can't think of any examples right now, but have
a very definite memory that there are several items in the config like
this - if you feed the status xml output back to update/define "you're
gonna have a bad time". That's why virsh edit asks for the INACTIVE xml.

Did you see this when trying to do an update-device manually, or as the
result of some management application that has forgotten to add the
INACTIVE flag to its request for XML?

If the latter, then I guess I can live with ignoring the error in order
to preserve backward compatibility with the broken application.
Reluctant ACK.

> Ignore the address in the parent <graphics> attribute
> when no type='address' listens are found,
> the same we ignore the address for the <listen> subelements
> when parsing inactive XML.
> ---
>  src/conf/domain_conf.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d95dd3e..d458195 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -9614,12 +9614,16 @@ virDomainGraphicsDefParseXML(xmlNodePtr node,
>                          break;
>                      }
>                  }
> -                if (!matched) {
> +                if (found && !matched) {
>                      virReportError(VIR_ERR_XML_ERROR,
>                                     _("graphics listen attribute %s must match address "
>                                       "attribute of first listen element (found %s)"),
> -                                   listenAddr, found ? found : "none");
> +                                   listenAddr, found);
>                      goto error;
> +                } else if (!found && !matched) {


You are guaranteed that if found == NULL, then match == false, so the
2nd term is redundant.
(you never get into this code if listenAddr == NULL, so unless found !=
NULL the comparison that results in setting match = true will never be
true).

> +                    /* quietly ignore listen address if none of the listens
> +                     * are of type address */
> +                    VIR_FREE(listenAddr);
>                  }
>              }
>          }