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

[Victor Toso]

Hi,

On Sat, Jan 26, 2019 at 10:02:02AM -0500, Frediano Ziglio wrote:
> > 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?

I wasn't sure too, that would be best.

> >  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.

Right... Can be improved. Just to be clear (trying to be), my
intention is return TRUE/FALSE but error is set when something
unexpected has happened, likely caller does not want to use or
cannot use @plugin_name.

> > +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.

Which data is uninitialized here?

> > +    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?

This comes from the framework itself, we can consider an error if
nmatch != 3.

> > +        *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?

Now moved to spice_check_gst_plugin_version()

> 
> > -        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.

I don't like much c99, previous code did not have it ;)

I don't like much goto either but I found it useful for cleanup,
debug and return proper value when necessary, here on error.


> > +        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 ?

Yes, thanks for the review.

> 
> >  G_END_DECLS
> >  
> >  #endif /* __SPICE_CLIENT_SESSION_PRIV_H__ */
> 
> Frediano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190127/8453a7ec/attachment.sig>