[Qemu-devel] [PATCH] ui: add x_keymap.o to modules

[Qemu-devel] [PATCH] ui: add x_keymap.o to modules

[Paolo Bonzini]

x_keymap.o is common to the SDL and GTK+ modules, and it causes the
QEMU binary to link to the X11 libraries.  Add it separately to the
modules to keep the main QEMU binary smaller.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 ui/Makefile.objs | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index cc78434..00f6976 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -15,10 +15,6 @@ common-obj-$(CONFIG_COCOA) += cocoa.o
 common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
 common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o
 
-common-obj-$(CONFIG_X11) += x_keymap.o
-x_keymap.o-cflags := $(X11_CFLAGS)
-x_keymap.o-libs := $(X11_LIBS)
-
 # ui-sdl module
 common-obj-$(CONFIG_SDL) += sdl.mo
 ifeq ($(CONFIG_SDLABI),1.2)
@@ -46,6 +42,13 @@ gtk.mo-objs += gtk-gl-area.o
 endif
 endif
 
+ifeq ($(CONFIG_X11),y)
+sdl.mo-objs += x_keymap.o
+gtk.mo-objs += x_keymap.o
+x_keymap.o-cflags := $(X11_CFLAGS)
+x_keymap.o-libs := $(X11_LIBS)
+endif
+
 common-obj-$(CONFIG_CURSES) += curses.mo
 curses.mo-objs := curses.o
 curses.mo-cflags := $(CURSES_CFLAGS)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] ui: add x_keymap.o to modules

[Daniel P. Berrangé]

On Thu, May 17, 2018 at 02:39:42PM +0200, Paolo Bonzini wrote:
> x_keymap.o is common to the SDL and GTK+ modules, and it causes the
> QEMU binary to link to the X11 libraries.  Add it separately to the
> modules to keep the main QEMU binary smaller.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  ui/Makefile.objs | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/ui/Makefile.objs b/ui/Makefile.objs
> index cc78434..00f6976 100644
> --- a/ui/Makefile.objs
> +++ b/ui/Makefile.objs
> @@ -15,10 +15,6 @@ common-obj-$(CONFIG_COCOA) += cocoa.o
>  common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
>  common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o
>  
> -common-obj-$(CONFIG_X11) += x_keymap.o
> -x_keymap.o-cflags := $(X11_CFLAGS)
> -x_keymap.o-libs := $(X11_LIBS)
> -
>  # ui-sdl module
>  common-obj-$(CONFIG_SDL) += sdl.mo
>  ifeq ($(CONFIG_SDLABI),1.2)
> @@ -46,6 +42,13 @@ gtk.mo-objs += gtk-gl-area.o
>  endif
>  endif
>  
> +ifeq ($(CONFIG_X11),y)
> +sdl.mo-objs += x_keymap.o
> +gtk.mo-objs += x_keymap.o

Would this cause symbol clash if both sdl & gtk modules are loaded
at the same time, or have we used linker scripts to limit what symbols
each module exposes ?

> +x_keymap.o-cflags := $(X11_CFLAGS)
> +x_keymap.o-libs := $(X11_LIBS)
> +endif
> +
>  common-obj-$(CONFIG_CURSES) += curses.mo
>  curses.mo-objs := curses.o
>  curses.mo-cflags := $(CURSES_CFLAGS)
> -- 
> 1.8.3.1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] ui: add x_keymap.o to modules

[Paolo Bonzini]

On 17/05/2018 14:44, Daniel P. Berrangé wrote:
>> +ifeq ($(CONFIG_X11),y)
>> +sdl.mo-objs += x_keymap.o
>> +gtk.mo-objs += x_keymap.o
> Would this cause symbol clash if both sdl & gtk modules are loaded
> at the same time, or have we used linker scripts to limit what symbols
> each module exposes ?
> 

We don't, but: 1) the file has only functions and no data; 2) in any
case the symbols are the same, so it is not a real clash.

Adding linker scripts would be a nice improvement, but it is not
necessary for this patch.

Another possibility would be to include x_keymap.c in the files that use
it and make qemu_xkeymap_mapping_table static, but I think it would be
the worst.

Paolo

Re: [Qemu-devel] [PATCH] ui: add x_keymap.o to modules

[Gerd Hoffmann]

  Hi,

> > +ifeq ($(CONFIG_X11),y)
> > +sdl.mo-objs += x_keymap.o
> > +gtk.mo-objs += x_keymap.o
> 
> Would this cause symbol clash if both sdl & gtk modules are loaded
> at the same time, or have we used linker scripts to limit what symbols
> each module exposes ?

Related: can modules depend on modules, so we could make x_keymap a
module of its own and have both gtk and sdl depend on it?

That would also be useful when trying to modularize spice.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] ui: add x_keymap.o to modules

[Daniel P. Berrangé]

