[libvirt] [PATCH v4 06/14] graphics: resolve address for listen type network in qemu_process

[Cole Robinson]

On 05/19/2016 04:50 PM, Cole Robinson wrote:
> On 05/19/2016 07:35 AM, Pavel Hrdina wrote:
>> Both VNC and SPICE requires the same code to resolve address for listen
>> type network.  Remove code duplication and create a new function that
>> will be used in qemuProcessSetupGraphics().
>>
>> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
>> ---
>>  src/qemu/qemu_command.c | 103 ++++++------------------------------------------
>>  src/qemu/qemu_process.c |  47 +++++++++++++++++++++-
>>  2 files changed, 57 insertions(+), 93 deletions(-)

>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index f4bf6c1..75c8e53 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -4388,6 +4388,32 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver,
>>  
>>  
>>  static int
>> +qemuProcessGraphicsSetupNetworkAddress(virDomainGraphicsListenDefPtr glisten,
>> +                                       const char *listenAddr)
>> +{
>> +    int rc;
>> +
>> +    if (!glisten->network) {
>> +        if (VIR_STRDUP(glisten->address, listenAddr) < 0)
>> +            return -1;
>> +        return 0;
>> +    }
>> +
> 
> This is a logic change. Previously we accept this XML
> 
> <graphics ...>
>   <listen type='network'/>
> </graphics>
> 
> and silently treat that as using vnc_listen/spice_listen. Now we stick that
> address in the XML like
> 
> <graphics ...>
>   <listen type='network' address='$vnc_listen'/>
> </graphics>
> 
> Which at least is more explicit, but it is a logic change. Just shows that the
> address type='network' stuff needs more test coverage at least. I think at
> some point we should reject bare type='network' if it doesn't have a @network
> attribute
> 
> If that change was intentional it should be an additive patch after this
> cleanup, with a test case
> 

Hmm okay I see that it must be intentional, because the qemu_command code now
depends on glisten->address. So ACK to this as long as you add a todo item to
add some test cases for this stuff, and to figure out the bare <listen
type='network'/> weirdness

Thanks,
Cole