[vlc-devel] [PATCH 4/8] Separate out property marshalling code in D-Bus interfaces
mirsal
mirsal at mirsal.fr
Mon Jan 28 15:51:55 CET 2013
Hello Alex,
Minor comments inline, the rest looks good to me.
On Mon, 2013-01-28 at 11:54 +0000, Alex Merry wrote:
> Pull out code to marshal the various properties into separate functions
> (and make sure the return type of those functions is int). This is in
> preparation for adding org.freedesktop.DBus.Properties.GetAll support.
> ---
> modules/control/dbus/dbus_player.c | 106 ++++++++++++++++++++++------------
> modules/control/dbus/dbus_tracklist.c | 67 ++++++++++++++-------
> 2 files changed, 117 insertions(+), 56 deletions(-)
>
> diff --git a/modules/control/dbus/dbus_player.c b/modules/control/dbus/dbus_player.c
> index ef40688..e73b543 100644
> --- a/modules/control/dbus/dbus_player.c
> +++ b/modules/control/dbus/dbus_player.c
> @@ -39,19 +39,14 @@
> #include "dbus_player.h"
> #include "dbus_common.h"
>
> -static void MarshalLoopStatus ( intf_thread_t *, DBusMessageIter * );
> +static int MarshalLoopStatus ( intf_thread_t *, DBusMessageIter * );
>
> -DBUS_METHOD( Position )
> -{ /* returns position in microseconds */
> - REPLY_INIT;
> - OUT_ARGUMENTS;
> - DBusMessageIter v;
> +static int
> +MarshalPosition( intf_thread_t *p_intf, DBusMessageIter *container )
> +{
> dbus_int64_t i_pos;
> -
> - if( !dbus_message_iter_open_container( &args, DBUS_TYPE_VARIANT, "x", &v ) )
> - return DBUS_HANDLER_RESULT_NEED_MEMORY;
> -
> - input_thread_t *p_input = playlist_CurrentInput( PL );
> + input_thread_t *p_input;
> + p_input = playlist_CurrentInput( p_intf->p_sys->p_playlist );
>
> if( !p_input )
> i_pos = 0;
> @@ -62,9 +57,22 @@ DBUS_METHOD( Position )
> vlc_object_release( p_input );
> }
>
> - if( !dbus_message_iter_append_basic( &v, DBUS_TYPE_INT64, &i_pos ) )
> + dbus_message_iter_append_basic( container, DBUS_TYPE_INT64, &i_pos );
dbus_message_iter_append_basic can fail to allocate memory, the
marshaller should return VLC_ENOMEM in that case;
I know that it's not done consistently throughout the module, but new
code should behave correctly with regards to memory nonetheless.
> + return VLC_SUCCESS;
> +}
> +
> +DBUS_METHOD( Position )
> +{ /* returns position in microseconds */
> + REPLY_INIT;
> + OUT_ARGUMENTS;
> + DBusMessageIter v;
> +
> + if( !dbus_message_iter_open_container( &args, DBUS_TYPE_VARIANT, "x", &v ) )
> return DBUS_HANDLER_RESULT_NEED_MEMORY;
>
> + MarshalPosition( p_this, &v );
Missing return value check
> if( !dbus_message_iter_close_container( &args, &v ) )
> return DBUS_HANDLER_RESULT_NEED_MEMORY;
>
> @@ -165,7 +173,7 @@ DBUS_METHOD( Seek )
> REPLY_SEND;
> }
>
> -static void
> +static int
> MarshalVolume( intf_thread_t *p_intf, DBusMessageIter *container )
> {
> float f_vol = playlist_VolumeGet( p_intf->p_sys->p_playlist );
> @@ -174,6 +182,7 @@ MarshalVolume( intf_thread_t *p_intf, DBusMessageIter *container )
>
> double d_vol = f_vol;
> dbus_message_iter_append_basic( container, DBUS_TYPE_DOUBLE, &d_vol );
> + return VLC_SUCCESS;
> }
>
> DBUS_METHOD( VolumeGet )
> @@ -302,7 +311,7 @@ DBUS_METHOD( OpenUri )
> REPLY_SEND;
> }
>
> -static void
> +static int
> MarshalCanPlay( intf_thread_t *p_intf, DBusMessageIter *container )
> {
> playlist_t *p_playlist = p_intf->p_sys->p_playlist;
> @@ -312,6 +321,7 @@ MarshalCanPlay( intf_thread_t *p_intf, DBusMessageIter *container )
> PL_UNLOCK;
>
> dbus_message_iter_append_basic( container, DBUS_TYPE_BOOLEAN, &b_can_play );
> + return VLC_SUCCESS;
> }
>
> DBUS_METHOD( CanPlay )
> @@ -332,7 +342,7 @@ DBUS_METHOD( CanPlay )
> REPLY_SEND;
> }
>
> -static void
> +static int
> MarshalCanPause( intf_thread_t *p_intf, DBusMessageIter *container )
> {
> dbus_bool_t b_can_pause = FALSE;
> @@ -347,6 +357,7 @@ MarshalCanPause( intf_thread_t *p_intf, DBusMessageIter *container )
>
> dbus_message_iter_append_basic( container, DBUS_TYPE_BOOLEAN,
> &b_can_pause );
> + return VLC_SUCCESS;
> }
>
> DBUS_METHOD( CanPause )
> @@ -367,23 +378,29 @@ DBUS_METHOD( CanPause )
> REPLY_SEND;
> }
>
> -DBUS_METHOD( CanControl )
> +static int
> +MarshalCanControl( intf_thread_t *p_intf, DBusMessageIter *container )
> {
> - VLC_UNUSED( p_this );
> + VLC_UNUSED( p_intf );
> + dbus_bool_t b_can_control = TRUE;
>
> + dbus_message_iter_append_basic( container, DBUS_TYPE_BOOLEAN,
> + &b_can_control );
> + return VLC_SUCCESS;
> +}
> +
> +DBUS_METHOD( CanControl )
> +{
> REPLY_INIT;
> OUT_ARGUMENTS;
>
> DBusMessageIter v;
> - dbus_bool_t b_can_control = TRUE;
>
> if( !dbus_message_iter_open_container( &args, DBUS_TYPE_VARIANT,
> "b", &v ) )
> return DBUS_HANDLER_RESULT_NEED_MEMORY;
>
> - if( !dbus_message_iter_append_basic( &v, DBUS_TYPE_BOOLEAN,
> - &b_can_control ) )
> - return DBUS_HANDLER_RESULT_NEED_MEMORY;
> + MarshalCanControl( p_this, &v );
Same here
> if( !dbus_message_iter_close_container( &args, &v ) )
> return DBUS_HANDLER_RESULT_NEED_MEMORY;
> @@ -391,7 +408,7 @@ DBUS_METHOD( CanControl )
> REPLY_SEND;
> }
>
> -static void
> +static int
> MarshalCanSeek( intf_thread_t *p_intf, DBusMessageIter *container )
> {
> dbus_bool_t b_can_seek = FALSE;
> @@ -405,6 +422,7 @@ MarshalCanSeek( intf_thread_t *p_intf, DBusMessageIter *container )
> }
>
> dbus_message_iter_append_basic( container, DBUS_TYPE_BOOLEAN, &b_can_seek );
> + return VLC_SUCCESS;
> }
>
> DBUS_METHOD( CanSeek )
> @@ -425,11 +443,12 @@ DBUS_METHOD( CanSeek )
> REPLY_SEND;
> }
>
> -static void
> +static int
> MarshalShuffle( intf_thread_t *p_intf, DBusMessageIter *container )
> {
> dbus_bool_t b_shuffle = var_GetBool( p_intf->p_sys->p_playlist, "random" );
> dbus_message_iter_append_basic( container, DBUS_TYPE_BOOLEAN, &b_shuffle );
> + return VLC_SUCCESS;
> }
>
> DBUS_METHOD( ShuffleGet )
> @@ -463,7 +482,7 @@ DBUS_METHOD( ShuffleSet )
> REPLY_SEND;
> }
>
> -static void
> +static int
> MarshalPlaybackStatus( intf_thread_t *p_intf, DBusMessageIter *container )
> {
> input_thread_t *p_input;
> @@ -491,6 +510,7 @@ MarshalPlaybackStatus( intf_thread_t *p_intf, DBusMessageIter *container )
>
> dbus_message_iter_append_basic( container, DBUS_TYPE_STRING,
> &psz_playback_status );
> + return VLC_SUCCESS;
> }
>
> DBUS_METHOD( PlaybackStatus )
> @@ -511,7 +531,7 @@ DBUS_METHOD( PlaybackStatus )
> REPLY_SEND;
> }
>
> -static void
> +static int
> MarshalRate( intf_thread_t *p_intf, DBusMessageIter *container )
> {
> double d_rate;
> @@ -525,6 +545,7 @@ MarshalRate( intf_thread_t *p_intf, DBusMessageIter *container )
> d_rate = 0.;
>
> dbus_message_iter_append_basic( container, DBUS_TYPE_DOUBLE, &d_rate );
> + return VLC_SUCCESS;
> }
>
> DBUS_METHOD( RateGet )
> @@ -566,21 +587,27 @@ DBUS_METHOD( RateSet )
> REPLY_SEND;
> }
>
> -DBUS_METHOD( MinimumRate )
> +static int
> +MarshalMinimumRate( intf_thread_t *p_intf, DBusMessageIter *container )
> {
> - VLC_UNUSED( p_this );
> + VLC_UNUSED( p_intf );
> + double d_min_rate = (double) INPUT_RATE_MIN / INPUT_RATE_DEFAULT;
> +
> + dbus_message_iter_append_basic( container, DBUS_TYPE_DOUBLE, &d_min_rate );
> + return VLC_SUCCESS;
Please add a check for memory here as well
> +}
>
> +DBUS_METHOD( MinimumRate )
> +{
> REPLY_INIT;
> OUT_ARGUMENTS;
>
> DBusMessageIter v;
> - double d_min_rate = (double) INPUT_RATE_MIN / INPUT_RATE_DEFAULT;
>
> if( !dbus_message_iter_open_container( &args, DBUS_TYPE_VARIANT, "d", &v ) )
> return DBUS_HANDLER_RESULT_NEED_MEMORY;
>
> - if( !dbus_message_iter_append_basic( &v, DBUS_TYPE_DOUBLE, &d_min_rate ) )
> - return DBUS_HANDLER_RESULT_NEED_MEMORY;
> + MarshalMinimumRate( p_this, &v );
and check the return value of the marshaller
> if( !dbus_message_iter_close_container( &args, &v ) )
> return DBUS_HANDLER_RESULT_NEED_MEMORY;
> @@ -588,21 +615,27 @@ DBUS_METHOD( MinimumRate )
> REPLY_SEND;
> }
>
> -DBUS_METHOD( MaximumRate )
> +static int
> +MarshalMaximumRate( intf_thread_t *p_intf, DBusMessageIter *container )
> {
> - VLC_UNUSED( p_this );
> + VLC_UNUSED( p_intf );
> + double d_max_rate = (double) INPUT_RATE_MAX / INPUT_RATE_DEFAULT;
>
> + dbus_message_iter_append_basic( container, DBUS_TYPE_DOUBLE, &d_max_rate );
> + return VLC_SUCCESS;
Same here
> +}
> +
> +DBUS_METHOD( MaximumRate )
> +{
> REPLY_INIT;
> OUT_ARGUMENTS;
>
> DBusMessageIter v;
> - double d_max_rate = (double) INPUT_RATE_MAX / INPUT_RATE_DEFAULT;
>
> if( !dbus_message_iter_open_container( &args, DBUS_TYPE_VARIANT, "d", &v ) )
> return DBUS_HANDLER_RESULT_NEED_MEMORY;
>
> - if( !dbus_message_iter_append_basic( &v, DBUS_TYPE_DOUBLE, &d_max_rate ) )
> - return DBUS_HANDLER_RESULT_NEED_MEMORY;
> + MarshalMaximumRate( p_this, &v );
And here
> if( !dbus_message_iter_close_container( &args, &v ) )
> return DBUS_HANDLER_RESULT_NEED_MEMORY;
> @@ -610,7 +643,7 @@ DBUS_METHOD( MaximumRate )
> REPLY_SEND;
> }
>
> -static void
> +static int
> MarshalLoopStatus( intf_thread_t *p_intf, DBusMessageIter *container )
> {
> const char *psz_loop_status;
> @@ -626,6 +659,7 @@ MarshalLoopStatus( intf_thread_t *p_intf, DBusMessageIter *container )
>
> dbus_message_iter_append_basic( container, DBUS_TYPE_STRING,
> &psz_loop_status );
> + return VLC_SUCCESS;
> }
>
> DBUS_METHOD( LoopStatusGet )
> diff --git a/modules/control/dbus/dbus_tracklist.c b/modules/control/dbus/dbus_tracklist.c
> index 1081343..49b33d1 100644
> --- a/modules/control/dbus/dbus_tracklist.c
> +++ b/modules/control/dbus/dbus_tracklist.c
> @@ -4,10 +4,12 @@
> * Copyright © 2006-2011 Rafaël Carré
> * Copyright © 2007-2011 Mirsal Ennaime
> * Copyright © 2009-2011 The VideoLAN team
> + * Copyright © 2013 Alex Merry
> * $Id$
> *
> * Authors: Mirsal Ennaime <mirsal at mirsal fr>
> * Rafaël Carré <funman at videolanorg>
> + * Alex Merry <dev at randomguy3 me uk>
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as published by
> @@ -286,20 +288,16 @@ DBUS_METHOD( RemoveTrack )
> REPLY_SEND;
> }
>
> -DBUS_METHOD( Tracks )
> -{ /* Tracks property */
> - VLC_UNUSED( p_this );
> -
> - REPLY_INIT;
> - OUT_ARGUMENTS;
> -
> - DBusMessageIter tracks, v;
> - char *psz_track_id = NULL;
> - playlist_t *p_playlist = PL;
> +static int
> +MarshalTracks( intf_thread_t *p_intf, DBusMessageIter *container )
> +{
> + DBusMessageIter tracks;
> + char *psz_track_id = NULL;
> + playlist_t *p_playlist = p_intf->p_sys->p_playlist;
> input_item_t *p_input = NULL;
>
> - dbus_message_iter_open_container( &args, DBUS_TYPE_VARIANT, "ao", &v );
> - dbus_message_iter_open_container( &v, DBUS_TYPE_ARRAY, "o", &tracks );
> + dbus_message_iter_open_container( container, DBUS_TYPE_ARRAY, "o",
> + &tracks );
>
> PL_LOCK;
>
> @@ -315,8 +313,7 @@ DBUS_METHOD( Tracks )
> &psz_track_id ) )
> {
> PL_UNLOCK;
> - dbus_message_iter_abandon_container( &v, &tracks );
> - dbus_message_iter_abandon_container( &args, &v );
> + dbus_message_iter_abandon_container( container, &tracks );
> return DBUS_HANDLER_RESULT_NEED_MEMORY;
> }
>
> @@ -325,27 +322,57 @@ DBUS_METHOD( Tracks )
>
> PL_UNLOCK;
>
> - if( !dbus_message_iter_close_container( &v, &tracks ) ||
> - !dbus_message_iter_close_container( &args, &v ) )
> + if( !dbus_message_iter_close_container( container, &tracks ) )
> + return DBUS_HANDLER_RESULT_NEED_MEMORY;
> +
> + return VLC_SUCCESS;
> +}
> +
> +DBUS_METHOD( Tracks )
> +{ /* Tracks property */
> + REPLY_INIT;
> + OUT_ARGUMENTS;
> +
> + DBusMessageIter v;
> +
> + dbus_message_iter_open_container( &args, DBUS_TYPE_VARIANT, "ao", &v );
> +
> + if( MarshalTracks( p_this, &v ) != VLC_SUCCESS ) {
> + dbus_message_iter_abandon_container( &args, &v );
> + return DBUS_HANDLER_RESULT_NEED_MEMORY;
> + }
> +
> + if( !dbus_message_iter_close_container( &args, &v ) )
> return DBUS_HANDLER_RESULT_NEED_MEMORY;
>
> REPLY_SEND;
> }
>
> +static int
> +MarshalCanEditTracks( intf_thread_t *p_intf, DBusMessageIter *container )
> +{
> + VLC_UNUSED( p_intf );
> + const dbus_bool_t b_ret = TRUE;
> +
> + if( !dbus_message_iter_append_basic( container, DBUS_TYPE_BOOLEAN, &b_ret ) )
> + {
> + return DBUS_HANDLER_RESULT_NEED_MEMORY;
> + }
> +
> + return VLC_SUCCESS;
> +}
> +
> DBUS_METHOD( CanEditTracks )
> { /* CanEditTracks property */
> - VLC_UNUSED( p_this );
> REPLY_INIT;
> OUT_ARGUMENTS;
>
> DBusMessageIter v;
> - const dbus_bool_t b_ret = TRUE;
>
> if( !dbus_message_iter_open_container( &args, DBUS_TYPE_VARIANT, "b", &v ) )
> return DBUS_HANDLER_RESULT_NEED_MEMORY;
>
> - if( !dbus_message_iter_append_basic( &v, DBUS_TYPE_BOOLEAN, &b_ret ) )
> - {
> + if( MarshalCanEditTracks( p_this, &v ) != VLC_SUCCESS ) {
> dbus_message_iter_abandon_container( &args, &v );
> return DBUS_HANDLER_RESULT_NEED_MEMORY;
> }
I can amend your patch as my remarks are rather trivial
Cheers,
--
mirsal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20130128/e444568b/attachment.sig>
More information about the vlc-devel
mailing list