[Spice-devel] [PATCH v1 1/2] gst: add helper for runtime check on gst plugin version

[Frediano Ziglio]

> 
> From: Victor Toso <me at victortoso.com>
> 
> This patch factor out the runtime check for pulsesrc plugin version in
> order to be used in the follow up patch by channel-display-gst.c
> 
> This code can be shared between spice-gstaudio.c and
> channel-display-gst.c.
> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/spice-gstaudio.c     | 89 ++++++++++++++++++++++++++--------------
>  src/spice-session-priv.h |  3 ++
>  2 files changed, 62 insertions(+), 30 deletions(-)
> 
> diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> index 5f4a2fe..4bab17f 100644
> --- a/src/spice-gstaudio.c
> +++ b/src/spice-gstaudio.c
> @@ -26,6 +26,7 @@
>  #include "spice-common.h"
>  #include "spice-session.h"
>  #include "spice-util.h"
> +#include "spice-session-priv.h"
>

I don't think this is a good header. Why spice-gstaudio needs to know the
internal details of spice-session?
  
>  struct stream {
>      GstElement              *pipe;
> @@ -549,45 +550,73 @@ static gboolean connect_channel(SpiceAudio *audio,
> SpiceChannel *channel)
>      return FALSE;
>  }
>  
> +/* Runtime check if a given @plugin_name is >= @major. at minor.@micro version
> in the system.
> + * Set @err and return FALSE on unexected errors */

This is false, on some cases FALSE is returned without setting
err.

> +G_GNUC_INTERNAL gboolean
> +spice_check_gst_plugin_version(const gchar *plugin_name,
> +                               guint major, guint minor, guint micro,
> +                               GError **err)
> +{
> +    GstPluginFeature *plugin_feature;
> +    GstPlugin *plugin;
> +    guint nmatch, plugin_major, plugin_minor, plugin_micro;
> +
> +    if (!gst_init_check(NULL, NULL, err)) {
> +        return FALSE;
> +    }
> +
> +    plugin_feature = gst_registry_lookup_feature(gst_registry_get(),
> plugin_name);
> +    if (!plugin_feature) {
> +        return FALSE;
> +    }
> +
> +    plugin = gst_plugin_feature_get_plugin(plugin_feature);
> +    nmatch = sscanf(gst_plugin_get_version(plugin), "%u.%u.%u",
> +                    &plugin_major, &plugin_minor, &plugin_micro);
> +
> +    spice_debug("Requiring %s version %u.%u.%u, system has %s",
> +                plugin_name, major, minor, micro,
> gst_plugin_get_version(plugin));

This would access uninitialized data if nmatch < 3.

> +    gst_object_unref(plugin);
> +    gst_object_unref(plugin_feature);
> +
> +    if (nmatch != 3) {

Maybe initializing plugin_micro to 0 and check < 2 here?
So to support versions like 1.14?

> +        *err = g_error_new(G_IO_ERROR, G_IO_ERROR_FAILED,
> +                           "bad versioning scheme for plugin %s",
> plugin_name);
> +        return FALSE;
> +    }
> +
> +    if (plugin_major != major) {
> +        return plugin_major > major;
> +    }
> +
> +    if (plugin_minor != minor) {
> +        return plugin_minor > minor;
> +    }
> +
> +    return plugin_micro >= micro;
> +}
> +
>  SpiceGstaudio *spice_gstaudio_new(SpiceSession *session, GMainContext
>  *context,
>                                    const char *name)
>  {
>      GError *err = NULL;
>  
> -    if (gst_init_check(NULL, NULL, &err)) {

Why not calling gst_init_check?

> -        GstPluginFeature *pulsesrc;
> -
> -        pulsesrc = gst_registry_lookup_feature(gst_registry_get(),
> "pulsesrc");
> -        if (pulsesrc) {
> -            unsigned major, minor, micro;
> -            GstPlugin *plugin = gst_plugin_feature_get_plugin(pulsesrc);
> -
> -            if (sscanf(gst_plugin_get_version(plugin), "%u.%u.%u",
> -                       &major, &minor, &micro) != 3) {
> -                g_warn_if_reached();
> -                gst_object_unref(plugin);
> -                gst_object_unref(pulsesrc);
> -                return NULL;
> -            }
> -
> -            if (major < 1 ||
> -                (major == 1 && minor < 14) ||
> -                (major == 1 && minor == 14 && micro < 5)) {
> -                g_warning("Bad pulsesrc version %s, lowering its rank",
> -                          gst_plugin_get_version(plugin));
> -                gst_plugin_feature_set_rank(pulsesrc, GST_RANK_NONE);
> -            }
> -
> -            gst_object_unref(plugin);
> -            gst_object_unref(pulsesrc);
> +    if (!spice_check_gst_plugin_version("pulsesrc", 1, 14, 5, &err)) {
> +        if (err != NULL) {
> +            goto err_out;

I personally don't like much gotos, previous code didn't have it.

>          }
> +        g_warning("Bad pulsesrc version, set plugin's rank to NONE");
>  
> -        return g_object_new(SPICE_TYPE_GSTAUDIO,
> -                            "session", session,
> -                            "main-context", context,
> -                            NULL);
> +        GstPluginFeature *pulsesrc =
> gst_registry_lookup_feature(gst_registry_get(), "pulsesrc");
> +        gst_plugin_feature_set_rank(pulsesrc, GST_RANK_NONE);
> +        gst_object_unref(pulsesrc);
>      }
>  
> +    return g_object_new(SPICE_TYPE_GSTAUDIO,
> +                        "session", session,
> +                        "main-context", context,
> +                        NULL);
> +err_out:
>      g_warning("Disabling GStreamer audio support: %s", err->message);
>      g_clear_error(&err);
>      return NULL;
> diff --git a/src/spice-session-priv.h b/src/spice-session-priv.h
> index 6ece7e0..fa14826 100644
> --- a/src/spice-session-priv.h
> +++ b/src/spice-session-priv.h
> @@ -98,6 +98,9 @@ guint spice_session_get_n_display_channels(SpiceSession
> *session);
>  gboolean spice_session_set_migration_session(SpiceSession *session,
>  SpiceSession *mig_session);
>  SpiceAudio *spice_audio_get(SpiceSession *session, GMainContext *context);
>  const gchar* spice_audio_data_mode_to_string(gint mode);
> +gboolean spice_check_gst_plugin_version(const gchar *plugin_name,
> +                               guint major, guint minor, guint micro,
> +                               GError **err);

Maybe in spice-utils.h ?

>  G_END_DECLS
>  
>  #endif /* __SPICE_CLIENT_SESSION_PRIV_H__ */

Frediano