[U-Boot] [PATCH 06/13] fastboot: Move fastboot response functions to fastboot core

[Tom Rini]

On Sun, Sep 06, 2015 at 06:11:37PM +0200, Maxime Ripard wrote:
> On Fri, Sep 04, 2015 at 01:20:44PM -0400, Tom Rini wrote:
> > On Mon, Aug 31, 2015 at 04:46:06PM +0200, Maxime Ripard wrote:
> > 
> > > The functions and a few define to generate a fastboot message to be sent
> > > back to the host were so far duplicated among the users.
> > > 
> > > Move them all to a common place.
> > [snip]
> > > diff --git a/common/aboot.c b/common/aboot.c
> > > index 18ff30ee6d11..37ad50efc50a 100644
> > > --- a/common/aboot.c
> > > +++ b/common/aboot.c
> > > @@ -277,7 +277,7 @@ void store_sparse_image(sparse_storage_t *storage,
> > >  
> > >  	sparse_header = sparse_parse_header(&data);
> > >  	if (!sparse_header) {
> > > -		fastboot_fail("sparse header issue\n");
> > > +		printf("sparse header issue\n");
> > >  		return;
> > >  	}
> > >  
> > > @@ -288,7 +288,6 @@ void store_sparse_image(sparse_storage_t *storage,
> > >  	if (sparse_header->blk_sz % storage->block_sz) {
> > >  		printf("%s: Sparse image block size issue [%u]\n",
> > >  		       __func__, sparse_header->blk_sz);
> > > -		fastboot_fail("sparse image block size issue");
> > >  		return;
> > >  	}
> > >  
> > > @@ -299,7 +298,7 @@ void store_sparse_image(sparse_storage_t *storage,
> > >  	for (chunk = 0; chunk < sparse_header->total_chunks; chunk++) {
> > >  		chunk_header = sparse_parse_chunk(sparse_header, &data);
> > >  		if (!chunk_header) {
> > > -			fastboot_fail("Unknown chunk type");
> > > +			printf("Unknown chunk type");
> > >  			return;
> > >  		}
> > >  
> > > @@ -314,7 +313,6 @@ void store_sparse_image(sparse_storage_t *storage,
> > >  		if (blk + blkcnt > storage->start + storage->size) {
> > >  			printf("%s: Request would exceed partition size!\n",
> > >  			       __func__);
> > > -			fastboot_fail("Request would exceed partition size!");
> > >  			return;
> > >  		}
> > >  
> > > @@ -331,7 +329,6 @@ void store_sparse_image(sparse_storage_t *storage,
> > >  			if (buffer_blks != buffer_blk_cnt) {
> > >  				printf("%s: Write %d failed %d\n",
> > >  				       __func__, i, buffer_blks);
> > > -				fastboot_fail("flash write failure");
> > >  				return;
> > >  			}
> > >  
> > > @@ -348,9 +345,10 @@ void store_sparse_image(sparse_storage_t *storage,
> > >  	printf("........ wrote %u bytes to '%s'\n", bytes_written,
> > >  	       storage->name);
> > >  
> > > -	if (total_blocks != sparse_header->total_blks)
> > > -		fastboot_fail("sparse image write failure");
> > > +	if (total_blocks != sparse_header->total_blks) {
> > > +		printf("sparse image write failure");
> > > +		return;
> > > +	}
> > >  
> > > -	fastboot_okay("");
> > >  	return;
> > >  }
> > 
> > Why in the case of this image do we not need to do fastboot_fail/okay
> > now?  It's not obvious from the rest of the changes to me, thanks!
> 
> I took that shortcut while refactoring and forgot to fix it...
> 
> The issue is that we don't have access to the response buffer in those
> functions. We could pass it as an argument, but that would:
> 
>   - Prevent the calling function (that "owns" the buffer pointer) to
>     use fastboot_fail / fastboot_okay, which feels a bit wrong.
> 
>   - On a more theorical point, the sparse image format should be
>     decoupled from the fastboot protocol, hence not really rely of
>     these functions.
> 
> What we can do though, is simply return an error code, and the storage
> layers could use fastboot_okay / fastboot_fail themselves. Does that
> sound good?

Error code and letting the higher level deal with it sounds good to me!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150906/799efb0f/attachment.sig>