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

[Christophe de Dinechin]

> On 23 Feb 2018, at 11:47, Christophe Fergeau  wrote:
> 
> On Fri, Feb 23, 2018 at 11:42:42AM +0100, Christophe de Dinechin wrote:
>> The suggestion about nacking commits that don’t have a long log seems a bit 
>> extreme for one-line cleanups.
> 
> This patch is not a one-line cleanup, of course I would not blindly nack
> trivial/self explanatory  patches, but I also don't want to get into a
> discussion defining what a trivial patch is.

Please re-read. I did acknowledge this was no longer a trivial patch and should 
have a long commit log. I said it diverged.

> 
> Christophe
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe Fergeau]

On Fri, Feb 23, 2018 at 11:42:42AM +0100, Christophe de Dinechin wrote:
> The suggestion about nacking commits that don’t have a long log seems a bit 
> extreme for one-line cleanups.

This patch is not a one-line cleanup, of course I would not blindly nack
trivial/self explanatory  patches, but I also don't want to get into a
discussion defining what a trivial patch is.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [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  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  wrote:
>>> 
>>> From: Christophe de Dinechin 
>>> 
>>> Signed-off-by: Christophe de Dinechin 
>>> ---
>>> 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 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, _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  
>> 
>> 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

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe Fergeau]

On Fri, Feb 23, 2018 at 08:18:52AM +0100, Christophe de Dinechin wrote:
> 
> > On Feb 23, 2018, at 8:07 AM, Frediano Ziglio  wrote:
> > 
> > From: Christophe de Dinechin 
> > 
> > Signed-off-by: Christophe de Dinechin 
> > ---
> > 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 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, _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  
> 
> 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...)

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe de Dinechin]

> On Feb 23, 2018, at 8:07 AM, Frediano Ziglio  wrote:
> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
> 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 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, _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  

then realized it was signed-off by me as well, which makes it weird ;-)

> -- 
> 2.14.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel