Re: [flashrom] [PATCH] Partial write cleanups

[Michael Karcher]

Am Mittwoch, den 03.11.2010, 03:51 +0100 schrieb Carl-Daniel Hailfinger:
> Signed-off-by: Carl-Daniel Hailfinger
>
> diff -u flashrom-partial_write_rolling_erase_write/flashrom.c flashrom-partial_write_rolling_erase_write_cleanup/flashrom.c
> --- flashrom-partial_write_rolling_erase_write/flashrom.c	(Arbeitskopie)
> +++ flashrom-partial_write_rolling_erase_write_cleanup/flashrom.c	(Arbeitskopie)
> @@ -878,14 +878,11 @@
> length of the chip.
>
> */
>  static int get_next_write(uint8_t *have, uint8_t *want, int len,
> 	  int *first_start, int *first_len,
> 	  enum write_granularity gran)
> 	  int *first_start, enum write_granularity gran)
>
> {
> int result = 0, rel_start = 0;
> int result = 0, rel_start = 0, first_len = 0;

As "result" no longer is the return value, please rename the variable.
Something like "write_needed" would make sense, but...
> @@ -903,14 +900,14 @@
>    				/* First location where have and want
>    				 * do not differ anymore.
>    				 */
> 			*first_len = i - rel_start;
> 			first_len = i - rel_start;
> 		break;
> 	}
> }
> }
> /* Did the loop terminate without setting first_len? */
> if (result && ! *first_len)
> 	*first_len = i - rel_start;
> if (result && ! first_len)
> 	first_len = i - rel_start;

... you could also simplify this stuff by having a variable
"in_write_region" instead of result, which you clear in the block where
you set "first_len", and then the "if" would just need to check the
"in_write_region" flag instead of combining two things.

Or, wait a moment! Do you see some similarity between the assignment in
the if and the assignment before the break? Yes! The expressions are
identical. So: remove the first_len assignment before the break, you
also don't need to clear a flag in that block, just break. And then you
do the first_len assignment if the variable till now known as result is
set. The logic is that the loop always terminates at the end of the
region to write. Which might either be because of the loop end condition
or the matching compare - who cares, actually.

No Ack yet as I want to see the result if incorporating my comments into
this function before.
> +out:
> free(oldcontents);
> free(newcontents);
>
> +out_nofree:
>    programmer_shutdown();

if you initialize oldcontents and newcontents with NULL, free is
guaranteed by ANSI C to be a no-op, so you might get rid of the
out_nofree label. Whether one wants to make use of that feature or
considers not calling free at all if there was no malloc is a matter of
taste.

Regards,
  Michael Karcher