Return-Path: <cyrus@holtmann.org>
MIME-Version: 1.0
In-Reply-To: <1308445186-27135-4-git-send-email-adamek.kuba@gmail.com>
References: <1308445186-27135-1-git-send-email-adamek.kuba@gmail.com>
	<1308445186-27135-4-git-send-email-adamek.kuba@gmail.com>
Date: Mon, 20 Jun 2011 11:14:45 +0300
Message-ID: <BANLkTikqYE7iL0w7DW9xaeUrRVg7yVeO1w@mail.gmail.com>
Subject: Re: [PATCH obexd 03/10] Store received headers in gw_obex_xfer object
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
To: Jakub Adamek <adamek.kuba@gmail.com>
Cc: linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset=ISO-8859-1
Sender: linux-bluetooth-owner@vger.kernel.org
List-ID: <linux-bluetooth.vger.kernel.org>

Hi Jakub,

On Sun, Jun 19, 2011 at 3:59 AM, Jakub Adamek <adamek.kuba@gmail.com> wrote:
> ---
> ?gwobex/obex-priv.c | ? 22 ++++++++++++++++++++++
> ?1 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/gwobex/obex-priv.c b/gwobex/obex-priv.c
> index aba7dd7..b9216e5 100644
> --- a/gwobex/obex-priv.c
> +++ b/gwobex/obex-priv.c
> @@ -39,6 +39,7 @@
> ?#include <sys/socket.h>
>
> ?#include <openobex/obex.h>
> +#include <openobex/obex_const.h>
>
> ?#ifdef HAVE_CONFIG_H
> ?# include "config.h"
> @@ -335,6 +336,7 @@ static void get_non_body_headers(obex_t *handle, obex_object_t *object,
> ? ? obex_headerdata_t hv;
> ? ? uint8_t hi;
> ? ? unsigned int hlen;
> + ? ?struct a_header *ah;
>
> ? ? xfer->target_size = GW_OBEX_UNKNOWN_LENGTH;
> ? ? xfer->modtime = -1;
> @@ -358,6 +360,26 @@ static void get_non_body_headers(obex_t *handle, obex_object_t *object,
> ? ? ? ? ? ? ? ? ? ? xfer->apparam_size = 0;
> ? ? ? ? ? ? ? ? break;
> ? ? ? ? ? ? default:
> + ? ? ? ? ? ? ? ?ah = g_new0(struct a_header, 1);
> + ? ? ? ? ? ? ? ?ah->hi = hi;
> + ? ? ? ? ? ? ? ?ah->hv_size = hlen;
> + ? ? ? ? ? ? ? ?switch (hi & OBEX_HDR_TYPE_MASK) {
> + ? ? ? ? ? ? ? ? ? ?case OBEX_HDR_TYPE_UINT8:
> + ? ? ? ? ? ? ? ? ? ?case OBEX_HDR_TYPE_UINT32:
> + ? ? ? ? ? ? ? ? ? ? ? ?ah->hv = hv;
> + ? ? ? ? ? ? ? ? ? ? ? ?break;
> + ? ? ? ? ? ? ? ? ? ?case OBEX_HDR_TYPE_BYTES:
> + ? ? ? ? ? ? ? ? ? ?case OBEX_HDR_TYPE_UNICODE:
> + ? ? ? ? ? ? ? ? ? ? ? ?ah->hv.bs = g_try_malloc(hlen);
> + ? ? ? ? ? ? ? ? ? ? ? ?if (ah->hv.bs) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?memcpy((void *) ah->hv.bs, hv.bs, hlen);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?ah->hv_size = hlen;
> + ? ? ? ? ? ? ? ? ? ? ? ?} else {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?ah->hv_size = hlen;
> + ? ? ? ? ? ? ? ? ? ? ? ?}

No need to add braces for single line statements.

> + ? ? ? ? ? ? ? ? ? ? ? ?break;
> + ? ? ? ? ? ? ? ?}
> + ? ? ? ? ? ? ? ?xfer->aheaders = g_slist_append(xfer->aheaders, ah);

I would suggest having this code separated in another function e.g.
get_aheader, it is easier to identify what the code is doing and
switch inside a switch is not very nice to read/understand IMO.

-- 
Luiz Augusto von Dentz