[PATCH] screenshooter: Add missing field initializers for wl_output_listener

[Pekka Paalanen]

On Tue, 7 Jul 2015 10:18:47 -0700
"Jasper St. Pierre" <jstpierre at mecheye.net> wrote:

> Wacky. In any case, NULL is not a valid listener, which is sort of
> terrible, but it is how it is. You need to create an empty function
> that does nothing.

No, you don't.

If one needs to shut up the compiler by adding NULLs, so be it. There
is absolutely no need to pretend anything else, you are only shutting
up the compiler. If anything ever happens to call that function
pointer, you want to know about it, because it's a bug in the server
most likely. So NULL will do just fine. A no-op function at best is
just useless and at worst hides real bugs.

I've never seen such warnings when building Weston, FWIW.

This is all part of the interface extension mechanism. A client is
allowed to be built against any revision of a protocol interface .xml
definition, as far as Wayland is concerned. The client build itself may
require a certain minimum version from the interface definitions. This
is all completely normal as usual, similar to requiring certain minimal
version of library headers to build against.

Assume a client program has been written and compiles fine without
warnings. Then, someone extends the protocol interface the client is
using, adding new events. This means that the listener struct grows new
fields at the end. This is perfectly safe and expected, and is
intended to not require any changes to the client.

Why is it safe? Because when the client binds to a global interface
through wl_registry, it negotiates an interface version at runtime.
The largest possible interface version is the minimum of three
things: the interface version available in the headers/XML file, the
interface version actually implemented in the client, and the interface
version advertised by the server. If negotiation results in v1, then
the server will never send events that were added in v2, just like
Daniel explained.

That is why it is not necessary to plug in valid functions or even
NULLs into the listener struct fields a client will never need.

I'm not sure, but I think we may even have version checks in libwayland
such, that if a buggy server sent an event that is not defined by the
negotiated interface version, it will be detected rather than calling
an uninitialized or NULL function pointer in the listener.

IMHO, in this particular case the compiler is warning without a reason.

If Weston's build does not exhibit these warnings, I'd rather not apply
this patch. If it does, I'd look into shutting up the compiler in a way
that we don't have to patch all places every time an interface grows.


Thanks,
pq

> On Tue, Jul 7, 2015 at 10:17 AM, Christopher Michael
> <cpmichael at osg.samsung.com> wrote:
> > On 07/07/2015 01:15 PM, Jasper St. Pierre wrote:
> >>
> >> Shouldn't missing fields in structs be auto-initialized to 0 / NULL? I
> >> thought that was part of the C specification.
> >>
> >
> > I thought so also however when compiling some other code which was also
> > creating a wl_output_listener, I uncovered the warnings about missing field
> > initializers. When I looked/referenced the existing screenshooting code I
> > noticed that they were missing from there also, so I just made a quick patch
> > to address that.
> >
> > Cheers,
> > Chris
> >
> >
> >> On Tue, Jul 7, 2015 at 8:52 AM, Christopher Michael
> >> <cpmichael at osg.samsung.com> wrote:
> >>>
> >>> This patch adds missing placeholders for the wl_output listener
> >>> functions 'done' and 'scale. Currently these placeholders are being
> >>> set to NULL as the done and scale callbacks are not used in the
> >>> screenshot client.
> >>>
> >>> Signed-off-by: Chris Michael <cp.michael at samsung.com>
> >>> ---
> >>>   clients/screenshot.c | 4 +++-
> >>>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/clients/screenshot.c b/clients/screenshot.c
> >>> index f11e3ba..0d9b320 100644
> >>> --- a/clients/screenshot.c
> >>> +++ b/clients/screenshot.c
> >>> @@ -114,7 +114,9 @@ display_handle_mode(void *data,
> >>>
> >>>   static const struct wl_output_listener output_listener = {
> >>>       display_handle_geometry,
> >>> -    display_handle_mode
> >>> +    display_handle_mode,
> >>> +    NULL,
> >>> +    NULL,
> >>>   };
> >>>
> >>>   static void
> >>> --