On Thursday, August 07, 2014 at 01:48:06 AM, Steve Rae wrote:
> On 14-07-30 06:37 PM, Marek Vasut wrote:
> > On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote:
> > [...]
> >
> >> +
> >> +#include <common.h>
> >> +#include <fb_mmc.h>
> >> +#include <part.h>
> >> +#include <sparse_format.h>
> >> +
> >> +/* The 64 defined bytes plus \0 */
> >> +#define RESPONSE_LEN (64 + 1)
> >> +
> >> +static char *response_str;
> >
> > I'd suggest to pass this "response_str" around instead of making it
> > global.
>
> That would involve adding it to fastboot_resp(), which is called 11
> times in this code, from 3 different functions (so would need to add
> this to two of the functions...). And as these evolve, there will likely
> be more nested functions, which would all require "passing it around"....
> I think that this "static global pointer" is a cleaner implementation.
Eventually, the amount of these static variables in the code will grow and it
will become increasingly difficult to weed them out. I believe it would be even
better to pass around some kind of a structure with "private data" of the
fastboot, which would cater for all possible variables which might come in the
future. What do you think ?
> >> +static void fastboot_resp(const char *s)
> >> +{
> >> + strncpy(response_str, s, RESPONSE_LEN);
> >> + response_str[RESPONSE_LEN - 1] = '\0';
> >
> > This could be shrunk to a single snprintf(response_str, RESPONSE_LENGTH,
> > s); I think, but I'm not sure if the overhead won't grow.
>
> snprintf() is used very sparingling in U-Boot
This is not a reason to avoid it.
> , and with the cautionary statements in README (line 852)
Which statements? Can you please point them out? I fail to see them, sorry.
> and the fact that CONFIG_SYS_VSNPRINTF is not defined for armv7 builds, I am
not going to use it....
Is it a problem to define it? Also, even without CONFIG_SYS_VSNPRINTF , the
functions are still available, see the README:
857 If this option is not given then these functions will
858 silently discard their buffer size argument - this means
859 you are not getting any overflow checking in this case.
I have yet to see some hard-evidence against using safe printing functions here.
> >> +}
> >> +
> >> +static int is_sparse_image(void *buf)
> >> +{
> >> + sparse_header_t *s_header = (sparse_header_t *)buf;
> >> +
> >> + if ((le32_to_cpu(s_header->magic) == SPARSE_HEADER_MAGIC) &&
> >> + (le16_to_cpu(s_header->major_version) == 1))
> >> + return 1;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void write_sparse_image(block_dev_desc_t *dev_desc,
> >> + disk_partition_t *info, const char *part_name,
> >> + void *buffer, unsigned int download_bytes)
> >> +{
> >> + lbaint_t blk;
> >> + lbaint_t blkcnt;
> >> + lbaint_t blks;
> >> + sparse_header_t *s_header = (sparse_header_t *)buffer;
> >> + chunk_header_t *c_header;
> >> + void *buf;
> >> + uint32_t blk_sz;
> >> + uint32_t remaining_chunks;
> >> + uint32_t bytes_written = 0;
> >> +
> >> + blk_sz = le32_to_cpu(s_header->blk_sz);
> >> +
> >> + /* verify s_header->blk_sz is exact multiple of info->blksz */
> >> + if (blk_sz != (blk_sz & ~(info->blksz - 1))) {
> >> + printf("%s: Sparse image block size issue [%u]\n",
> >> + __func__, blk_sz);
> >> + fastboot_resp("FAILsparse image block size issue");
> >
> > Can't you just make the fastboot_resp() function a variadic one AND move
> > the printf() into the fastboot_resp() function? You could then even get
> > consistent output on both the device and in the response if you
> > snprintf() into the response_str first and then printf() the
> > response_str .
>
> Generally, the printf() statements which are sent to the console, and
> the fastboot_resp() statements which are sent to the host running the
> "fastboot" application are not the same....
OK, thanks!
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
