On Mon, Jul 09, 2012 at 04:59:02PM -0700, Chase Douglas wrote: > On 07/09/2012 04:26 PM, Peter Hutterer wrote: > >On Mon, Jul 09, 2012 at 10:19:50AM -0700, Chase Douglas wrote: > >>On 07/08/2012 06:50 PM, Peter Hutterer wrote: > >>>On Fri, Jul 06, 2012 at 11:08:13AM -0700, Chase Douglas wrote: > >>>>On 07/05/2012 05:50 PM, Peter Hutterer wrote: > >>>>>On Thu, Jul 05, 2012 at 02:09:13PM -0700, Chase Douglas wrote: > >>>>>>On 07/03/2012 10:42 PM, Peter Hutterer wrote: > >>>>>>>On Tue, Jul 03, 2012 at 10:33:50AM -0700, Chase Douglas wrote: > >>>>>>>>On 07/02/2012 11:44 PM, Peter Hutterer wrote: > >>>>>>>>>Signed-off-by: Peter Hutterer <[email protected]> > >>>>>>>>>--- > >>>>>>>>> include/xorg/gtest/xorg-gtest-xserver.h | 13 ++++++++++++ > >>>>>>>>> src/environment.cpp | 31 > >>>>>>>>> +++------------------------- > >>>>>>>>> src/xserver.cpp | 34 > >>>>>>>>> +++++++++++++++++++++++++++++++ > >>>>>>>>> 3 files changed, 50 insertions(+), 28 deletions(-) > >>>>>>>>> > >>>>>>>>>diff --git a/include/xorg/gtest/xorg-gtest-xserver.h > >>>>>>>>>b/include/xorg/gtest/xorg-gtest-xserver.h > >>>>>>>>>index 52a2fd0..821b01f 100644 > >>>>>>>>>--- a/include/xorg/gtest/xorg-gtest-xserver.h > >>>>>>>>>+++ b/include/xorg/gtest/xorg-gtest-xserver.h > >>>>>>>>>@@ -53,6 +53,19 @@ class XServer : public xorg::testing::Process { > >>>>>>>>> void Start(std::string &program); > >>>>>>>>> > >>>>>>>>> /** > >>>>>>>>>+ * Terminates this server process. Will signal the server to > >>>>>>>>>terminate > >>>>>>>>>+ * multiple times before giving up. > >>>>>>>>>+ * > >>>>>>>>>+ * @return false if the server did not terminate, true otherwise > >>>>>>>>>+ */ > >>>>>>>>>+ bool Terminate(void); > >>>>>>>>>+ > >>>>>>>>>+ /** > >>>>>>>>>+ * Kills the server. With a vengeance. > >>>>>>>>>+ */ > >>>>>>>>>+ bool Kill(void); > >>>>>>>> > >>>>>>>>We don't need to recreate these functions. We've already inherited > >>>>>>>>them from xorg::testing::Process. Those implementations should work > >>>>>>>>automatically if we set up the XServer class properly. > >>>>>>> > >>>>>>>The implementation is different to the one in Process, this one does > >>>>>>>the > >>>>>>>server-specific bits like waiting for the process to shut down and then > >>>>>>>complaining to the log. > >>>>>> > >>>>>>I read too quickly to realize the difference between the > >>>>>>Environment/XServer implementation and the Process implementation, > >>>>>>but I'm wondering if we should just move the extra stuff to the > >>>>>>Process implementation. Then we won't need to do any redefinition or > >>>>>>overriding. > >>>>> > >>>>>I think we should keep Process basic. For most processes failing to kill > >>>>>it > >>>>>should be an issue in itself, it's just the server that takes too long to > >>>>>shut down and needs special handling. > >>>> > >>>>I thought about that too. I think the question to ask is: do we > >>>>think most users or subclasses of Process are going to want wait for > >>>>the process to really die. The Process::Kill and Terminate methods > >>>>just try once to raise the signal to the child process. They return > >>>>true if the signal was sent, but don't check what happens after > >>>>that. Perhaps a better approach to resolving this issue would be to > >>>>add second versions of these methods that would take a timeout value > >>>>and return true only if the signal was successfully sent and the > >>>>process died. > >>>>KillAndCheck(timeout) and TerminateAndCheck(timeout) maybe? > >>> > >>>maybe so, but let's worry about that when we have other users of Process > >>>that need this feature in a generic parent class? > >> > >>I can't argue with that :). Feel free to leave them in the XServer > >>object for now, though I'd prefer if you renamed them to something > >>like KillAndCheck so it's obvious that it actually does something > >>different than the parent class Kill method. > > > >but isn't that the point of polymorphism? your object will do the right > >thing, regardless of what class it happens to be at the time. > > The difference here is that we are doing two different things. > Process::Kill() merely signals the process. XServer::KillAndCheck() > signals the process and waits for it to die. Using polymorphism to > hide implementation differences between classes is useful. Using > polymorphism to hide behavior changes is dangerous. > > Let's say you define XServer::Kill(), so that it waits a second to > check if it has died. Now, let's say there is a function that is > passed a Process object and calls Kill() on it. If the object is > simply a Process object, Kill() will return immediately after > signalling the process. If the object is an XServer project, it will > only return after waiting for up to a second. This could create odd > interactions due to the hidden behavior difference depending on the > actual type of the object. > > What qualifies as a behavior difference vs an implementation > difference isn't black and white, but this particular case seems to > me to clearly fall on the behavior side of the fence.
changed locally, will be in next patch set Cheers, Peter _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
