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

[Christophe de Dinechin]

> On 22 Feb 2018, at 15:56, 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 v2:
> - rebased.
> 
> Change since v1:
> - do not clash with possible short 'b' option.
> ---
> src/spice-streaming-agent.cpp | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 4b14b6f..1876880 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,12 @@ int main(int argc, char* argv[])
>     int logmask = LOG_UPTO(LOG_WARNING);
>     const char *pluginsdir = PLUGINSDIR;
>     enum {
> -        OPT_PLUGINS_DIR = UCHAR_MAX+1
> +        OPT_PLUGINS_DIR = UCHAR_MAX+1,

As per my reply to Snir, I’d rather have no init on one of the actual options, I suggest:

	OPT_FIRST = UCHAR_MAX,
	OPT_PLUGINS_DIR,
	OPT_LOG_BINARY

But I still did not understand why you think accepting ‘-b’ is not a good idea. I could think of two reasons, disagree with both:

- Should not be short because debug only. "make -d", "ssh -v" are counter examples that explain why I disagree with that one.
- We are at risk of running out of short options. The current list explains why I’m not very concerned with that one.

Thanks
Christophe
 
> +        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 +487,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;
> -- 
> 2.14.3
>