From mboxrd@z Thu Jan  1 00:00:00 1970
From: Lukasz Majewski <l.majewski@samsung.com>
Date: Mon, 23 Jul 2012 18:11:05 +0200
Subject: [U-Boot] [PATCH 3/7] dfu: DFU backend implementation
In-Reply-To: <201207200032.01591.vapier@gentoo.org>
References: <1341308291-14663-1-git-send-email-l.majewski@samsung.com>
	<1341308291-14663-4-git-send-email-l.majewski@samsung.com>
	<201207200032.01591.vapier@gentoo.org>
Message-ID: <20120723181105.79440da7@amdc308.digital.local>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Mike Frysinger,

> On Tuesday 03 July 2012 05:38:07 Lukasz Majewski wrote:
> > +		puts("UPLOAD ... done\n");
> > +		puts("Ctrl+C to exit ...\n");
> 
> combine into a single puts() ?
Ok, will be done
> 
> > +static int dfu_fill_entity(struct dfu_entity *dfu, char* s, int
> > alt,
> > +			    char *interface, int num)
> > +{
> > +	char *st = NULL;
> > +	int n = 0;
> > +
> > +	debug("%s: %s interface: %s num: %d\n", __func__, s,
> > interface, num); +
> > +	st = dfu_extract_token(&s, &n);
> > +	strncpy((char *) &dfu->name, st, n);
> 
> what's with the pointless cast ?  just do:
> 	strncpy(dfu->name, st, n);
Yes, you are right.

> 
> also, "n" here is wrong.  it should be sizeof(dfu->name), not
> (presumably) the length of st.  considering this is the 2nd time i
> noticed this bug in the dfu patchset, you might want to do a grep on
> your patches to see if your other string related usage is wrong.

I will double-check the string operations.

> 
> > +void dfu_free_entities(void)
> > +{
> > +	struct dfu_entity *dfu = NULL, *p = NULL;
> 
> no point in assigning to NULL here

Will remove.
> 
> > +static char *dfu_get_dev_type(enum dfu_device_type t)
> 
> static const char *...
Will correct.
> 
> > +{
> > +	static char *dev_t[] = {NULL, "MMC", "ONENAND", "NAND" };
> 
> static const char * const dev_t[] = {...}
Ok
> 
> > +static char *dfu_get_layout(enum dfu_device_type l)
> 
> static const char *...
> 
Ok
> > +{
> > +	static char *dfu_layout[] = {NULL, "RAW_ADDR", "FAT",
> > "EXT" };
> 
> static const char * const dfu_layout[] = {...}
> 
OK
> > +void dfu_show_entities(void)
> > +{
> > +	struct dfu_entity *dfu = NULL;
> 
> no point in assigning to NULL
OK
> 
> > +struct dfu_entity *dfu_get_entity(int alt)
> > +{
> > +	struct dfu_entity *dfu = NULL;
> 
> no point in assigning to NULL
OK
> -mike



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group