Merge lp:~openerp-community/openobject-server/stefan-therp_lp879872-6.1 into lp:openobject-server

[Stefan Rijnhart (Opener)]

This branch slightly modifies the code around the pg_restore command, so that any error condition is properly signalled.

[Olivier Dony (Odoo) (odo-openerp)]

Hi Stefan,

The documentation[1] mentions a warning about the use of Popen.wait() in conjunction with the subprocess writing large amount of data to the pipes, and suggests Popen.communicate() as an alternative. We seem to be fully reading the pipes before wait() is called, so I imagine it might be safe, but wouldn't Popen.communicate() be a better match for our use case?

Also, have you by happenstance tested this code on Windows, to make sure the return codes are identical and reliable on all platforms?

Thanks!

[1] http://docs.python.org/library/subprocess.html#subprocess.Popen.wait

review: Needs Information

[Olivier Dony (Odoo) (odo-openerp)]

(Setting the branch to Work In Progress to indicate we expect some more investigation before merging it. Thanks!)

[Stefan Rijnhart (Opener) (stefan-opener)]

Not sure who changed the status of this proposal to 'Needs review' yesterday evening. As far as I'm concerned it is still 'Work in progress' for lack of a status indicating 'Any community member, feel free to work on it'

[Antony Lesuisse (OpenERP) (al-openerp)]

It was me. Sorry, set it back to 'Needs review' once you think it's ready. Please make sure you dont forget to reap your dead children.

Unmerged revisions

3774. By Stefan Rijnhart (Opener) on 2011-10-22

[FIX] adapted other call to exec_pg_command_pipe() to modified return value

3773. By Stefan Rijnhart (Opener) on 2011-10-22

[FIX] check proper return value of pg_restore

=== modified file 'openerp/service/web_services.py'
--- openerp/service/web_services.py	2011-10-13 10:47:50 +0000
+++ openerp/service/web_services.py	2011-10-22 12:02:24 +0000
@@ -213,10 +213,10 @@
             cmd.append('--port=' + str(tools.config['db_port']))
         cmd.append(db_name)
 
-        stdin, stdout = tools.exec_pg_command_pipe(*tuple(cmd))
-        stdin.close()
-        data = stdout.read()
-        res = stdout.close()
+        pop = tools.exec_pg_command_pipe(*tuple(cmd))
+        pop.stdin.close()
+        data = pop.stdout.read()
+        res = pop.wait()
         if res:
             logger.notifyChannel("web-services", netsvc.LOG_ERROR,
                     'DUMP DB: %s failed\n%s' % (db_name, data))
@@ -257,11 +257,11 @@
             args2=list(args2)
             args2.append(' ' + tmpfile)
             args2=tuple(args2)
-        stdin, stdout = tools.exec_pg_command_pipe(*args2)
+        pop = tools.exec_pg_command_pipe(*args2)
         if not os.name == "nt":
-            stdin.write(base64.decodestring(data))
-        stdin.close()
-        res = stdout.close()
+            pop.stdin.write(base64.decodestring(data))
+        pop.stdin.close()
+        res = pop.wait()
         if res:
             raise Exception, "Couldn't restore database"
         logger.notifyChannel("web-services", netsvc.LOG_INFO,

=== modified file 'openerp/tools/misc.py'
--- openerp/tools/misc.py	2011-09-22 09:54:43 +0000
+++ openerp/tools/misc.py	2011-10-22 12:02:24 +0000
@@ -103,10 +103,9 @@
         raise Exception('Couldn\'t find %s' % name)
     # on win32, passing close_fds=True is not compatible
     # with redirecting std[in/err/out]
-    pop = subprocess.Popen((prog,) + args, bufsize= -1,
+    return subprocess.Popen((prog,) + args, bufsize= -1,
           stdin=subprocess.PIPE, stdout=subprocess.PIPE,
           close_fds=(os.name=="posix"))
-    return (pop.stdin, pop.stdout)
 
 def exec_command_pipe(name, *args):
     prog = find_in_path(name)