[libvirt] [Patch]: spice agent-mouse support [v3]
Osier Yang
jyang at redhat.com
Wed Mar 7 08:15:37 UTC 2012
On 03/07/2012 10:02 AM, Zhou Peng wrote:
> On Tue, Mar 6, 2012 at 10:08 PM, Osier Yang<jyang at redhat.com> wrote:
>> On 2012年03月05日 16:17, Zhou Peng wrote:
>>>
>>> Signed-off-by: Zhou Peng<zhoupeng at nfs.iscas.ac.cn>
>>>
>>> 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.
The new introduced XML will make sense for all hypervisor
drivers, (Note that libvirt is a general lib for kinds of
hypervisor drivers), so you should declare it's only for
qemu if really wants to ducument it. E.g.
<snip>
If no rom bar is specified, the qemu default will be used
(older versions of qemu used a default of "off", while newer
qemus have a default of "on").
</snip>
>
>> 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 :-)
We can live with it, either way. :-)
>>
>>
>>> + 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
>
More information about the libvir-list
mailing list