Re: [libvirt] [PATCH 3/3] libxl: Implement basic video device selection

[Michal Privoznik <mprivozn@redhat.com>]

On 27.03.2014 17:55, Stefan Bader wrote:
> This started as an investigation into an issue where libvirt (using the
> libxl driver) and the Xen host, like an old couple, could not agree on
> who is responsible for selecting the VNC port to use.
>
> Things usually (and a bit surprisingly) did work because, just like that
> old couple, they had the same idea on what to do by default. However it
> was possible that this ended up in a big argument.
>
> The problem is that display information exists in two different places:
> in the vfbs list and in the build info. And for launching the device model,
> only the latter is used. But that never gets initialized from libvirt. So
> Xen allows the device model to select a default port while libvirt thinks
> it has told Xen that this is done by libvirt (though the vfbs config).
>
> While fixing that, I made a stab at actually evaluating the configuration
> of the video device. So that it is now possible to at least decide between
> a Cirrus or standard VGA emulation and to modify the VRAM within certain
> limits using libvirt.
>
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>   src/libxl/libxl_conf.c |   85 ++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 85 insertions(+)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index b8de72a..fdd2726 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1298,6 +1298,82 @@ libxlMakeCapabilities(libxl_ctx *ctx)
>       return NULL;
>   }
>
> +static void
> +libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config)
> +{
> +    libxl_domain_build_info *b_info = &d_config->b_info;
> +
> +    /*
> +     * Take the first defined video device (graphics card) to display
> +     * on the first graphics device (display).
> +     * Right now only type and vram info is used and anything beside
> +     * type xen and vga is mapped to cirrus.
> +     */
> +    if (def->nvideos) {
> +        unsigned int min_vram = 8 * 1024;
> +
> +        switch (def->videos[0]->type) {
> +            case VIR_DOMAIN_VIDEO_TYPE_VGA:
> +            case VIR_DOMAIN_VIDEO_TYPE_XEN:
> +                b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
> +                /*
> +                 * Libxl enforces a minimal VRAM size of 8M when using
> +                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
> +                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
> +                 * Avoid build failures and go with the minimum if less
> +                 * is specified.
> +                 */
> +                switch (b_info->device_model_version) {
> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> +                        min_vram = 8 * 1024;
> +                        break;
> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> +                    default:
> +                        min_vram = 16 * 1024;
> +                }
> +                break;
> +            default:
> +                /*
> +                 * Ignore any other device type and use Cirrus. Again fix
> +                 * up the minimal VRAM to what libxl expects.
> +                 */
> +                b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
> +                switch (b_info->device_model_version) {
> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> +                        min_vram = 4 * 1024; /* Actually the max, too */
> +                        break;
> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> +                    default:
> +                        min_vram = 8 * 1024;
> +                }
> +        }
> +        b_info->video_memkb = (def->videos[0]->vram > min_vram) ?
> +                               def->videos[0]->vram :
> +                               LIBXL_MEMKB_DEFAULT;
> +    } else {
> +        libxl_defbool_set(&b_info->u.hvm.nographic, 1);
> +    }
> +
> +    /*
> +     * When making the list of displays, only VNC and SDL types were
> +     * taken into account. So it seems sensible to connect the default
> +     * video device to the first in the vfb list.
> +     *
> +     * FIXME: Copy the structures and fixing the strings feels a bit dirty.
> +     */
> +    if (d_config->num_vfbs) {
> +        libxl_device_vfb *vfb0 = &d_config->vfbs[0];
> +
> +	b_info->u.hvm.vnc = vfb0->vnc;
> +        VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb0->vnc.listen);
> +        VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb0->vnc.passwd);
> +        b_info->u.hvm.sdl = vfb0->sdl;
> +        VIR_STRDUP(b_info->u.hvm.sdl.display, vfb0->sdl.display);
> +        VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb0->sdl.xauthority);
> +        VIR_STRDUP(b_info->u.hvm.keymap, vfb0->keymap);

You need to check for the return value of VIR_STRDUP(). Moreover, the 
indentation seems off. Looking forward to v2.

Michal