Xen devel - Sep 2008 - [Xen-ia64-devel] [PATCH] xm: Don''t spawn vncviewer twice.

Hi. I found this issue with IA64 box while looking at the c/s of
18204:21dd1fdb73d8, but I believe the things is same with x86 box.
I''m not sure whether this patch coexist with the old ioemu which
is to be removed soon. So it might be desirable to apply
this patch after deleting the internal ioemu tree.

thanks,

xm: Don''t spawn vncviewer twice.
Without this patch, vncviewer process remain as follows.
> # pgrep -fl vnc
> 4303 vncviewer -log *:stdout:0 -listen 5501
> 5089 vncviewer -log *:stdout:0 -listen 5502
> 5763 vncviewer -log *:stdout:0 -listen 5503
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

diff -r 2571ec1ddde1 tools/python/xen/xm/create.py
--- a/tools/python/xen/xm/create.py	Tue Sep 16 22:01:28 2008 +0900
+++ b/tools/python/xen/xm/create.py	Wed Sep 17 16:57:45 2008 +0900
@@ -1116,39 +1116,7 @@
         if port in ports: continue
         return d
     return None
-vncpid = None
 
-def spawn_vnc(display):
-    """Spawns a vncviewer that listens on the specified display.
On success,
-    returns the port that the vncviewer is listening on and sets the global
-    vncpid.  On failure, returns 0.  Note that vncviewer is daemonized.
-    """
-    vncargs = (["vncviewer", "-log",
"*:stdout:0",
-            "-listen", "%d" % (VNC_BASE_PORT + display) ])
-    global vncpid
-    vncpid = utils.daemonize("vncviewer", vncargs)
-    if vncpid == 0:
-        return 0
-
-    return VNC_BASE_PORT + display
-
-def preprocess_vnc(vals):
-    """If vnc was specified, spawn a vncviewer in listen mode
-    and pass its address to the domain on the kernel command line.
-    """
-    if vals.dryrun: return
-    if vals.vncviewer:
-        vnc_display = choose_vnc_display()
-        if not vnc_display:
-            warn("No free vnc display")
-            return
-        print ''VNC='', vnc_display
-        vnc_port = spawn_vnc(vnc_display)
-        if vnc_port > 0:
-            vnc_host = get_host_addr()
-            vnc = ''VNC_VIEWER=%s:%d'' % (vnc_host, vnc_port)
-            vals.extra = vnc + '' '' + vals.extra
-    
 def preprocess(vals):
     preprocess_disk(vals)
     preprocess_pci(vals)
@@ -1156,7 +1124,6 @@
     preprocess_ioports(vals)
     preprocess_ip(vals)
     preprocess_nfs(vals)
-    preprocess_vnc(vals)
     preprocess_vtpm(vals)
     preprocess_access_control(vals)
     preprocess_cpuid(vals, ''cpuid'')
@@ -1193,23 +1160,10 @@
     try:
         dominfo = server.xend.domain.create(config)
     except xmlrpclib.Fault, ex:
-        import signal
-        if vncpid:
-            os.kill(vncpid, signal.SIGKILL)
         if ex.faultCode == xen.xend.XendClient.ERROR_INVALID_DOMAIN:
             err("the domain ''%s'' does not exist." %
ex.faultString)
         else:
             err("%s" % ex.faultString)
-    except Exception, ex:
-        # main.py has good error messages that let the user know what failed.
-        # unless the error is a create.py specific thing, it should be handled
-        # at main. The purpose of this general-case
''Exception'' handler is to
-        # clean up create.py specific processes/data but since create.py does
-        # not know what to do with the error, it should pass it up.
-        import signal
-        if vncpid:
-            os.kill(vncpid, signal.SIGKILL)
-        raise
 
     dom = sxp.child_value(dominfo, ''name'')
 
diff -r 2571ec1ddde1 tools/python/xen/xm/new.py
--- a/tools/python/xen/xm/new.py	Tue Sep 16 22:01:28 2008 +0900
+++ b/tools/python/xen/xm/new.py	Wed Sep 17 16:57:45 2008 +0900
@@ -37,17 +37,11 @@
     try:
         server.xend.domain.new(config)
     except xmlrpclib.Fault, ex:
-        import signal
-        if vncpid:
-            os.kill(vncpid, signal.SIGKILL)
         if ex.faultCode == XendClient.ERROR_INVALID_DOMAIN:
             err("the domain ''%s'' does not exist." %
ex.faultString)
         else:
             err("%s" % ex.faultString)
     except Exception, ex:
-        import signal
-        if vncpid:
-            os.kill(vncpid, signal.SIGKILL)
         err(str(ex))
 
 


-- 
yamahata

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@lists.xensource.com
http://lists.xensource.com/xen-ia64-devel

