[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] PATCH 2/2: Support <video> element in QEMU driver



On Wed, Jul 08, 2009 at 12:24:42PM +0200, Daniel Veillard wrote:
> On Tue, Jul 07, 2009 at 11:15:15PM +0100, Daniel P. Berrange wrote:
> > This patch extends the QEMU driver to support the <video> element
> > for configuring the type of video adapter exposed to the guest.
> > 
> > It implements the XML -> ARGV and ARGV -> XML conversions, so both
> > import & export work. Some of the existing tests are updated to
> > use the <video> element to get test coverage. The QEMU help parsing
> > test is changed to use symbolic constants instead of black magic
> > hex constants. This supports both old -std-vga and new -vga style
> > QEMU args.
> [...]
> > +    if (def->nvideos) {
> > +        if (def->nvideos > 1) {
> > +            qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> > +                             "%s", _("only one video card is currentely supported"));
> > +            goto error;
> > +        }
> > +
> > +        if (qemuCmdFlags & QEMUD_CMD_FLAG_VGA) {
> > +            if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_XEN) {
> > +                /* nothing - vga has no effect on Xen pvfb */
> > +            } else {
> > +                const char *vgastr = qemuVideoTypeToString(def->videos[0]->type);
> > +                if (!vgastr) {
> > +                    qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> > +                                     _("video type %s is not supported with QEMU"),
> > +                                     virDomainVideoTypeToString(def->videos[0]->type));
> > +                    goto error;
> > +                }
> [...]
> > +            default:
> > +                qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> > +                                 _("video type %s is not supported with QEMU"),
> > +                                 virDomainVideoTypeToString(def->videos[0]->type));
> 
>   It would be a bit nicer to catch those at define time, but that would
> imply duplicating code.

This is arguably true of quite alot of other config options too. We can't
determine whether anything is truely supported though until we actually
run the QEMU emulator and ask for its supported args. I'm not sure this is
necessarily right todo at define time, because we'd need to have this error
checking code here anyway and I think its better not to duplicate it in
two places.


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]