I found these two on my signal-handling branch but they're not really signal related. I guess the second one sort of is. I was trying to figure out why killall anaconda was doing weird things to metacity, and this is why.
David Shea (2): Remove the /usr/bin/liveinst symlink during uninstall Use exec instead of system to run metacity
anaconda | 11 +++++------ data/liveinst/Makefile.am | 3 +++ 2 files changed, 8 insertions(+), 6 deletions(-)
--- data/liveinst/Makefile.am | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/data/liveinst/Makefile.am b/data/liveinst/Makefile.am index 7a3562f..c4becd0 100644 --- a/data/liveinst/Makefile.am +++ b/data/liveinst/Makefile.am @@ -31,6 +31,9 @@ dist_xinit_SCRIPTS = zz-liveinst.sh install-exec-local: $(MKDIR_P) $(DESTDIR)$(bindir) $(LN_S) consolehelper $(DESTDIR)$(bindir)/liveinst + +uninstall-local: + rm -f $(DESTDIR)$(bindir)/liveinst endif
EXTRA_DIST = README liveinst.desktop.in
This way we don't up with a second process named "anaconda" that's actually a middleman for the window manager. --- anaconda | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/anaconda b/anaconda index 4d4676e..6ff4a89 100755 --- a/anaconda +++ b/anaconda @@ -269,14 +269,13 @@ def startMetacityWM(): # after this point the method should never return (or throw an exception # outside) try: - returncode = iutil.execWithRedirect('metacity', ["--display", ":1", "--sm-disable"]) - except BaseException as e: + os.execlp("metacity", "metacity", "--display", ":1", "--sm-disable") + except BaseException: # catch all possible exceptions - log.error("Problems running the window manager: %s", e) - os._exit(1) + pass
- log.info("The window manager has terminated.") - os._exit(returncode) + log.error("Problems running the window manager") + os._exit(1)
return childpid
On Wed, 2014-07-30 at 17:00 -0400, David Shea wrote:
This way we don't up with a second process named "anaconda" that's actually a middleman for the window manager.
anaconda | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/anaconda b/anaconda index 4d4676e..6ff4a89 100755 --- a/anaconda +++ b/anaconda @@ -269,14 +269,13 @@ def startMetacityWM(): # after this point the method should never return (or throw an exception # outside) try:
returncode = iutil.execWithRedirect('metacity', ["--display", ":1", "--sm-disable"])except BaseException as e:
os.execlp("metacity", "metacity", "--display", ":1", "--sm-disable")except BaseException:
I know you didn't come up with with it, but do we really need to catch all BaseException-inherited exceptions here? I mean, it shouldn't throw an expception outside, but if it raises anything else from OSError, it's a bug we should fix, right? I'm probably thinking too much in the context of the recent "at the edge with RAM" context, but I've seen various failures we hide in case something like ENOMEM starts happening.
On 07/31/2014 05:11 AM, Vratislav Podzimek wrote:
On Wed, 2014-07-30 at 17:00 -0400, David Shea wrote:
This way we don't up with a second process named "anaconda" that's actually a middleman for the window manager.
anaconda | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/anaconda b/anaconda index 4d4676e..6ff4a89 100755 --- a/anaconda +++ b/anaconda @@ -269,14 +269,13 @@ def startMetacityWM(): # after this point the method should never return (or throw an exception # outside) try:
returncode = iutil.execWithRedirect('metacity', ["--display", ":1", "--sm-disable"])except BaseException as e:
os.execlp("metacity", "metacity", "--display", ":1", "--sm-disable")except BaseException:I know you didn't come up with with it, but do we really need to catch all BaseException-inherited exceptions here? I mean, it shouldn't throw an expception outside, but if it raises anything else from OSError, it's a bug we should fix, right? I'm probably thinking too much in the context of the recent "at the edge with RAM" context, but I've seen various failures we hide in case something like ENOMEM starts happening.
Yeah, raising anything other than OSError would be weird and something to fix, but I think the idea behind catching BaseException is so that odd errors are logged somewhere instead of just printed to the screen and forgotten. Of course this means I shouldn't have removed the part where we actually log the exception, but I don't see catching it in this case as harmful. If we catch it, we log an error and immediately exit. If we don't catch it, python prints a backtrace to the console and exits. If we run this code after sys.excepthook has already been installed and don't catch BaseException, then something bad will probably happen.
On Thu, 2014-07-31 at 08:48 -0400, David Shea wrote:
On 07/31/2014 05:11 AM, Vratislav Podzimek wrote:
On Wed, 2014-07-30 at 17:00 -0400, David Shea wrote:
This way we don't up with a second process named "anaconda" that's actually a middleman for the window manager.
anaconda | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/anaconda b/anaconda index 4d4676e..6ff4a89 100755 --- a/anaconda +++ b/anaconda @@ -269,14 +269,13 @@ def startMetacityWM(): # after this point the method should never return (or throw an exception # outside) try:
returncode = iutil.execWithRedirect('metacity', ["--display", ":1", "--sm-disable"])except BaseException as e:
os.execlp("metacity", "metacity", "--display", ":1", "--sm-disable")except BaseException:I know you didn't come up with with it, but do we really need to catch all BaseException-inherited exceptions here? I mean, it shouldn't throw an expception outside, but if it raises anything else from OSError, it's a bug we should fix, right? I'm probably thinking too much in the context of the recent "at the edge with RAM" context, but I've seen various failures we hide in case something like ENOMEM starts happening.
Yeah, raising anything other than OSError would be weird and something to fix, but I think the idea behind catching BaseException is so that odd errors are logged somewhere instead of just printed to the screen and forgotten. Of course this means I shouldn't have removed the part where we actually log the exception, but I don't see catching it in this case as harmful. If we catch it, we log an error and immediately exit. If we don't catch it, python prints a backtrace to the console and exits. If we run this code after sys.excepthook has already been installed and don't catch BaseException, then something bad will probably happen.
So shouldn't we at least restore the original excepthook in the child process?
On 07/31/2014 09:09 AM, Vratislav Podzimek wrote:
On Thu, 2014-07-31 at 08:48 -0400, David Shea wrote:
On 07/31/2014 05:11 AM, Vratislav Podzimek wrote:
On Wed, 2014-07-30 at 17:00 -0400, David Shea wrote:
This way we don't up with a second process named "anaconda" that's actually a middleman for the window manager.
anaconda | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/anaconda b/anaconda index 4d4676e..6ff4a89 100755 --- a/anaconda +++ b/anaconda @@ -269,14 +269,13 @@ def startMetacityWM(): # after this point the method should never return (or throw an exception # outside) try:
returncode = iutil.execWithRedirect('metacity', ["--display", ":1", "--sm-disable"])except BaseException as e:
os.execlp("metacity", "metacity", "--display", ":1", "--sm-disable")except BaseException:I know you didn't come up with with it, but do we really need to catch all BaseException-inherited exceptions here? I mean, it shouldn't throw an expception outside, but if it raises anything else from OSError, it's a bug we should fix, right? I'm probably thinking too much in the context of the recent "at the edge with RAM" context, but I've seen various failures we hide in case something like ENOMEM starts happening.
Yeah, raising anything other than OSError would be weird and something to fix, but I think the idea behind catching BaseException is so that odd errors are logged somewhere instead of just printed to the screen and forgotten. Of course this means I shouldn't have removed the part where we actually log the exception, but I don't see catching it in this case as harmful. If we catch it, we log an error and immediately exit. If we don't catch it, python prints a backtrace to the console and exits. If we run this code after sys.excepthook has already been installed and don't catch BaseException, then something bad will probably happen.
So shouldn't we at least restore the original excepthook in the child process?
Yeah, probably. Maybe an iutil function that does a fork/exec plus all this extra stuff would be nice. I think I'll save this commit for the signal handling batch and work on making that part of it a bit nicer.
We're setting an exception on error so we shouldn't continue and return None. --- pyanaconda/isys/isys.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pyanaconda/isys/isys.c b/pyanaconda/isys/isys.c index 23fb77f..3de0d79 100644 --- a/pyanaconda/isys/isys.c +++ b/pyanaconda/isys/isys.c @@ -82,7 +82,7 @@ static PyObject * doSetSystemTime(PyObject *s, PyObject *args) { return NULL;
if (settimeofday(&tv, NULL) != 0) - PyErr_SetFromErrno(PyExc_SystemError); + return PyErr_SetFromErrno(PyExc_SystemError);
Py_INCREF(Py_None); return Py_None;
On Wed, 2014-07-30 at 17:00 -0400, David Shea wrote:
I found these two on my signal-handling branch but they're not really signal related. I guess the second one sort of is. I was trying to figure out why killall anaconda was doing weird things to metacity, and this is why.
David Shea (2): Remove the /usr/bin/liveinst symlink during uninstall Use exec instead of system to run metacity
anaconda | 11 +++++------ data/liveinst/Makefile.am | 3 +++ 2 files changed, 8 insertions(+), 6 deletions(-)
Other than that one question these all look good to me.
anaconda-patches@lists.fedorahosted.org
-
David Shea -
Vratislav Podzimek