Isaku Yamahata writes ("[PATCH] xm: Don''t spawn vncviewer
twice."):
> Hi. I found this issue with IA64 box while looking at the c/s of
> 18204:21dd1fdb73d8, but I believe the things is same with x86 box.
Summary: I agree with the change but the commit message should read:
  Remove obsolete mechanism using vncviewer -listen


(Your numbers are out of sequence: over here 21dd1fdb73d8 is 18178.)

Since 21dd1fdb73d8 there have been (as far as I can see) three
separate mechanisms for achieving a VNC display:
 1. xm spawns vncviewer after getting vnc display info
     from qemu-dm via xenstore  (introduced in 21dd1fdb73d8)
 2. xm spawns vncviewer -listen and qemu-dm connects to it
 3. qemu-dm spawns vncviewer (!)

The latter two are are rather strange - No.3 is very strange indeed.
So I decided that rather than try to get No.2 or No.3 on track for
going into qemu upstream, I would drop them.  After discussion on
xen-devel I introduced the mechanism No.1, above.

No.1 is controlled by the --spawn-vncviewer (and --vncviewer-autopass)
command line options to xm, by analogy with the -c option.

Nos.2 and 3 are controlled by elements of the domain configuration
file - and their code still remains.  So if you turn all of the vnc
options on you can get several vncviewers (although only one of them
will work).

I think it is good to remove the support for the passive connection
mode No.2, which is what your patch seems to do.  After all
ioemu-remote will never connect to such a vncviewer.  The options to
engage this functionality were already removed from the example config
files by Keir in 18241:bf4ef45e6a38.
> I''m not sure whether this patch coexist with the old ioemu which
> is to be removed soon. So it might be desirable to apply
> this patch after deleting the internal ioemu tree.
I think the old tree is pretty much dead now ...

Ian.

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@lists.xensource.com
http://lists.xensource.com/xen-ia64-devel

Thanks for the detailed analysis.
Here is the updated patch. I added your message into the log
message with slight modifications. So feel free to add your
signed-off-by.


xm: Remove obsolete mechanism using vncviewer -listen

Without this patch, vncviewer processes remain as follows.
This patch fixes it.
> # pgrep -fl vnc
> 4303 vncviewer -log *:stdout:0 -listen 5501
> 5089 vncviewer -log *:stdout:0 -listen 5502
> 5763 vncviewer -log *:stdout:0 -listen 5503
details:
Since 21dd1fdb73d8 there have been (as far as I can see) three
separate mechanisms for achieving a VNC display:
 1. xm spawns vncviewer after getting vnc display info
     from qemu-dm via xenstore  (introduced in 21dd1fdb73d8)
 2. xm spawns vncviewer -listen and qemu-dm connects to it
 3. qemu-dm spawns vncviewer (!)

The latter two are rather strange - No.3 is very strange indeed.
So I decided that rather than try to get No.2 or No.3 on track for
going into qemu upstream, No.2 and No.3 would be dropped.
After discussion on xen-devel the mechanism No.1 was introduced, above.

No.1 is controlled by the --spawn-vncviewer (and --vncviewer-autopass)
command line options to xm, by analogy with the -c option.

Nos.2 and 3 are controlled by elements of the domain configuration
file - and their code still remains.  So if you turn all of the vnc
options on you can get several vncviewers (although only one of them
will work).

This patch removes the support for the passive connection mode No.2
After all ioemu-remote will never connect to such a vncviewer.
The options to engage this functionality were already removed from
the example config files by Keir in 18241:bf4ef45e6a38.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

diff -r 2571ec1ddde1 tools/python/xen/xm/create.py
--- a/tools/python/xen/xm/create.py	Tue Sep 16 22:01:28 2008 +0900
+++ b/tools/python/xen/xm/create.py	Wed Sep 17 18:44:33 2008 +0900
@@ -1116,39 +1116,7 @@
         if port in ports: continue
         return d
     return None
-vncpid = None
 
-def spawn_vnc(display):
-    """Spawns a vncviewer that listens on the specified display.
On success,
-    returns the port that the vncviewer is listening on and sets the global
-    vncpid.  On failure, returns 0.  Note that vncviewer is daemonized.
-    """
-    vncargs = (["vncviewer", "-log",
"*:stdout:0",
-            "-listen", "%d" % (VNC_BASE_PORT + display) ])
-    global vncpid
-    vncpid = utils.daemonize("vncviewer", vncargs)
-    if vncpid == 0:
-        return 0
-
-    return VNC_BASE_PORT + display
-
-def preprocess_vnc(vals):
-    """If vnc was specified, spawn a vncviewer in listen mode
-    and pass its address to the domain on the kernel command line.
-    """
-    if vals.dryrun: return
-    if vals.vncviewer:
-        vnc_display = choose_vnc_display()
-        if not vnc_display:
-            warn("No free vnc display")
-            return
-        print ''VNC='', vnc_display
-        vnc_port = spawn_vnc(vnc_display)
-        if vnc_port > 0:
-            vnc_host = get_host_addr()
-            vnc = ''VNC_VIEWER=%s:%d'' % (vnc_host, vnc_port)
-            vals.extra = vnc + '' '' + vals.extra
-    
 def preprocess(vals):
     preprocess_disk(vals)
     preprocess_pci(vals)
@@ -1156,7 +1124,6 @@
     preprocess_ioports(vals)
     preprocess_ip(vals)
     preprocess_nfs(vals)
-    preprocess_vnc(vals)
     preprocess_vtpm(vals)
     preprocess_access_control(vals)
     preprocess_cpuid(vals, ''cpuid'')
@@ -1193,23 +1160,10 @@
     try:
         dominfo = server.xend.domain.create(config)
     except xmlrpclib.Fault, ex:
-        import signal
-        if vncpid:
-            os.kill(vncpid, signal.SIGKILL)
         if ex.faultCode == xen.xend.XendClient.ERROR_INVALID_DOMAIN:
             err("the domain ''%s'' does not exist." %
ex.faultString)
         else:
             err("%s" % ex.faultString)
-    except Exception, ex:
-        # main.py has good error messages that let the user know what failed.
-        # unless the error is a create.py specific thing, it should be handled
-        # at main. The purpose of this general-case
''Exception'' handler is to
-        # clean up create.py specific processes/data but since create.py does
-        # not know what to do with the error, it should pass it up.
-        import signal
-        if vncpid:
-            os.kill(vncpid, signal.SIGKILL)
-        raise
 
     dom = sxp.child_value(dominfo, ''name'')
 
diff -r 2571ec1ddde1 tools/python/xen/xm/new.py
--- a/tools/python/xen/xm/new.py	Tue Sep 16 22:01:28 2008 +0900
+++ b/tools/python/xen/xm/new.py	Wed Sep 17 18:44:33 2008 +0900
@@ -37,17 +37,11 @@
     try:
         server.xend.domain.new(config)
     except xmlrpclib.Fault, ex:
-        import signal
-        if vncpid:
-            os.kill(vncpid, signal.SIGKILL)
         if ex.faultCode == XendClient.ERROR_INVALID_DOMAIN:
             err("the domain ''%s'' does not exist." %
ex.faultString)
         else:
             err("%s" % ex.faultString)
     except Exception, ex:
-        import signal
-        if vncpid:
-            os.kill(vncpid, signal.SIGKILL)
         err(str(ex))
 
 


On Wed, Sep 17, 2008 at 10:26:15AM +0100, Ian Jackson
wrote:
> Isaku Yamahata writes ("[PATCH] xm: Don''t spawn vncviewer
twice."):
> > Hi. I found this issue with IA64 box while looking at the c/s of
> > 18204:21dd1fdb73d8, but I believe the things is same with x86 box.
> 
> Summary: I agree with the change but the commit message should read:
>   Remove obsolete mechanism using vncviewer -listen
> 
> 
> (Your numbers are out of sequence: over here 21dd1fdb73d8 is 18178.)
> 
> Since 21dd1fdb73d8 there have been (as far as I can see) three
> separate mechanisms for achieving a VNC display:
>  1. xm spawns vncviewer after getting vnc display info
>      from qemu-dm via xenstore  (introduced in 21dd1fdb73d8)
>  2. xm spawns vncviewer -listen and qemu-dm connects to it
>  3. qemu-dm spawns vncviewer (!)
> 
> The latter two are are rather strange - No.3 is very strange indeed.
> So I decided that rather than try to get No.2 or No.3 on track for
> going into qemu upstream, I would drop them.  After discussion on
> xen-devel I introduced the mechanism No.1, above.
> 
> No.1 is controlled by the --spawn-vncviewer (and --vncviewer-autopass)
> command line options to xm, by analogy with the -c option.
> 
> Nos.2 and 3 are controlled by elements of the domain configuration
> file - and their code still remains.  So if you turn all of the vnc
> options on you can get several vncviewers (although only one of them
> will work).
> 
> I think it is good to remove the support for the passive connection
> mode No.2, which is what your patch seems to do.  After all
> ioemu-remote will never connect to such a vncviewer.  The options to
> engage this functionality were already removed from the example config
> files by Keir in 18241:bf4ef45e6a38.
> 
> > I''m not sure whether this patch coexist with the old ioemu
which
> > is to be removed soon. So it might be desirable to apply
> > this patch after deleting the internal ioemu tree.
> 
> I think the old tree is pretty much dead now ...
> 
> Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 
-- 
yamahata

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@lists.xensource.com
http://lists.xensource.com/xen-ia64-devel