[Bug 32477] Log Call events

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=32477

--- Comment #12 from Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> 2011-03-23 14:28:02 PDT ---
(In reply to comment #11)
> +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */
> 
> Not really (in lots of files).

Right, it seems wrong, I don't really know which editor uses that, this has
been copy-pasted over-and-over. Shall we simply remove them all ?

> 
> +++ b/telepathy-logger/call-event-internal.h
> +#include <telepathy-logger/streamed-media-channel-internal.h>
> 
> Seems unnecessary and wrong, remove it.

Fixed.

> 
> +++ b/telepathy-logger/call-event.h
> +  TPL_CALL_END_REASON_FORWARED,
> 
> FORWARDED

Fixed.

> 
> +++ b/telepathy-logger/call-event.c
> +#define DEBUG_FLAG TPL_DEBUG_LOG_STORE
> 
> TPL_DEBUG_LOG_EVENT sounds more appropriate.

Copy paste ... fixed.

> 
> + * An object representing a text log event.
> 
> s/text/call/

Fixed

> 
> +    "forwared",
> 
> forwarded

Been consistent in my typos, fixed. Also found missing ',' causing crash, same
in Call.

> 
> +static void
> +tpl_call_event_finalize (GObject * obj)
> +{
> +  G_OBJECT_CLASS (tpl_call_event_parent_class)->finalize (obj);
> +}
> 
> Don't override finalize for nothing.

Hmm, made me notice I don't free anything. Replaced with _dispose and
now tp_clear_object() and tp_clear_pointer() the end_actor and
detailed_end_reason. Will run this under valgrind and fix remaining if they
exists.

> 
> +  switch (param_id) {
> 
> put the brace on another line

Right, fixed.

> 
> +++ b/telepathy-logger/streamed-media-channel.c
> +  /* user_data is a TplChannel instance */
> +  tp_proxy_prepare_async (chan, chan_features, proxy_prepared_cb, ctx);
> 
> I'm not sure that comment is correct?

Actually the user_data is NULL, we use the object stored in the chain. This is
copy paste from Text channel, I've removed it from
Text/Call/StreamedMediaChannel.

> 
> +  if (error != NULL)
> +    {
> +      _tpl_action_chain_terminate (ctx, error);
> +      return;
> +    }
> 
> Add a DEBUG with error->message.

We give the error to the caller, it's up to the caller to decide if the error
is worth logging. In this case the caller do log it.

> 
> +  if (my_handle == 0)
> +      my_handle = tp_connection_get_self_handle (tp_conn);
> 
> That should be indented 2 spaces

Fixed.

> 
> +      "Unkown",
> 
> Unknown

I've fixed them all.

> 
> +      /* Workaround missing rejected reason. A call is rejected when the
> +       * receiver terminates that call before accepting it, and no other
> +       * reason was provided. Also, even if the call was not answered, the
> +       * spec inforces that the end_reason must be user_requested */
> 
> enforces
> 
> +  _tpl_log_manager_add_event (logmanager, TPL_EVENT (call_log), &error);
> +
> +  if (error != NULL)
> +    {
> +      PATH_DEBUG (self, "LogStore: %s", error->message);
> 
> LogStore seems wrong here. StreamedMediaChannel ?

Fixed.

> 
> 
> +  DEBUG ("New call, timestamp=%i-%02i-%02i %02i:%02i:%02i UTC",
> +      g_date_time_get_year (self->priv->timestamp),
> +      g_date_time_get_month (self->priv->timestamp),
> +      g_date_time_get_day_of_month (self->priv->timestamp),
> +      g_date_time_get_hour (self->priv->timestamp),
> +      g_date_time_get_minute (self->priv->timestamp),
> +      g_date_time_get_second (self->priv->timestamp));
> 
> g_date_time_format may come handy here.

Didn't want to alloc/free, but I agree it looks ugly, fixed, also fixed in
CallChannel.

> 
> 
> + * TplStreamedMediaChannel is actually a subclass of the abstract
> + * TplChannel which is a subclass of TpChannel.
> 
> It's rather a subclass of TpChannel, implementing TplChannel.

Also fixed in CallChannel, was changed very recently.

> 
> 
> +++ b/telepathy-logger/entity.c
> -      "avatar-token", avatar_token,
> +      "avatar-token", avatar_token == NULL ? "" : avatar_token,
> 
> I'm not sure of the benefits of that change. Can you explain them to me? It
> looks a bit ABI break too, though that's a bit borderline.

The reason is that for XML log store there is no difference between NULL and
empty string. Meaning that if you write NULL you will endup reading "". I'm
changing un-obfuscate the unit test and make the representation unique. This
change does not break API since it affect the writing part (you can't create an
event from the outside). The reading part was always getting "" anyway.

> 
> +++ b/telepathy-logger/log-store-xml.c
> 
> +  log_str = g_strdup_printf ("<call time='%s' "
> +      "id='%s' name='%s' isuser='%s' token='%s' "
> +      "duration='%" G_GINT64_FORMAT "' "
> +      "actor='%s' actortype='%s' "
> +      "actorname='%s' actortoken='%s' "
> +      "reason='%s' detail='%s'/>\n"
> 
> what is actor here? Also, is this multiconference-ready? If not, I guess that's
> not too important. We can probably extend it when we start supporting that in
> any clients, and maybe by then we're already using sqlite...

It's the actor as found in the Call_State_Reason, see
http://telepathy.freedesktop.org/spec/Channel_Type_Call.html#Struct:Call_State_Reason
. It's required to interpret the reason and detail. This actor could be anyone
in the conference room, e.g. an room admin kicked you. I'm not sure what's your
point about multiconference-ready + sqlite, if you mean interfaces Conference,
Splittable and MergeableConference, no they are not represented.

> 
> +        tpl_entity_get_avatar_token (actor) == NULL ?
> +           "" : tpl_entity_get_avatar_token (actor),
> 
> I guess you should escape that too.

escape ? that too ? Maybe you mean it's not required since I now create entity
with "" as default avatar_token ? I agree, but it's also an extra safety in the
case somebody break it in the future.

> 
> +++ b/telepathy-logger/call-channel.c
> +G_DEFINE_TYPE_WITH_CODE (TplCallChannel, _tpl_call_channel,
> +    TP_TYPE_CHANNEL,
> +    G_IMPLEMENT_INTERFACE (TPL_TYPE_CHANNEL, tpl_call_channel_iface_init))
> 
> Have you thought about inheriting from TpyCallChannel instead as you did with
> TpTextChannel? That could ease our job.

Yeah, as I already explained, I pretty much dislike the submodules and having
unstable static libraries being put in more then 1 project. I'll port it, but
unstable library like Yell are not my thing.

> 
> +#define TPL_IFACE_CHANNEL_TYPE_CALL \
> +  "org.freedesktop.Telepathy.Channel.Type.Call.DRAFT"
> +#define TPL_IFACE_QUARK_CHANNEL_TYPE_CALL tpl_get_channel_type_call_interface
> ()
> 
> +static GQuark
> +tpl_get_channel_type_call_interface (void)
> +{
> +  static GQuark interface = 0;
> +
> +  if (G_UNLIKELY (interface == 0))
> +    interface = g_quark_from_static_string (TPL_IFACE_CHANNEL_TYPE_CALL);
> +
> +  return interface;
> +}
> 
> We should use the tpy APIs here, e.g. TPY_IFACE_QUARK_CHANNEL_TYPE_CALL.
> 
> +typedef enum
> +{
> +  PENDING_INITIATOR_STATE = 1,
> +  PENDING_RECEIVER_STATE,
> +  ACCEPTED_STATE,
> +  ENDED_STATE,
> +} CallState;
> 
> TpyCallState
> 
> 
> +  /* user_data is a TplChannel instance */
> +  tp_proxy_prepare_async (chan, chan_features, proxy_prepared_cb, ctx);
> 
> not sure about the comment
> 
> +  GList *it;
> 
> It looks so much readable to do *l, but that's a personal preference :)

Btw, 'it' mean iterator, pretty much the same as 'l' for list.

> 
> +      "Unkown",
> 
> Again :p
> 
> 
> +    default:
> +      /* just wait */
> +      break;
> 
> a DEBUG may be useful there

>From the logger stand point, the other states does not mean anything, have a
look at the CM traces to debug CM.

> 
> 
> +  dbus_g_proxy_add_signal (proxy,
> +    "CallStateChanged",
> +    G_TYPE_UINT,
> +    G_TYPE_UINT,
> +    dbus_g_type_get_struct ("GValueArray",
> +        G_TYPE_UINT,
> +        G_TYPE_UINT,
> +        G_TYPE_STRING,
> +        G_TYPE_INVALID),
> +    dbus_g_type_get_map ("GHashTable", G_TYPE_STRING, G_TYPE_VALUE),
> +    G_TYPE_INVALID);
> +
> +  dbus_g_proxy_add_signal (proxy,
> +      "CallMembersChanged",
> +      dbus_g_type_get_map ("GHashTable", G_TYPE_UINT, G_TYPE_UINT),
> +      DBUS_TYPE_G_UINT_ARRAY,
> +      G_TYPE_INVALID);
> +
> +  dbus_g_proxy_connect_signal (proxy,
> +     "CallStateChanged",
> +      G_CALLBACK (call_state_changed_cb), self, NULL);
> +
> +  dbus_g_proxy_connect_signal (proxy,
> +     "CallMembersChanged",
> +      G_CALLBACK (call_members_changed_cb), self, NULL);
> +
> +  tp_g_signal_connect_object (TP_CHANNEL (self), "group-members-changed",
> +      G_CALLBACK (chan_members_changed_cb), self, 0);
> 
> Use TpyCallChannel and connect to its GObject signals. If something is missing,
> file a bug. There's also the tpy_cli_* APIs that would be better than this
> calls.
> 
> +  DEBUG ("New call, timestamp=%i-%02i-%02i %02i:%02i:%02i UTC",
> 
> Same comment as before
> 
> 
> +  dbus_g_object_register_marshaller (tpl_marshal_VOID__UINT_UINT_BOXED_BOXED,
> +      G_TYPE_NONE,
> +      G_TYPE_UINT, G_TYPE_UINT, G_TYPE_BOXED, G_TYPE_BOXED,
> +      G_TYPE_INVALID);
> +
> +  dbus_g_object_register_marshaller (tpl_marshal_VOID__BOXED_BOXED,
> +      G_TYPE_NONE,
> +      G_TYPE_BOXED, G_TYPE_BOXED,
> +      G_TYPE_INVALID);
> 
> And I guess you can then remove that.
> 
> That's for the first few commits, I'll continue tomorrow.

Ok, I'll have a look at having static yell copy inside logger, you don't have
to mention each part that would go away, I understand very well what I did, and
what the code generator puts in tpy_cli, and what the TpyCallChannel wrapper
should do.

I'm still highly concerned by recent issues found in the call API and potential
effect on the log format. I'll poke people tomorrow to figure-out if there is
intention to change the call state reasons, detailed reasons, etc.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.