public inbox for linux-bluetooth@vger.kernel.org
 help / color / mirror / Atom feed
From: Johan Hedberg <johan.hedberg@gmail.com>
To: Jaikumar Ganesh <jaikumar@google.com>
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH] Update SDP storage records when a record is deleted.
Date: Fri, 23 Oct 2009 02:58:58 +0300	[thread overview]
Message-ID: <20091022235858.GA20063@jh-x301> (raw)
In-Reply-To: <1256253562-22532-1-git-send-email-jaikumar@google.com>

Hi Jaikumar,

A few quick coments on this:

On Thu, Oct 22, 2009, Jaikumar Ganesh wrote:
> +static void update_service_records(struct btd_device *device) {

Coding style: opening bracket of a function on its own line.

>  	records = read_records(&src, &device->bdaddr);
> +	for (l = device->uuids; l; l = l->next) {
> +		uuid = l->data;
> +		rec = find_record_in_list(records, uuid);
> +		if (rec)
> +			records = sdp_list_remove(records, rec);
> +	}

It looks like you're leaking memory here. I guess rec should be freed
after the sdp_list_remove.

> +	for (seq = records; seq; seq = seq->next) {
> +		rec = (sdp_record_t *) seq->data;

Since seq->data is void* the cast is unnecessary here.

> +		delete_record(srcaddr, dstaddr, rec->handle);
> +	}
> +	if (records)
> +		sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
> +}

Coding style: I'd add empty lines between the for-loops as well as before
the if-statement here.

Other than those issues one thing that bothers me a little is that it
seems the req->profiles_removed list isn't used anymore at this point but
a list of removed records is built from scratch a second time. Are you
sure that this existing list of removed UUIDs couldn't be used somehow to
make the code more efficient? It might well be that this I'm missing
something and this list can't be used but I thought it'd be good to check
that you've considered this possibility nevertheless.

Johan

  reply	other threads:[~2009-10-22 23:58 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-10-22 23:19 [PATCH] Update SDP storage records when a record is deleted Jaikumar Ganesh
2009-10-22 23:58 ` Johan Hedberg [this message]
2009-10-23 13:49 ` Luiz Augusto von Dentz
2009-10-23 15:17   ` Jaikumar Ganesh
2009-10-23 17:44     ` Luiz Augusto von Dentz
2009-10-26 15:20       ` Jaikumar Ganesh
2009-10-28 19:00         ` jaikumar Ganesh
2009-10-28 23:34         ` Johan Hedberg
2009-10-28 23:45           ` Jaikumar Ganesh
2009-10-28 23:54             ` Johan Hedberg
2009-10-29  0:02               ` Jaikumar Ganesh
2009-10-29  0:15                 ` Johan Hedberg
2009-10-29 18:42                   ` Jaikumar Ganesh
2009-10-29 20:30                     ` Jaikumar Ganesh
2009-10-29 20:34                       ` Luiz Augusto von Dentz
2009-10-29 21:04                         ` Jaikumar Ganesh
2009-10-29 21:13                           ` Johan Hedberg

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20091022235858.GA20063@jh-x301 \
    --to=johan.hedberg@gmail.com \
    --cc=jaikumar@google.com \
    --cc=linux-bluetooth@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox