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

[Frediano Ziglio]

> 
> > 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,

but is not --first. Maybe INVALID_OPT ??
what's the problem of having an option initialized?

> 	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.
> 

But the "log_binary is really a boolean" don't justify that change.
Also the -d and -v options you specified are quite general and not that specific.
And they are quite BSD like tools which usually liked short options (ssh which was
born and raised BSD has only short options and syntaxes like -o MyAddedOption=value
GNU prefers long options instead).

> 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;

Frediano