On Tue, Mar 6, 2012 at 10:08 PM, Osier Yang <jyang@xxxxxxxxxx> wrote:
> On 2012年03月05日 16:17, Zhou Peng wrote:
>>
>> Signed-off-by: Zhou Peng<zhoupeng@xxxxxxxxxxxxxxx>
>>
>> spice agent-mouse support
>>
>> Usage:
>> <graphics type='spice'>
>> <mouse mode='client'|'server'/>
>> <graphics/>
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 6fcca94..0adf859 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -2809,6 +2809,14 @@ qemu-kvm -net nic,model=? /dev/null
>> to<code>no</code>,<span class="since">since
>> 0.9.3</span>.
>> </p>
>> +<p>
>> + It can be specified whether client or server mouse mode
>> + to use for spice. The default is client which enables
>> + passing mouse events via Spice agent.
>
>
> Not true from the codes, (no default value is set for "mode"). And
It's qemu spice's default. That is, if mouse mode is not specified in qemu argv.
Here it's equal to <graphics type='spice'> element
without specifying <mouse mode=xx/> sub-element.
And IMO passing nothing is consistent and better with qemu
if no <mouse> sub-elem in vm-xml.
> think above lines can be omitted. It's duplicate with below
> somehow. Below is enough.
>
>
>> + The mouse mode is set by the<code>mouse<code/> element,
>> + setting it's<code>mode<code/> attribute to one of
>> +<code>server</code> or<code>client</code>.
>
>
> Better to document since which release the element is introduced.
> e.g. <span class="since">0.9.11</span>
>
>
>> +</p>
>> </dd>
>> <dt><code>"rdp"</code></dt>
>> <dd>
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index 3908733..bb0df03 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -1776,6 +1776,17 @@
>> <empty/>
>> </element>
>> </optional>
>> +<optional>
>> +<element name="mouse">
>> +<attribute name="mode">
>> +<choice>
>> +<value>server</value>
>> +<value>client</value>
>> +</choice>
>> +</attribute>
>> +<empty/>
>> +</element>
>> +</optional>
>> </interleave>
>> </group>
>> <group>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index f9654f1..b99e770 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -460,6 +460,12 @@
>> VIR_ENUM_IMPL(virDomainGraphicsSpicePlaybackCompression,
>> "on",
>> "off");
>>
>> +VIR_ENUM_IMPL(virDomainGraphicsSpiceMouseMode,
>> + VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_LAST,
>> + "default",
>> + "server",
>> + "client");
>> +
>> VIR_ENUM_IMPL(virDomainGraphicsSpiceStreamingMode,
>> VIR_DOMAIN_GRAPHICS_SPICE_STREAMING_MODE_LAST,
>> "default",
>> @@ -5710,6 +5716,26 @@ virDomainGraphicsDefParseXML(xmlNodePtr node,
>> VIR_FREE(copypaste);
>>
>> def->data.spice.copypaste = copypasteVal;
>> + } else if (xmlStrEqual(cur->name, BAD_CAST "mouse")) {
>> + const char *mode = virXMLPropString(cur, "mode");
>> + int modeVal;
>> +
>> + if (!mode) {
>> + virDomainReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("spice mouse missing
>> mode"));
>> + goto error;
>> + }
>> +
>> + if ((modeVal =
>> +
>> virDomainGraphicsSpiceMouseModeTypeFromString(mode))<= 0) {
>> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
>
>
> s/VIR_ERR_INTERNAL_ERROR/VIR_ERR_XML_ERROR/
>
>> + _("unknown mouse mode value
>> '%s'"), mode);
>> + VIR_FREE(mode);
>> + goto error;
>> + }
>> + VIR_FREE(mode);
>> +
>> + def->data.spice.mousemode = modeVal;
>> }
>> }
>> cur = cur->next;
>> @@ -11401,7 +11427,8 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
>> }
>> if (!children&& (def->data.spice.image || def->data.spice.jpeg
>> ||
>>
>> def->data.spice.zlib ||
>> def->data.spice.playback ||
>> - def->data.spice.streaming ||
>> def->data.spice.copypaste)) {
>> + def->data.spice.streaming ||
>> def->data.spice.copypaste ||
>> + def->data.spice.mousemode)) {
>> virBufferAddLit(buf, ">\n");
>> children = 1;
>> }
>> @@ -11420,6 +11447,9 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
>> if (def->data.spice.streaming)
>> virBufferAsprintf(buf, "<streaming mode='%s'/>\n",
>>
>>
>> virDomainGraphicsSpiceStreamingModeTypeToString(def->data.spice.streaming));
>> + if (def->data.spice.mousemode)
>> + virBufferAsprintf(buf, "<mouse mode='%s'/>\n",
>> +
>> virDomainGraphicsSpiceMouseModeTypeToString(def->data.spice.mousemode));
>> if (def->data.spice.copypaste)
>> virBufferAsprintf(buf, "<clipboard copypaste='%s'/>\n",
>>
>>
>> virDomainGraphicsSpiceClipboardCopypasteTypeToString(def->data.spice.copypaste));
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 596be4d..a9c118a 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -1003,6 +1003,14 @@ enum virDomainGraphicsSpicePlaybackCompression {
>> VIR_DOMAIN_GRAPHICS_SPICE_PLAYBACK_COMPRESSION_LAST
>> };
>>
>> +enum virDomainGraphicsSpiceMouseMode {
>> + VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_DEFAULT = 0,
>> + VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_SERVER,
>> + VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT,
>> +
>> + VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_LAST
>> +};
>> +
>> enum virDomainGraphicsSpiceStreamingMode {
>> VIR_DOMAIN_GRAPHICS_SPICE_STREAMING_MODE_DEFAULT = 0,
>> VIR_DOMAIN_GRAPHICS_SPICE_STREAMING_MODE_FILTER,
>> @@ -1072,6 +1080,7 @@ struct _virDomainGraphicsDef {
>> struct {
>> int port;
>> int tlsPort;
>> + int mousemode;
>> char *keymap;
>> virDomainGraphicsAuthDef auth;
>> unsigned int autoport :1;
>> @@ -2061,6 +2070,7 @@ VIR_ENUM_DECL(virDomainGraphicsSpiceZlibCompression)
>> VIR_ENUM_DECL(virDomainGraphicsSpicePlaybackCompression)
>> VIR_ENUM_DECL(virDomainGraphicsSpiceStreamingMode)
>> VIR_ENUM_DECL(virDomainGraphicsSpiceClipboardCopypaste)
>> +VIR_ENUM_DECL(virDomainGraphicsSpiceMouseMode)
>> VIR_ENUM_DECL(virDomainNumatuneMemMode)
>> VIR_ENUM_DECL(virDomainSnapshotState)
>> /* from libvirt.h */
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index b9baf9a..ccaa72d 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -252,6 +252,8 @@ virDomainChrDefFree;
>> virDomainChrDefNew;
>> virDomainChrSourceDefCopy;
>> virDomainChrSourceDefFree;
>> +virDomainGraphicsSpiceMouseModeTypeFromString;
>> +virDomainGraphicsSpiceMouseModeTypeToString;
>> virDomainChrSpicevmcTypeFromString;
>> virDomainChrSpicevmcTypeToString;
>> virDomainChrTcpProtocolTypeFromString;
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index 64a4546..2bbc077 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -154,6 +154,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
>> "drive-iotune", /* 85 */
>> "system_wakeup",
>> "scsi-disk.channel",
>> + "spice-agent-mouse",
>> );
>>
>> struct qemu_feature_flags {
>> @@ -1049,8 +1050,13 @@ qemuCapsComputeCmdFlags(const char *help,
>> if ((p = strstr(p, "|none"))&& p< nl)
>>
>> qemuCapsSet(flags, QEMU_CAPS_VGA_NONE);
>> }
>> - if (strstr(help, "-spice"))
>> + if (strstr(help, "-spice")) {
>> qemuCapsSet(flags, QEMU_CAPS_SPICE);
>> +
>> + /* If SPICE is avail, agent-mouse option will be avail for qemu,
>> + * although 'qemu --help' doesn't show it. */
>
>
> So we don't need it, no reason to have a redundant flag.
OK I will del QEMU_CAPS_SPICE_AGENTMOUSE related code.
>
>
>> + qemuCapsSet(flags, QEMU_CAPS_SPICE_AGENTMOUSE);
>> + }
>> if (strstr(help, "boot=on"))
>> qemuCapsSet(flags, QEMU_CAPS_DRIVE_BOOT);
>> if (strstr(help, "serial=s"))
>> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
>> index db584ce..2e9faba 100644
>> --- a/src/qemu/qemu_capabilities.h
>> +++ b/src/qemu/qemu_capabilities.h
>> @@ -122,6 +122,7 @@ enum qemuCapsFlags {
>> QEMU_CAPS_DRIVE_IOTUNE = 85, /* -drive bps= and friends */
>> QEMU_CAPS_WAKEUP = 86, /* system_wakeup monitor command
>> */
>> QEMU_CAPS_SCSI_DISK_CHANNEL = 87, /* Is scsi-disk.channel available?
>> */
>> + QEMU_CAPS_SPICE_AGENTMOUSE = 88, /* -spice agent-mouse=on|off */
>>
>> QEMU_CAPS_LAST, /* this must always be the last
>> item */
>> };
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 01adf0d..b5fc5b1 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -5391,6 +5391,23 @@ qemuBuildCommandLine(virConnectPtr conn,
>>
>> VIR_FREE(netAddr);
>>
>> + int mm = def->graphics[0]->data.spice.mousemode;
>
>
> Generally we want the declaration of a variable at the head of block.
I noticed first and thought it libvirt's code style
but at last I found this is not a consistent and compulsive style for
libvirt's source
and IMO I think it's more readable this way for 'mm' used only once but not
scattering in qemuBuildCommandLine :-)
>
>
>> + if (mm) {
>> + if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SPICE_AGENTMOUSE)) {
>> + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("spice agent-mouse is not supported
>> with this QEMU"));
>> + goto error;
>> + }
>
>
> And this is no need too. There was checking on QEMU_CAPS_SPICE
> early before this hunk.
>
>
>> + switch (mm) {
>> + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_SERVER:
>> + virBufferAsprintf(&opt, ",agent-mouse=off");
>> + break;
>> + case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT:
>> + virBufferAsprintf(&opt, ",agent-mouse=on");
>> + break;
>> + }
>
>
> As a habit, you'd want the "default" branch, though it's unlikely
> be hitted as there was checking while parsing currently, and who
> knowns if a new possiable value for mode will be added in future,
> and qemu driver doesn't support it.
It's very sure invalid mouse mode never be here becasue checked before.
I think, caller and callee don't need to check twice.
IMO, If new mouse mode added in the future,
a new case must be appended here too.
And whether qemu driver is consistent it's QEMU_CAPS_XX's responsiblity
(pls correct if any err)
It's like 'switch (disk->device)' and etc in qemu_command.c
But never mind to add
default:
break;
>
>
>> + }
>> +
>> /* In the password case we set it via monitor command, to avoid
>> * making it visible on CLI, so there's no use of password=XXX
>> * in this bit of the code */
>> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-spice-agentmouse.args
>> b/tests/qemuxml2argvdata/qemuxml2argv-spice-agentmouse.args
>> new file mode 100644
>> index 0000000..46be6d8
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-spice-agentmouse.args
>> @@ -0,0 +1,19 @@
>> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
>> QEMU_AUDIO_DRV=spice \
>> +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nodefconfig -nodefaults \
>> +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -device \
>> +virtio-serial-pci,id=virtio-serial1,bus=pci.0,addr=0xa -hda \
>> +/dev/HostVG/QEMUGuest1 -chardev spicevmc,id=charchannel0,name=vdagent
>> -device \
>>
>> +virtserialport,bus=virtio-serial1.0,nr=3,chardev=charchannel0,id=channel0\
>> +,name=com.redhat.spice.0 -usb -spice
>> port=5903,tls-port=5904,addr=127.0.0.1,\
>> +agent-mouse=off,x509-dir=/etc/pki/libvirt-spice,tls-channel=main -device
>> \
>> +virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
>> +
>> +#LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test
>> QEMU_AUDIO_DRV=spice \
>> +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nodefconfig -nodefaults \
>> +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -device \
>> +virtio-serial-pci,id=virtio-serial1,bus=pci.0,addr=0xa -hda \
>> +/dev/HostVG/QEMUGuest1 -chardev spicevmc,id=charchannel0,name=vdagent
>> -device \
>>
>> +virtserialport,bus=virtio-serial1.0,nr=3,chardev=charchannel0,id=channel0\
>> +,name=com.redhat.spice.0 -usb -spice
>> port=5903,tls-port=5904,addr=127.0.0.1,\
>> +agent-mouse=on,x509-dir=/etc/pki/libvirt-spice,tls-channel=main -device \
>> +virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
>
>
> Ah, not good, guess you want to switch the testing between mode="off"
> and mode="on". I even not sure if "make check" passes with this,
> very possiably not, and you won't get what you want, as nobody would
> like to edit the tests to cover the both tests.
OK I will split in two file.
>
>
>> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-spice-agentmouse.xml
>> b/tests/qemuxml2argvdata/qemuxml2argv-spice-agentmouse.xml
>> new file mode 100644
>> index 0000000..5ab7c5e
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-spice-agentmouse.xml
>> @@ -0,0 +1,40 @@
>> +<domain type='qemu'>
>> +<name>QEMUGuest1</name>
>> +<uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>> +<memory>219136</memory>
>> +<vcpu cpuset='1-4,8-20,525'>1</vcpu>
>> +<os>
>> +<type arch='i686' machine='pc'>hvm</type>
>> +<boot dev='hd'/>
>> +</os>
>> +<clock offset='utc'/>
>> +<on_poweroff>destroy</on_poweroff>
>> +<on_reboot>restart</on_reboot>
>> +<on_crash>destroy</on_crash>
>> +<devices>
>> +<emulator>/usr/bin/qemu</emulator>
>> +<disk type='block' device='disk'>
>> +<source dev='/dev/HostVG/QEMUGuest1'/>
>> +<target dev='hda' bus='ide'/>
>> +<address type='drive' controller='0' bus='0' target='0' unit='0'/>
>> +</disk>
>> +<controller type='usb' index='0'/>
>> +<controller type='ide' index='0'/>
>> +<controller type='virtio-serial' index='1'>
>> +<address type='pci' domain='0x0000' bus='0x00' slot='0x0a'
>> function='0x0'/>
>> +</controller>
>> +<!--
>> +<graphics type='spice' port='5903' tlsPort='5904' autoport='no'
>> listen='127.0.0.1'>
>> +<mouse mode='client'/>
>> +-->
>
>
> Hum, this proved my guess. You can remove these comments directly.
> As we just want to see if the XML is parsed/formated, and command
> line is built correctly, not really check how the guest will act in
> "on|off" mode.
I misunderstand your review in this patch's first revision.
I thought you tell me to cover all test case.
> And by the way, would you mind posting the patch using git send-email?
> text diff can be re-indented by thunderbird in a crazy way. :-)
OK.
Thank you for your review.
> Regards,
> Osier
--
Zhou Peng
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list