On Thu, May 17, 2018 at 02:51:10PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > +ifeq ($(CONFIG_X11),y)
> > > +sdl.mo-objs += x_keymap.o
> > > +gtk.mo-objs += x_keymap.o
> > 
> > Would this cause symbol clash if both sdl & gtk modules are loaded
> > at the same time, or have we used linker scripts to limit what symbols
> > each module exposes ?
> 
> Related: can modules depend on modules, so we could make x_keymap a
> module of its own and have both gtk and sdl depend on it?
> 
> That would also be useful when trying to modularize spice.

Yes, you could create a  xkeymap.so, and link to that from both
sdl.so and gtk.so, so you'll only get one copy of xkeymap.so if
both are loaded.

I don't think it is worth it for this particular case though, since
we'll be deleting the SDL1 code when 2.14 dev cycle opens, leaving
GTK as the only user of it.  SDL2 already has abstracted keycodes.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] ui: add x_keymap.o to modules

[Daniel P. Berrangé]

On Thu, May 17, 2018 at 02:50:04PM +0200, Paolo Bonzini wrote:
> On 17/05/2018 14:44, Daniel P. Berrangé wrote:
> >> +ifeq ($(CONFIG_X11),y)
> >> +sdl.mo-objs += x_keymap.o
> >> +gtk.mo-objs += x_keymap.o
> > Would this cause symbol clash if both sdl & gtk modules are loaded
> > at the same time, or have we used linker scripts to limit what symbols
> > each module exposes ?
> > 
> 
> We don't, but: 1) the file has only functions and no data; 2) in any
> case the symbols are the same, so it is not a real clash.

Ok, as long as dlopen() doesn't whine its fine with me.

> Adding linker scripts would be a nice improvement, but it is not
> necessary for this patch.

Agreed.

> Another possibility would be to include x_keymap.c in the files that use
> it and make qemu_xkeymap_mapping_table static, but I think it would be
> the worst.

It isn't worth trying to be too clever since we're deleting SDL1 in
the 2.14 dev cycle.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] ui: add x_keymap.o to modules

[Paolo Bonzini]

On 17/05/2018 14:51, Gerd Hoffmann wrote:
>   Hi,
> 
>>> +ifeq ($(CONFIG_X11),y)
>>> +sdl.mo-objs += x_keymap.o
>>> +gtk.mo-objs += x_keymap.o
>>
>> Would this cause symbol clash if both sdl & gtk modules are loaded
>> at the same time, or have we used linker scripts to limit what symbols
>> each module exposes ?
> 
> Related: can modules depend on modules, so we could make x_keymap a
> module of its own and have both gtk and sdl depend on it?
> 
> That would also be useful when trying to modularize spice.

How hard would it be to modularize the libspice-server side?  The part
of the library that is used by QXL rendering should have much fewer
dependencies than the part that is used for keyboard, mouse, audio,
vmchannel/agent, etc.

Then you could link libspice-server-core into QEMU and libspice-server
into the modules.  Unless both have been linked together, functions such
as spice_server_add_client would fail, and so would adding most of the
SPICE_INTERFACE_* interface kinds.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] ui: add x_keymap.o to modules

[Gerd Hoffmann]

  Hi,

> > Related: can modules depend on modules, so we could make x_keymap a
> > module of its own and have both gtk and sdl depend on it?
> > 
> > That would also be useful when trying to modularize spice.
> 
> How hard would it be to modularize the libspice-server side?  The part
> of the library that is used by QXL rendering should have much fewer
> dependencies than the part that is used for keyboard, mouse, audio,
> vmchannel/agent, etc.

kbd, mouse, audio is needed on the client side and is not part of
libspice-server anyway.  So spice is much less of a burden compared
to sdl/gtk which bring alot of ui toolkit deps.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] ui: add x_keymap.o to modules

[Paolo Bonzini]

On 17/05/2018 15:45, Gerd Hoffmann wrote:
>   Hi,
> 
>>> Related: can modules depend on modules, so we could make x_keymap a
>>> module of its own and have both gtk and sdl depend on it?
>>>
>>> That would also be useful when trying to modularize spice.
>>
>> How hard would it be to modularize the libspice-server side?  The part
>> of the library that is used by QXL rendering should have much fewer
>> dependencies than the part that is used for keyboard, mouse, audio,
>> vmchannel/agent, etc.
> 
> kbd, mouse, audio is needed on the client side and is not part of
> libspice-server anyway.

Yes, I'm talking about separating the client side from the QXL rendering
part.

>  So spice is much less of a burden compared
> to sdl/gtk which bring alot of ui toolkit deps.

But SPICE does bring in 16 libraries, including both of NSS and OpenSSL...

Paolo

Re: [Qemu-devel] [PATCH] ui: add x_keymap.o to modules

[Gerd Hoffmann]

On Thu, May 17, 2018 at 02:39:42PM +0200, Paolo Bonzini wrote:
> x_keymap.o is common to the SDL and GTK+ modules, and it causes the
> QEMU binary to link to the X11 libraries.  Add it separately to the
> modules to keep the main QEMU binary smaller.

Added to ui patch queue.

thanks,
  Gerd