Re: [libvirt] [PATCH 2/4] Add a <graphics> type for SPICE protocol

On Mon, Oct 19, 2009 at 04:52:10PM +0100, Daniel P. Berrange wrote: > On Mon, Oct 19, 2009 at 05:47:47PM +0200, Dan Kenigsberg wrote: > > Thanks for the patches (and sorry for the late response). > > > > The patches are fine, though I am still missing a means to control which > > of spice channels are to be encrypted. Also missing is a way to set > > key/cert files and cipher suite per domain. (I aksed spice folks to > > avoid this problem by using reasonable defaults). More of a problem is > > the need to set/reset spice "ticket" (one time password), or at least > > disable it completely on the command line (with ,disable-ticketing). > > For VNC, we leave key/cert file configuration to be done per-host > in /etc/libvirt/qemu.conf. I was anticipating the same for SPICE, > with a spice_tls_x509_cert_dir parameter, to match the existing > vnc_tls_x509_cert_dir parameter. We could easily add cipher suite > parameters to the config too if there's a compelling need to use a > non-default setting. > > IIRC, there's an open RFE for the 'ticket' stuff already. you are correct. > > > On Tue, Sep 29, 2009 at 04:43:51PM +0100, Daniel P. Berrange wrote: > > > This supports the -qxl argument in RHEL-5's fork of KVM > > > which has SPICE support. QXL is a graphics card, but > > > inexplicably doesn't use the standard -vga syntax for > > > generic configuration. Also -qxl is rather useless unless > > > you also supply -spice (coming in next patch) > > > > > + > > > + if (virAsprintf(&optstr, "%u,ram=%u", > > > + def->videos[0]->heads, > > > + (def->videos[0]->vram /1024)) < 0) > > > + goto no_memory; > > > > this hides spice's own default, and sends ",ram=0" if xml lacks vram > > attribute. I think it would be better to drop ",ram" completely if > > vram==0. > > hmm, I missed something somewhere then, because our XML parser should > always set a default vram value if it is omitted, so you should never > get a ram=0 flag. What I am saying is that hiding spice's default with adding diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 27fcdf1..67b9e2a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1798,6 +1798,9 @@ virDomainVideoDefaultRAM(virDomainDefPtr def, /* Original Xen PVFB hardcoded to 4 MB */ return 4 * 1024; + case VIR_DOMAIN_VIDEO_TYPE_QXL: + return 64 * 1024; + default: return 0; } seems suboptimal to me.