Re: [PATCH 2/2] atmodem: add serial line debugging to gatchat

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2312 bytes --]

Hi Andres,

>  drivers/atmodem/atmodem.c |   13 ++++++++++++-
>  gatchat/gatchat.c         |   33 ++++++++++++++++++++++++++++++---
>  gatchat/gatchat.h         |    7 +++++--
>  include/log.h             |    5 +++++
>  src/log.c                 |   39 +++++++++++++++++++++++++++++++++------
>  src/main.c                |    1 +
>  6 files changed, 86 insertions(+), 12 deletions(-)

Send two separate patches, one for gatchat related functionality.  One for the 
ofono integration.

> +static void g_at_chat_debug_log(GAtChat *chat, gboolean is_read,
> +		char *str, gsize len)

I suggest you use const char *str here

> +{
> +	char type = is_read ? '<' : '>';
> +	char *tmp;
> +
> +	if (!chat->debugf || !len)
> +		return;
> +
> +	if (len > 1024) {
> +		/* let's keep it to a reasonable length.. */
> +		chat->debugf("%c (ignoring string of length %d)\n", type, len);

27.007 specifies a max command result length of 2048, Section 5.1:
"the total number of characters, including line terminators, in the 
information text shall not exceed 2048 characters."

> +		return;
> +	}
> +
> +	str[len] = '\0';	/* XXX: is it safe to overwrite this byte? */

Probably not

> +	tmp = g_strescape(str, "\"");
> +	chat->debugf("%c %s\n", type, tmp);
> +	g_free(tmp);
> +}
> +
> -GAtChat *g_at_chat_new(GIOChannel *channel, int flags)
> +GAtChat *g_at_chat_new(GIOChannel *channel, int flags, GAtDebugFunc

Lets make the debug function settable / gettable.  I.e. something like:

g_at_chat_set_debug_info(GAtChat *chat, GAtDebugFunc *debugf, void *debugdata)
g_at_chat_get_debug_func(GAtChat *chat)
g_at_chat_get_debug_data(GAtChat *chat)

>  GAtChat *g_at_chat_ref(GAtChat *chat)
> diff --git a/gatchat/gatchat.h b/gatchat/gatchat.h
> index 969f6f4..4353e08 100644
> --- a/gatchat/gatchat.h
> +++ b/gatchat/gatchat.h
> @@ -36,6 +36,8 @@ typedef void (*GAtResultFunc)(gboolean success, GAtResult
> *result, gpointer user_data);
>  typedef void (*GAtNotifyFunc)(GAtResult *result, gpointer user_data);
>  typedef void (*GAtDisconnectFunc)(gpointer user_data);
> +typedef void (*GAtDebugFunc)(const char *format, ...)
> +		__attribute__((format(printf, 1, 2)));
>

make the signature a simple void (*foo)(const char *str, void *data);

Regards,
-Denis