[Spice-devel] [PATCH spice-streaming-agent 5/5] Provide helper to give information to daemon

[Lukáš Hrázký]

On Fri, 2018-02-02 at 13:06 -0500, Frediano Ziglio wrote:
> > 
> > On Thu, 2018-02-01 at 16:40 +0000, Frediano Ziglio wrote:
> > > This is required so the daemon can manage the various sessions.
> > 
> > At first glance, this looks like a really big hack. At second glance, I
> > understand why you did it... (doesn't mean I like it now ;)) But in any
> > case this deserves a much better description... well, preferably in the
> > code, not (only) commit message.
> > 
> > Also, I would really like to think of a better way of doing this. Seems
> > the streaming agent is starting to have quite a lot of moving parts...
> > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  data/spice-streaming.desktop.in |  3 +--
> > >  spice-streaming-agent.spec.in   |  1 +
> > >  src/Makefile.am                 |  5 +++++
> > >  src/spice-streaming-helper      | 24 ++++++++++++++++++++++++
> > >  4 files changed, 31 insertions(+), 2 deletions(-)
> > >  create mode 100755 src/spice-streaming-helper
> > > 
> > > diff --git a/data/spice-streaming.desktop.in
> > > b/data/spice-streaming.desktop.in
> > > index dcb30a3..5e4e863 100644
> > > --- a/data/spice-streaming.desktop.in
> > > +++ b/data/spice-streaming.desktop.in
> > > @@ -1,8 +1,7 @@
> > >  [Desktop Entry]
> > >  Name=SPICE Streaming Agent
> > >  Comment=Agent for Streaming the framebuffer to Spice
> > > -Exec=@BINDIR@/spice-streaming-agent
> > > -#TryExec=/usr/bin/spice-streaming-agent
> > > +Exec=@BINDIR@/spice-streaming-helper
> > >  Terminal=false
> > >  Type=Application
> > >  Categories=
> > > diff --git a/spice-streaming-agent.spec.in b/spice-streaming-agent.spec.in
> > > index eef8f90..54a3598 100644
> > > --- a/spice-streaming-agent.spec.in
> > > +++ b/spice-streaming-agent.spec.in
> > > @@ -54,6 +54,7 @@ fi
> > >  %doc COPYING ChangeLog README
> > >  %{_udevrulesdir}/90-spice-guest-streaming.rules
> > >  %{_bindir}/spice-streaming-agent
> > > +%{_bindir}/spice-streaming-helper
> > >  %{_sysconfdir}/xdg/autostart/spice-streaming.desktop
> > >  %{_datadir}/gdm/greeter/autostart/spice-streaming.desktop
> > >  /usr/lib/systemd/system/spice-streaming-agent.socket
> > 
> > Does this add dependency on Python 3? Should it be added somewhere in
> > this file?
> > 
> 
> Usually the RPM script are smart enough nowadays to understand.
> I'll check.
> 
> > > diff --git a/src/Makefile.am b/src/Makefile.am
> > > index d2f8a47..9e9c7e7 100644
> > > --- a/src/Makefile.am
> > > +++ b/src/Makefile.am
> > > @@ -5,6 +5,7 @@
> > >  
> > >  NULL =
> > >  SUBDIRS = . unittests
> > > +EXTRA_DIST =
> > >  
> > >  AM_CPPFLAGS = \
> > >  	-DSPICE_STREAMING_AGENT_PROGRAM \
> > > @@ -61,3 +62,7 @@ spice_streaming_agent_SOURCES = \
> > >  	daemon.cpp \
> > >  	daemon.h \
> > >  	$(NULL)
> > > +
> > > +helperdir = $(bindir)
> > > +helper_SCRIPTS = spice-streaming-helper
> > > +EXTRA_DIST += spice-streaming-helper
> > > diff --git a/src/spice-streaming-helper b/src/spice-streaming-helper
> > > new file mode 100755
> > > index 0000000..beba559
> > > --- /dev/null
> > > +++ b/src/spice-streaming-helper
> > > @@ -0,0 +1,24 @@
> > > +#!/usr/bin/env python3
> > > +import os
> > > +import socket
> > > +import sys
> > > +import struct
> > > +
> > 
> > I am not sure how mandatory this actually is in Python, but I would
> > prefer to have this here:
> > if __name__ == "__main__":
> >     main()
> > 
> 
> not mandatory, helps for bigger programs or in a library.
> 
> > > +sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
> > > +
> > > +server_address = "\0spice-streaming-agent-daemon"
> > > +def code(var, type):
> > 
> > Like... it's pretty obvious and the code is local here, but still...
> > name the function properly? :)
> > 
> 
> encode_env ?

Sure :)

> > > +    val = os.environ.get(var, '').encode('utf-8')
> > > +    return struct.pack('BB', type, len(val)) + val
> > > +
> > > +display = code('DISPLAY', 1)
> > > +xauthority = code('XAUTHORITY', 2)
> > > +
> > > +try:
> > > +    sock.connect(server_address)
> > > +    data = struct.pack('B', 1) + display + xauthority
> > > +    sock.send(data)
> > > +    sock.recv(10)
> > 
> > What's the recv(10) for, actually?
> > 
> 
> To wait for reply or disconnection.
> Yes, not quickly understandable, better to put a comment.

But there is no reply, right? So why do you need to wait?

Lukas

> > > +except socket.error as msg:
> > > +    print(msg, file=sys.stderr)
> > > +    sys.exit(1)