Re: [Spice-devel] [PATCH spice-streaming-agent v3] log_binary is really a boolean
> 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
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
> 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, &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 >> >> 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
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, &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 > > 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
> 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, &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 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