[Spice-devel] [PATCH spice-streaming-agent 1/2] mjpeg plugin: split off the declaration into a header

Frediano Ziglio fziglio at redhat.com
Tue Jan 30 16:54:53 UTC 2018 

> 
> > On 30 Jan 2018, at 13:29, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> > 
> > On Tue, 2018-01-30 at 06:32 -0500, Frediano Ziglio wrote:
> >>> 
> >>> So that the plugin can later be included in a unit test.
> >>> 
> >>> Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> >>> ---
> >>> src/mjpeg-fallback.cpp | 26 ++++++--------------------
> >>> src/mjpeg-fallback.hpp | 34 ++++++++++++++++++++++++++++++++++
> >>> 2 files changed, 40 insertions(+), 20 deletions(-)
> >>> create mode 100644 src/mjpeg-fallback.hpp
> >>> 
> >>> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> >>> index f41e68a..fac34bc 100644
> >>> --- a/src/mjpeg-fallback.cpp
> >>> +++ b/src/mjpeg-fallback.cpp
> >>> @@ -4,6 +4,8 @@
> >>>  * Copyright 2017 Red Hat Inc. All rights reserved.
> >>>  */
> >>> 
> >>> +#include "mjpeg-fallback.hpp"
> >>> +
> >>> #include <config.h>
> >>> #include <cstring>
> >>> #include <exception>
> >>> @@ -13,9 +15,6 @@
> >>> #include <syslog.h>
> >>> #include <X11/Xlib.h>
> >>> 
> >>> -#include <spice-streaming-agent/plugin.hpp>
> >>> -#include <spice-streaming-agent/frame-capture.hpp>
> >>> -
> >>> #include "static-plugin.hpp"
> >>> #include "jpeg.hpp"
> >>> 
> >> 
> >> The include order looks weird.
> >> Surely you want the config include before anything else as it could
> >> change some system header behaviour.
> >> But I think including mjpeg-fallback.hpp after the system headers
> >> here is more coherent.
> > 
> > This is an interesting topic :)
> > 
> > The reason for including the header corresponding to a .cpp file first
> > in it is that you will get undefined symbol errors if you are missing
> > includes in the header which are present in the .cpp (and which you
> > would put in front of it). This is described for example in the Google
> > C++ Style Guide [1].
> > 
> > If there's something I should be aware of in C, please educate me :)
> 
> +1 for Lukas’ rationale.
> 
> LLVM enforces this order too, see
> https://llvm.org/docs/CodingStandards.html#include-style.
> It’s actually enforced by clang-format in that case.
> 

I think I inherited the order from many time ago, is a good point.
In spice-server I have a script that creates for each header a C file that
include a single header just to test that the headers can be included.
Would save me this manual job! Note that the spice-server style is different
but this does not mean cannot be changed.

> Also, it is weird is to include <config.h> and not “config.h”… It’s a project
> header, not system header.
> 

I think this inherited too but you are right, should be "" syntax.

> 
> Thanks
> Christophe
> 

About including config.h first:


Short explanation: can save lot of pain.


Long explanation follow!

