[libvirt] [PATCHv2 04/10] libxl: fill HVM SDL and VNC settings based on <graphics/> entries

[Jim Fehlig]

Marek Marczykowski-Górecki wrote:
> On Sat, Feb 07, 2015 at 12:22:51PM +0100, Marek Marczykowski-Górecki wrote:
>   
>> On Thu, Feb 05, 2015 at 02:53:44PM -0700, Jim Fehlig wrote:
>>     
>>> Marek Marczykowski-Górecki wrote:
>>>       
>>>> Vfb entries in domain config are used only by PV drivers. Qemu
>>>> parameters are build based on b_info struct. So fill it with the same
>>>> data as vfb entries (actually the first one).
>>>> This will additionally allow graphic-less domain, when no <graphics/>
>>>> entries are present in domain XML (previously VNC was always enabled).
>>>>   
>>>>         
>>> This should already be handled in libxlMakeVfbList().
>>>       
>> Indeed, I've blindly rebased that patch from older version, but...
>>
>>     
>>> Is there
>>> something missing in that function?
>>>       
>> ... yes, if there is no graphics defined, the driver will set
>> b_info->u.hvm.nographic to 1. Which isn't enough to disable graphic,
>> because actual condition in libxl is:
>>     if (info->nographic && (!info->sdl && !info->vnc)) {
>>         flexarray_append(dm_args, "-nographic");
>>     }
>>
>> I'll think about some better solution (perhaps only part about initially setting
>> vnc and sdl to false?).
>>     
>
> The problem is when domain XML contains no <video/> nor <graphics/>
> elements - libxl defaults then to setting up VNC (on 127.0.0.1). The
> solution here is to disable VNC/SDL and enable only when domain config
> says so. This still make VNC set when only <video/> is present (without
> <graphics type='vnc'/>), but at least there is a way make domain really
> headless.
>
> Below is patch which implement that approach. Note that it makes a
> big change in behaviour - it can make user's configuration broken (if
> he/she relied on this bug).
>   

The current behavior is a regression wrt the legacy Xen driver.  In that
driver, the absence of <video> and <graphics> results in invoking qemu
with -nographic.  Since we aim for compatibility with the old Xen
driver, I'd argue you are fixing the bug :-).

> Disclaimer: I've tested it only with qemu in stubdomain.
>   

I've tested it with a regular HVM domain.

> -----8<-----
> From 37126eea6927f38fcb7f7f1190143fd7de54e9e6 Mon Sep 17 00:00:00 2001
> From: Marek Marczykowski <marmarek at invisiblethingslab.com>
> Date: Sun, 7 Apr 2013 19:48:03 +0200
> Subject: [PATCH] libxl: disable VNC and SDL until explicitly enabled
> Cc: Marek Marczykowski-Górecki <marmarek at invisiblethingslab.com>
>
> Otherwise when VM have no graphics defined, it is still enabled (with
> some libxl defaults).
>   

The commit message should indicate this fixes a regression wrt the
legacy Xen driver.

> Signed-off-by: Marek Marczykowski-Górecki <marmarek at invisiblethingslab.com>
> ---
>  src/libxl/libxl_conf.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 1811a83..7ee4e54 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -745,6 +745,10 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>              return -1;
>          }
>  
> +        /* Disable VNC and SDL until explicitly enabled */
> +        libxl_defbool_set(&b_info->u.hvm.vnc.enable, 0);
> +        libxl_defbool_set(&b_info->u.hvm.sdl.enable, 0);
>   

ACK.  I'll adjust the commit message and push shortly.

Regards,
Jim