On 14-08-06 05:13 PM, Marek Vasut wrote:
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 ?
Yes -- if there is private data that the fastboot implementation
requires, then a data structure is the way to go. However, I still think
that this "fastboot response string" would even be an exception to that
private data....
+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.
true....
, and with the cautionary statements in README (line 852)
Which statements? Can you please point them out? I fail to see them, sorry.
I was referring to what you mention below...
852 - Safe printf() functions
853 Define CONFIG_SYS_VSNPRINTF to compile in safe versions of
854 the printf() functions. These are defined in
855 include/vsprintf.h and include snprintf(), vsnprintf() and
856 so on. Code size increase is approximately 300-500 bytes.
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.
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.
I don't want to be the first to defined it for all of armv7....
And I really don't want to define it only only my boards running so that
they can run 'fastboot'
What do you suggest?
+}
+
+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!
Thanks, Steve
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