The config.h is generated by configure script for different reasons:
- allow to configure some options;
- detect available libraries;
- detect portability issues.
Now... porting Unix application can be really a pain.
In config.h there can be options that change the declarations inside
system headers!
Just an example: _GNU_SOURCE. Enables some additional
functions/structures/whatever specific to GNU. In some cases change
the name of some structure fields. Now it may be possible that for
instance your .hpp header include "string" which include "string.h"
file which have some declaration added/removed. As you know headers
are usually protected by some guards to avoid double/recursive
inclusion. So including config.h after/before changes the available
functions.
So far so good, program won't compile and you can fix it.
In my experience porting programs unfortunately what can happen is
much worse. Some systems (really, I don't remember) export a different
structure depending on some defines. What can happen is that structure
"foo" in moduleA have some size/fields and others in moduleB. You link
the two modules in the same executable library and when the program
run you'll get some stack/heap corruption! Yes, this is a real case!
Linux is quite developer friendly but fixing these header details in
some nasty Unix environment is really a pain!
Last year (2017) some people complained that C99 is too recent!
I remember other cases where changing system include order make the
same source crash.
A case I remember are unixODBC headers where different defines
make the header present 2 different ABI... but obviously the binary
libraries you are linking expose just one, not hard to imagine the
consequences running with the wrong defines.
Yes, should not happen but really if to avoid mostly of these nasty
issues you just have to include config.h first you do it.

Frediano

> 
> > 
> >>> @@ -41,11 +40,6 @@ static inline uint64_t get_time()
> >>> }
> >>> 
> >>> namespace {
> >>> -struct MjpegSettings
> >>> -{
> >>> -    int fps;
> >>> -    int quality;
> >>> -};
> >>> 
> >>> class MjpegFrameCapture final: public FrameCapture
> >>> {
> >>> @@ -69,18 +63,6 @@ private:
> >>>     uint64_t last_time = 0;
> >>> };
> >>> 
> >>> -class MjpegPlugin final: public Plugin
> >>> -{
> >>> -public:
> >>> -    FrameCapture *CreateCapture() override;
> >>> -    unsigned Rank() override;
> >>> -    void ParseOptions(const ConfigureOption *options);
> >>> -    SpiceVideoCodecType VideoCodecType() const {
> >>> -        return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> >>> -    }
> >>> -private:
> >>> -    MjpegSettings settings = { 10, 80 };
> >>> -};
> >>> }
> >>> 
> >>> MjpegFrameCapture::MjpegFrameCapture(const MjpegSettings&
> >>> settings):
> >>> @@ -205,6 +187,10 @@ void MjpegPlugin::ParseOptions(const
> >>> ConfigureOption
> >>> *options)
> >>>     }
> >>> }
> >>> 
> >>> +SpiceVideoCodecType MjpegPlugin::VideoCodecType() const {
> >>> +    return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> >>> +}
> >>> +
> >>> static bool
> >>> mjpeg_plugin_init(Agent* agent)
> >>> {
> >>> diff --git a/src/mjpeg-fallback.hpp b/src/mjpeg-fallback.hpp
> >>> new file mode 100644
> >>> index 0000000..8044244
> >>> --- /dev/null
> >>> +++ b/src/mjpeg-fallback.hpp
> >>> @@ -0,0 +1,34 @@
> >>> +/* Plugin implementation for Mjpeg
> >>> + *
> >>> + * \copyright
> >>> + * Copyright 2017 Red Hat Inc. All rights reserved.
> >>> + */
> >>> +#ifndef SPICE_STREAMING_AGENT_MJPEG_FALLBACK_HPP
> >>> +#define SPICE_STREAMING_AGENT_MJPEG_FALLBACK_HPP
> >>> +
> >>> +#include <spice-streaming-agent/plugin.hpp>
> >>> +#include <spice-streaming-agent/frame-capture.hpp>
> >>> +
> >>> +
> >>> +namespace SpiceStreamingAgent {
> >>> +
> >>> +struct MjpegSettings
> >>> +{
> >>> +    int fps;
> >>> +    int quality;
> >>> +};
> >>> +
> >>> +class MjpegPlugin final: public Plugin
> >>> +{
> >>> +public:
> >>> +    FrameCapture *CreateCapture() override;
> >>> +    unsigned Rank() override;
> >>> +    void ParseOptions(const ConfigureOption *options);
> >>> +    SpiceVideoCodecType VideoCodecType() const;
> >>> +private:
> >>> +    MjpegSettings settings = { 10, 80 };
> >>> +};
> >>> +
> >>> +} // namespace SpiceStreamingAgent
> >>> +
> >>> +#endif // SPICE_STREAMING_AGENT_MJPEG_FALLBACK_HPP
> >> 
> >> Frediano
> > 
> > Lukas
> > 
> > 
> > [1]
> > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

More information about the Spice-devel mailing list