[Spice-devel] [PATCH spice-streaming-agent v2 2/5] Add file with utilities for Xorg

[Lukáš Hrázký]

Hi,

On Mon, 2018-05-21 at 11:45 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  src/Makefile.am               |  2 ++
>  src/spice-streaming-agent.cpp |  1 +
>  src/xorg-utils.cpp            | 40 +++++++++++++++++++++++++++++++++++
>  src/xorg-utils.hpp            | 13 ++++++++++++
>  4 files changed, 56 insertions(+)
>  create mode 100644 src/xorg-utils.cpp
>  create mode 100644 src/xorg-utils.hpp
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 18ed22c..276478f 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -54,6 +54,8 @@ spice_streaming_agent_SOURCES = \
>  	concrete-agent.hpp \
>  	error.cpp \
>  	error.hpp \
> +	xorg-utils.cpp \
> +	xorg-utils.hpp \
>  	mjpeg-fallback.cpp \
>  	mjpeg-fallback.hpp \
>  	jpeg.cpp \
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 2325119..6388bb2 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -9,6 +9,7 @@
>  #include "mjpeg-fallback.hpp"
>  #include "stream-port.hpp"
>  #include "error.hpp"
> +#include "xorg-utils.hpp"
>  
>  #include <spice/stream-device.h>
>  #include <spice/enums.h>
> diff --git a/src/xorg-utils.cpp b/src/xorg-utils.cpp
> new file mode 100644
> index 0000000..e03a51e
> --- /dev/null
> +++ b/src/xorg-utils.cpp
> @@ -0,0 +1,40 @@
> +/* Utilities dealing with Xorg server
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +
> +#include <config.h>
> +#include "xorg-utils.hpp"
> +
> +#include <exception>
> +#include <stdexcept>
> +#include <X11/Xatom.h>
> +

You're missing the namespace here.

> +int
> +get_win_prop_int(Display *display, Window win, Atom atom)

Instead of Atom as the property indentifier, would it perhaps be better
to have a string name in the function interface? It seems the Atom will
always be created the same way, so we can spare the caller the
necessity.

> +{
> +    int status;
> +    unsigned char *prop;
> +    unsigned long bytes_after, nitems;
> +    int actual_format;
> +    Atom actual_type;
> +
> +    status = XGetWindowProperty(display, win, atom, 0, 64,
> +                                False, XA_INTEGER, &actual_type,
> +                                &actual_format, &nitems, &bytes_after,
> +                                &prop);
> +    if (status == Success && nitems > 0) {
> +        switch (actual_format) {
> +        case 8:
> +            return *(const signed char *)prop;
> +        case 16:
> +            return *(const short *)prop;
> +        case 32:
> +            // although format is 32 is represented always as a long which
> +            // could be 64 bit
> +            return *(const long *)prop;
> +        }
> +    }
> +    throw std::runtime_error("error getting property");

This is really lackluster error handling.

1. The message is way too generic, imagine getting this in the log and
not actually knowing the code. It doesn't even say it's an X Window
property, it could be absolutely anything. You should also include the
name of the property, possibly an identifier of the Window and most
importantly the actuall error description.

2. Since we got the error hierarchy, we should use it. I'm inclined to
say that instead of jamming all errors into error.{c,h}pp, it will
probably be better to put specific errors along with the code that
throws them. By that I mean to have something like this in the xorg-
utils.hpp header:

#include <error.h>

//...

class XPropertyError : public Error
{
//...
};

> +}
> diff --git a/src/xorg-utils.hpp b/src/xorg-utils.hpp
> new file mode 100644
> index 0000000..91ee930
> --- /dev/null
> +++ b/src/xorg-utils.hpp
> @@ -0,0 +1,13 @@
> +/* Utilities dealing with Xorg server
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +#ifndef STREAMING_AGENT_XORG_UTILS_HPP
> +#define STREAMING_AGENT_XORG_UTILS_HPP
> +
> +#include <X11/Xlib.h>
> +
> +int get_win_prop_int(Display *display, Window win, Atom atom);
> +
> +#endif // STREAMING_AGENT_XORG_UTILS_HPP