Hi,
On Thu, Feb 15, 2018 at 07:14:14AM +, Frediano Ziglio wrote:
> Some header used some slightly different naming style for
> header guards.
> Update the names to make them more coherent.
>
> Signed-off-by: Frediano Ziglio
> ---
> src/hexdump.h | 4 ++--
> src/jpeg.hpp | 4 ++--
> 2 files
Some header used some slightly different naming style for
header guards.
Update the names to make them more coherent.
Signed-off-by: Frediano Ziglio
---
src/hexdump.h | 4 ++--
src/jpeg.hpp | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/hexdump.h b/src/hexdump.h
in
>
> From: Christophe de Dinechin
>
> In file included from mjpeg-fallback.cpp:8:
> ./mjpeg-fallback.hpp:28:25: warning: 'VideoCodecType' overrides a member
> function but is not marked 'override' [-Winconsistent-missing-override]
> SpiceVideoCodecType VideoCodecType() const;
>
>
> From: Christophe de Dinechin
>
> Signed-off-by: Christophe de Dinechin
> ---
> src/spice-streaming-agent.cpp | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 7a3a46d..e825565 100644
> --- a/
>
> From: Christophe de Dinechin
>
> Signed-off-by: Christophe de Dinechin
> ---
> src/spice-streaming-agent.cpp | 49
> +--
> 1 file changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.
>
> From: Christophe de Dinechin
>
> Signed-off-by: Christophe de Dinechin
> ---
> src/spice-streaming-agent.cpp | 43
> ---
> 1 file changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.
>
> From: Christophe de Dinechin
>
> Signed-off-by: Christophe de Dinechin
> ---
> src/spice-streaming-agent.cpp | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index e9ef310..9a5c4fa 100644
> --- a/s
>
> From: Christophe de Dinechin
>
> In file included from mjpeg-fallback.cpp:8:
> ./mjpeg-fallback.hpp:28:25: warning: 'VideoCodecType' overrides a member
> function but is not marked 'override' [-Winconsistent-missing-override]
> SpiceVideoCodecType VideoCodecType() const;
>
>
> From: Christophe de Dinechin
>
> ./static-plugin.hpp:29:5: warning: 'const' qualifier on function type
> 'PluginInitFunc' (aka 'bool (spice::streaming_agent::Agent *)') has no
> effect [-Wignored-qualifiers]
> const PluginInitFunc* const init_func;
> ^~
>
> Signed-off-by: Christ
>
> From: Christophe de Dinechin
>
> Signed-off-by: Christophe de Dinechin
> ---
> src/spice-streaming-agent.cpp | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index e345d99..8f25940 100644
> ---
>
> From: Christophe de Dinechin
>
> As per e-mail discussions (https://patchwork.freedesktop.org/patch/200629/),
> using std::string for C-style string constants is against C++ good practices,
> see
> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rstr-zstring
> fo
>
> From: Christophe de Dinechin
>
> Signed-off-by: Christophe de Dinechin
> ---
> src/spice-streaming-agent.cpp | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 5fb20d1..e345d99 100
>
> From: Christophe de Dinechin
>
> Otherwise, clang complains:
>
> spice-streaming-agent.cpp:414:66: warning: format specifies type 'unsigned
> long' but the argument has type 'uint64_t' (aka 'unsigned long long')
> [-Wformat]
> fprintf(f_log, "%lu: Frame of %zu bytes:\n",
>
> From: Christophe de Dinechin
>
> We should really not be capturing cursor by reference here,
> since that value could be changed by the surrounding loop and sending
> the data could some day be made asynchronous.
>
> Signed-off-by: Christophe de Dinechin
> ---
> src/spice-streaming-agent.
>
> From: Christophe de Dinechin
>
> Signed-off-by: Christophe de Dinechin
> ---
> src/spice-streaming-agent.cpp | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index c4c52ef..760c211 100644
> ---
>
> From: Christophe de Dinechin
>
> Without this, clang emits among others:
>
> spice-streaming-agent.cpp:355:31: error: implicit instantiation of undefined
> template 'std::__1::basic_string,
> std::__1::allocator >'
> streamfd = open(streamport.c_str(), O_RDWR);
>
> On 9 Feb 2018, at 17:59, Frediano Ziglio wrote:
>
> +
> +// read part of the message till we get all
> +if (dev->msg_len < dev->hdr.size) {
> +dev->msg = g_realloc(dev->msg, dev->hdr.size);
> +dev->msg_len = dev->hdr.size;
> +}
>
> On 14 Feb 2018, at 14:35, Christophe Fergeau wrote:
>
> This one sounds more like an RFC to me
Well, this is really a bug fix in the documentation more than a RFC.
> , as from a quick look in server/,
> this is not the style currently in use.
As I pointed out in earlier discussions, this s
> On 14 Feb 2018, at 14:37, Christophe Fergeau wrote:
>
> On Thu, Feb 08, 2018 at 12:25:30PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin
>>
>> The objective of these guidelines is that:
>> - We avoid introducing new warnings
>> - We know how to fix old ones
>> - We do
> On 14 Feb 2018, at 17:29, Christophe Fergeau wrote:
>
> This changes the suggested style to what is currently used in
> spice-server codebase. This also removes a few sentences which
> are not really related to how one should format their header guards.
>
> Signed-off-by: Christophe Fergeau
> On 14 Feb 2018, at 17:34, Christophe Fergeau wrote:
>
> On Wed, Feb 14, 2018 at 10:45:56AM -0500, Frediano Ziglio wrote:
>>>
>>> Shouldn't this go with a Makefile rule? A few lines in the log what this
>>> is about, what is the goal for having this file, ... would not hurt.
>>>
>>> Christop
I cut quite a bit for clarity. What I cut I agree with ;-)
> On 14 Feb 2018, at 14:45, Lukáš Hrázký wrote:
>
> On Tue, 2018-02-13 at 17:10 +0100, Christophe de Dinechin wrote:
>> Hi Lukas,
>>
>
> For your comments which are unrelated to my changes, I commended
> uniformly with "Was in origina
From: Christophe de Dinechin
Without this, clang emits among others:
spice-streaming-agent.cpp:355:31: error: implicit instantiation of undefined
template 'std::__1::basic_string,
std::__1::allocator >'
streamfd = open(streamport.c_str(), O_RDWR);
^
Signed-of
From: Christophe de Dinechin
Otherwise, clang complains:
spice-streaming-agent.cpp:414:66: warning: format specifies type 'unsigned
long' but the argument has type 'uint64_t' (aka 'unsigned long long') [-Wformat]
fprintf(f_log, "%lu: Frame of %zu bytes:\n", get_time(),
fram
From: Christophe de Dinechin
./static-plugin.hpp:29:5: warning: 'const' qualifier on function type
'PluginInitFunc' (aka 'bool (spice::streaming_agent::Agent *)') has no effect
[-Wignored-qualifiers]
const PluginInitFunc* const init_func;
^~
Signed-off-by: Christophe de Dinechin
-
From: Christophe de Dinechin
Signed-off-by: Christophe de Dinechin
---
src/spice-streaming-agent.cpp | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index c4c52ef..760c211 100644
--- a/src/spice-streaming-
From: Christophe de Dinechin
Signed-off-by: Christophe de Dinechin
---
src/spice-streaming-agent.cpp | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index e9ef310..9a5c4fa 100644
--- a/src/spice-streaming-agen
From: Christophe de Dinechin
Signed-off-by: Christophe de Dinechin
---
src/spice-streaming-agent.cpp | 43 ---
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 9a5c4fa..a55
From: Christophe de Dinechin
In file included from mjpeg-fallback.cpp:8:
./mjpeg-fallback.hpp:28:25: warning: 'VideoCodecType' overrides a member
function but is not marked 'override' [-Winconsistent-missing-override]
SpiceVideoCodecType VideoCodecType() const;
^
../i
From: Christophe de Dinechin
Signed-off-by: Christophe de Dinechin
---
src/spice-streaming-agent.cpp | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index e345d99..8f25940 100644
--- a/src/spice-streaming-a
From: Christophe de Dinechin
This series implements a number of comments that were made during
previous code reviews, including a reworked / udpated version of the
stream RAII patch.
Christophe de Dinechin (14):
Add required header
Capture 'cursor' by value
streaming_requested is really a
From: Christophe de Dinechin
We should really not be capturing cursor by reference here,
since that value could be changed by the surrounding loop and sending
the data could some day be made asynchronous.
Signed-off-by: Christophe de Dinechin
---
src/spice-streaming-agent.cpp | 2 +-
1 file ch
From: Christophe de Dinechin
As per e-mail discussions (https://patchwork.freedesktop.org/patch/200629/),
using std::string for C-style string constants is against C++ good practices,
see
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rstr-zstring
for reference.
S
From: Christophe de Dinechin
Signed-off-by: Christophe de Dinechin
---
src/spice-streaming-agent.cpp | 49 +--
1 file changed, 29 insertions(+), 20 deletions(-)
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index a55e1a5..7a3
From: Christophe de Dinechin
Signed-off-by: Christophe de Dinechin
---
src/spice-streaming-agent.cpp | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 5fb20d1..e345d99 100644
--- a/src/spice-s
From: Christophe de Dinechin
Signed-off-by: Christophe de Dinechin
---
src/spice-streaming-agent.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 7a3a46d..e825565 100644
--- a/src/spice-streaming-ag
From: Christophe de Dinechin
This lets us get rid of C-style 'goto done' in do_capture.
Signed-off-by: Christophe de Dinechin
---
src/spice-streaming-agent.cpp | 32
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/spice-streaming-agent.cpp b
>
> It was mainly a debugging feature for the early development stages. It
> was agreed its usefulness is at the moment outweighed by the complexity
> it brings to the code.
>
> Signed-off-by: Lukáš Hrázký
Acked-by: Frediano Ziglio
Frediano
> ---
> Changes since v2:
> - fix grammar in the de
On Thu, Feb 08, 2018 at 11:09:35AM +0100, Victor Toso wrote:
> Hi,
>
> On Thu, Feb 08, 2018 at 05:01:05AM -0500, Frediano Ziglio wrote:
> > > > Depends on many cases. You don't want spurious changes to make harder to
> > > > look at the history for instance (that is a point for Nack).
> > > > The
On Wed, Feb 14, 2018 at 10:40:58AM -0500, Frediano Ziglio wrote:
> >
> > > On 14 Feb 2018, at 13:34, Lukáš Hrázký wrote:
> > >
> > > Introduce a unit test framework (Catch) to the codebase and a simple
> > > unit test for parsing the options of the mjpeg plugin.
> > >
> > > Signed-off-by: Lukáš
On Wed, Feb 14, 2018 at 10:45:56AM -0500, Frediano Ziglio wrote:
> >
> > Shouldn't this go with a Makefile rule? A few lines in the log what this
> > is about, what is the goal for having this file, ... would not hurt.
> >
> > Christophe
> >
>
> I think this file is supposed to just help develo
This changes the suggested style to what is currently used in
spice-server codebase. This also removes a few sentences which
are not really related to how one should format their header guards.
Signed-off-by: Christophe Fergeau
---
docs/spice_style.txt | 6 ++
1 file changed, 2 insertions(+)
> On 14 Feb 2018, at 16:43, Christophe Fergeau wrote:
>
> On Thu, Feb 08, 2018 at 12:25:29PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin
>>
>> Signed-off-by: Christophe de Dinechin
>> ---
>> docs/spice_style.txt | 19 +++
>> 1 file changed, 19 insertio
It was mainly a debugging feature for the early development stages. It
was agreed its usefulness is at the moment outweighed by the complexity
it brings to the code.
Signed-off-by: Lukáš Hrázký
---
Changes since v2:
- fix grammar in the description
- use ternary if for passing the LOG_PERROR flag
>
> Shouldn't this go with a Makefile rule? A few lines in the log what this
> is about, what is the goal for having this file, ... would not hurt.
>
> Christophe
>
I think this file is supposed to just help developers so should not
be in the Makefile.
I think you mean that the intention should
On Thu, Feb 08, 2018 at 12:25:29PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin
>
> Signed-off-by: Christophe de Dinechin
> ---
> docs/spice_style.txt | 19 +++
> 1 file changed, 19 insertions(+)
>
> diff --git a/docs/spice_style.txt b/docs/spice_style.tx
>
> > On 14 Feb 2018, at 13:34, Lukáš Hrázký wrote:
> >
> > Introduce a unit test framework (Catch) to the codebase and a simple
> > unit test for parsing the options of the mjpeg plugin.
> >
> > Signed-off-by: Lukáš Hrázký
> > ---
> > configure.ac | 3 ++
> > src/mjpe
On Wed, 2018-02-14 at 16:08 +0100, Christophe de Dinechin wrote:
> > On 14 Feb 2018, at 13:34, Lukáš Hrázký wrote:
> >
> > Introduce a unit test framework (Catch) to the codebase and a simple
> > unit test for parsing the options of the mjpeg plugin.
> >
> > Signed-off-by: Lukáš Hrázký
> > ---
On Wed, 2018-02-14 at 09:22 -0500, Frediano Ziglio wrote:
> >
> > Introduce a unit test framework (Catch) to the codebase and a simple
> > unit test for parsing the options of the mjpeg plugin.
> >
> > Signed-off-by: Lukáš Hrázký
> > ---
> > configure.ac | 3 ++
> > sr
> On 14 Feb 2018, at 15:28, Christophe Fergeau wrote:
>
> On Thu, Feb 08, 2018 at 08:16:37AM +0100, Christophe de Dinechin wrote:
>> Using std::string by default is *not* considered a good practice in
>> C++. The reference for C++ good practices are probably best summarized
>> by the C++ Core G
> On 14 Feb 2018, at 13:34, Lukáš Hrázký wrote:
>
> Introduce a unit test framework (Catch) to the codebase and a simple
> unit test for parsing the options of the mjpeg plugin.
>
> Signed-off-by: Lukáš Hrázký
> ---
> configure.ac | 3 ++
> src/mjpeg-fallback.cpp
On Thu, Feb 08, 2018 at 08:16:37AM +0100, Christophe de Dinechin wrote:
> Using std::string by default is *not* considered a good practice in
> C++. The reference for C++ good practices are probably best summarized
> by the C++ Core Guidelines:
> https://github.com/isocpp/CppCoreGuidelines/blob/mas
>
> Introduce a unit test framework (Catch) to the codebase and a simple
> unit test for parsing the options of the mjpeg plugin.
>
> Signed-off-by: Lukáš Hrázký
> ---
> configure.ac | 3 ++
> src/mjpeg-fallback.cpp| 5 +++
> src/mjpeg-fallback.hpp
On Mon, Jan 29, 2018 at 05:16:12PM -0500, Frediano Ziglio wrote:
> >
> >
> > > On 29 Jan 2018, at 12:00, Frediano Ziglio wrote:
> > >
> > > Signed-off-by: Frediano Ziglio
> > > ---
> > > Makefile.am | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Makefi
On 02/14/2018 01:58 PM, Lukáš Hrázký wrote:
It was mainly a debugging feature for the early development stages. It
was agreed it's usefulness is at the moment outweighted by the
its usefulness ...
spellchecker also suggests: outweighed
Looks good, see nit comment below (not important, smaller
On Tue, 2018-02-13 at 17:10 +0100, Christophe de Dinechin wrote:
> Hi Lukas,
>
>
> You asked for it, so here is my TON (TON stands for TON of nits). Except for
> what’s just below this, nothing major, just sharing ideas.
I did ask, I'm glad for those that are relevant :D thanks. Bonus points
fo
On Thu, Feb 08, 2018 at 12:25:30PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin
>
> The objective of these guidelines is that:
> - We avoid introducing new warnings
> - We know how to fix old ones
> - We don't have to isolate whitespace changes when submitting patches,
>
This one sounds more like an RFC to me, as from a quick look in server/,
this is not the style currently in use.
Christophe
On Thu, Feb 08, 2018 at 12:25:31PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin
>
> As written, the headers style guide looks quite wrong. In partic
On Mon, Feb 12, 2018 at 03:23:06AM -0500, Frediano Ziglio wrote:
> See
> https://lists.freedesktop.org/archives/spice-devel/2018-February/041724.html
>
> > From: Christophe de Dinechin
> >
> > Signed-off-by: Christophe de Dinechin
> > ---
> > docs/spice_style.txt | 11 ++-
> > 1 file
On Thu, Feb 08, 2018 at 03:58:46AM -0500, Frediano Ziglio wrote:
> > +
> > +[source,h]
> > +---
> > +#ifndef MY_MODULE_H
> > +#define MY_MODULE_H
> > +
> > +...
> > +
> > +#endif /* MY_MODULE_H */
> > +---
> > +
>
> Are we suggesting C style only comment or is clear the is not important?
> (I'm ju
On Thu, Feb 08, 2018 at 03:50:37AM -0500, Frediano Ziglio wrote:
> >
> > From: Christophe de Dinechin
> >
> > Signed-off-by: Christophe de Dinechin
> > ---
> > docs/spice_style.txt | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/docs/spice_style.txt b/docs/spice
On Thu, Feb 08, 2018 at 03:48:46AM -0500, Frediano Ziglio wrote:
> I think that the "Alternatively, use global `const` variables."
> is quite an ancient comment when spice-server was in C++
> (much before I join and I think even older than the git repository!)
There was a C++ *client* in spice.git
Shouldn't this go with a Makefile rule? A few lines in the log what this
is about, what is the goal for having this file, ... would not hurt.
Christophe
On Thu, Feb 08, 2018 at 12:25:21PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin
>
> Signed-off-by: Christophe de Dinech
On Thu, Feb 08, 2018 at 12:25:24PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin
>
> Signed-off-by: Christophe de Dinechin
> ---
> docs/spice_style.txt | 15 ---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/docs/spice_style.txt b/docs/sp
On Tue, Feb 13, 2018 at 02:40:38PM +0100, Christophe de Dinechin wrote:
>
>
> > On 13 Feb 2018, at 14:39, Frediano Ziglio wrote:
> >
> >>>
> >>> On 12 Feb 2018, at 17:58, Frediano Ziglio wrote:
> >>>
> >>> In SPICE, as in Glib, ERROR is more "strong" than CRITICAL
> >>> (I don't know where t
Use C++ standard library:
- std::string
- std::stoi() to parse the numbers (also note atoi() behavior is undefined in
case of errors)
- exceptions for errors, makes testing and potential future changes to
error handling easier.
Signed-off-by: Lukáš Hrázký
---
src/mjpeg-fallback.cpp | 41
Introduce a unit test framework (Catch) to the codebase and a simple
unit test for parsing the options of the mjpeg plugin.
Signed-off-by: Lukáš Hrázký
---
configure.ac | 3 ++
src/mjpeg-fallback.cpp| 5 +++
src/mjpeg-fallback.hpp| 1 +
Signed-off-by: Lukáš Hrázký
---
src/unittests/.gitignore | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/unittests/.gitignore b/src/unittests/.gitignore
index bb238d7..af41c48 100644
--- a/src/unittests/.gitignore
+++ b/src/unittests/.gitignore
@@ -1 +1,4 @@
/hexdump
+/test-hexdump.sh
This series introduces a C++ unit test framework called Catch to the
codebase, adds a simple unit test for the options parsing for the mjpeg
plugin and improves on the option parsing code.
Since we more or less agreed we can solve the Catch package in RHEL one
way or another, I suppose we can proc
It was mainly a debugging feature for the early development stages. It
was agreed it's usefulness is at the moment outweighted by the
complexity it brings to the code.
Signed-off-by: Lukáš Hrázký
---
Changes since v1:
- Log also to stderr if it's a tty
src/spice-streaming-agent.cpp | 72 +++
> On 14 Feb 2018, at 11:46, Lukáš Hrázký wrote:
>
> It was mainly a debugging feature for the early development stages. It
> was agreed it's usefulness is at the moment outweighted by the
> complexity it brings to the code.
>
> Signed-off-by: Lukáš Hrázký
> ---
> src/spice-streaming-agent.cpp
It was mainly a debugging feature for the early development stages. It
was agreed it's usefulness is at the moment outweighted by the
complexity it brings to the code.
Signed-off-by: Lukáš Hrázký
---
src/spice-streaming-agent.cpp | 71 ---
1 file changed,
> On 13 Feb 2018, at 17:36, Marc-Andre Lureau wrote:
>
> I was not really fond of passing options via URI, but apparently this
> is a standard practice.
So is passing port number after a colon.
Christophe
___
Spice-devel mailing list
Spice-devel@li
73 matches
Mail list logo
