Hi,
On Sat, Jan 26, 2019 at 10:02:02AM -0500, Frediano Ziglio wrote:
> > 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?
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.@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, µ) != 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel