[PATCH xorg-gtest 5/5] xserver: add RemoveLogFile()

[Chase Douglas]

On 08/13/2012 04:12 PM, Peter Hutterer wrote:
> On Mon, Aug 13, 2012 at 09:32:00AM -0700, Chase Douglas wrote:
>> On 08/09/2012 10:38 PM, Peter Hutterer wrote:
>>> Simple unlink() call to the logfile in use. The log file is only removed if
>>> the server was started and terminated successfully. If it was never started
>>> or the startup failed for some reason, this function does nothing.
>>>
>>> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
>>> ---
>>>   include/xorg/gtest/xorg-gtest-xserver.h | 6 ++++++
>>>   src/xserver.cpp                         | 5 +++++
>>>   2 files changed, 11 insertions(+)
>>>
>>> diff --git a/include/xorg/gtest/xorg-gtest-xserver.h b/include/xorg/gtest/xorg-gtest-xserver.h
>>> index f3bda9b..33446a8 100644
>>> --- a/include/xorg/gtest/xorg-gtest-xserver.h
>>> +++ b/include/xorg/gtest/xorg-gtest-xserver.h
>>> @@ -95,6 +95,12 @@ class XServer : public xorg::testing::Process {
>>>       virtual bool Kill(unsigned int timeout = 0);
>>>
>>>       /**
>>> +     * Remove the log file used by this server. If the server was never
>>> +     * started or the startup failed, this function does nothing.
>>> +     */
>>> +    void RemoveLogFile();
>>> +
>>> +    /**
>>>        * Waits until this server is ready to take connections.
>>>        */
>>>       void WaitForConnections(void);
>>> diff --git a/src/xserver.cpp b/src/xserver.cpp
>>> index 86c8484..114c736 100644
>>> --- a/src/xserver.cpp
>>> +++ b/src/xserver.cpp
>>> @@ -365,6 +365,11 @@ bool xorg::testing::XServer::Kill(unsigned int timeout) {
>>>       return true;
>>>   }
>>>
>>> +void xorg::testing::XServer::RemoveLogFile() {
>>> +  if (GetState() == TERMINATED)
>>> +    unlink(d_->options["-logfile"].c_str());
>>> +}
>>> +
>>>   void xorg::testing::XServer::SetOption(const std::string &key, const std::string &value) {
>>>     d_->options[key] = value;
>>>   }
>>>
>>
>> I would prefer to call it something that gives a better idea that it
>> is conditional on successful termination of the server.
>> RemoveLogFile() sounds like a method that would *always* remove the
>> log file.
>>
>> What about "CleanUpLogFile()"?
>
> tbh, I don't think there's much difference between the two names, and, had
> we both functions, I certainly couldn't tell the difference between the two
> without reading the documentation. The behaviour is stated in the API
> documentation and not removing what we didn't create is a good behaviour.

If it's not any clearer to you, then maybe there isn't a perfect 
solution. I guess I'm ok with RemoveLogFile().

>> Or, you could just leave it up to the caller to call it only where
>> it should be called. The caller can get the state just the same.
>
> Without the TERMINATED condition, this would simply be an unlink call, for
> any file the caller wants. If callers really just wanted to remove a file
> unconditionally, they can call unlink() themselves.

True.

After this discussion, I'm ready to give my tag:

Reviewed-by: Chase Douglas <chase.douglas at canonical.com>