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

[Martin Kletzander]

On Thu, Feb 26, 2015 at 09:57:22AM -0500, Laine Stump wrote:
>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.
>

I think we shouldn't fail parsing live XML when we were the ones who
generated it.  Take a look at device aliases for example.  We parse
them, but users aren't allowed to set them, so we don't parse them
when we are parsing inactive XML.  However, if I remember correctly,
we must be parsing them when parsing e.g. status XML.  So what if this
patch is modified so it behaves depending on flags?

>> 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);
>>                  }
>>              }
>>          }
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150226/2f441519/attachment-0001.sig>