Re: [libvirt] [Patch]: spice agent-mouse support [v3]

On 2012年03月05日 16:17, Zhou Peng wrote: > > Signed-off-by: Zhou Peng&lt;zhoupeng(a)nfs.iscas.ac.cn&gt; > > 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

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.

> +        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.

> +        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.

> +        } > + >          /* 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.

> 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.

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. :-)

Regards, Osier