>
> From: Victor Toso <me@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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.@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, µ) != 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel