[Spice-devel] [PATCH spice-streaming-agent v3] log_binary is really a boolean

[Christophe de Dinechin]

> On 23 Feb 2018, at 10:58, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 
> On Fri, Feb 23, 2018 at 08:18:52AM +0100, Christophe de Dinechin wrote:
>> 
>>> On Feb 23, 2018, at 8:07 AM, Frediano Ziglio <fziglio at redhat.com> wrote:
>>> 
>>> From: Christophe de Dinechin <dinechin at redhat.com>
>>> 
>>> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
>>> ---
>>> Change since v3:
>>> - change enum syntax.
>>> 
>>> Change since v2:
>>> - rebased.
>>> 
>>> Change since v1:
>>> - do not clash with possible short 'b' option.
>>> ---
>>> src/spice-streaming-agent.cpp | 13 +++++++++----
>>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>> index 4b14b6f..494cf8e 100644
>>> --- a/src/spice-streaming-agent.cpp
>>> +++ b/src/spice-streaming-agent.cpp
>>> @@ -58,9 +58,9 @@ struct SpiceStreamDataMessage
>>> 
>>> static bool streaming_requested = false;
>>> static bool quit_requested = false;
>>> +static bool log_binary = false;
>>> static std::set<SpiceVideoCodecType> client_codecs;
>>> static int streamfd = -1;
>>> -static int log_binary = 0;
>>> static std::mutex stream_mtx;
>>> 
>>> static int have_something_to_read(int timeout)
>>> @@ -451,11 +451,13 @@ int main(int argc, char* argv[])
>>>    int logmask = LOG_UPTO(LOG_WARNING);
>>>    const char *pluginsdir = PLUGINSDIR;
>>>    enum {
>>> -        OPT_PLUGINS_DIR = UCHAR_MAX+1
>>> +        OPT_first = UCHAR_MAX,
>>> +        OPT_PLUGINS_DIR,
>>> +        OPT_LOG_BINARY,
>>>    };
>> 
>>> -    struct option long_options[] = {
>>> +    static const struct option long_options[] = {
>>>        { "plugins-dir", required_argument, NULL, OPT_PLUGINS_DIR},
>>> -        { "log-binary", no_argument, &log_binary, 1},
>>> +        { "log-binary", no_argument, NULL, OPT_LOG_BINARY},
>>>        { "help", no_argument, NULL, 'h'},
>>>        { 0, 0, 0, 0}
>>>    };
>>> @@ -486,6 +488,9 @@ int main(int argc, char* argv[])
>>>            agent.AddOption(optarg, p);
>>>            break;
>>>        }
>>> +        case OPT_LOG_BINARY:
>>> +            log_binary = true;
>>> +            break;
>>>        case 'l':
>>>            log_filename = optarg;
>>>            break;
>> 
>> Was about to add
>> 
>> Acked-by: Christophe de Dinechin <dinechin at redhat.com> 
>> 
>> then realized it was signed-off by me as well, which makes it weird ;-)
> 
> Just realized by reading the commit log and what I remember from the
> reviews, I only have a very vague idea regarding what this is about.
> commit log needs improving... (I really should start nack'ing
> patches with only a short log without even reading them…)

Well, initially, it was just changing log_binary from int to bool.

But then it diverged because of the option processing aspect. I had used short option b, Frediano did not like it (he still did not convince me ;-), so then he replaced that with an enum for long options, and then Snir made another commit with a long option and there was a conflict risk with two options being initialized to UCHAR_MAX+1, so I asked for OPT_first, and now we end up with something that deserves a long commit log.

The suggestion about nacking commits that don’t have a long log seems a bit extreme for one-line cleanups.

> 
> Christophe