Hello Heinrich,
On Fri, Sep 17, 2021 at 5:36 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 9/16/21 4:09 PM, Oleksandr Suvorov wrote:
From: Ricardo Salveti ricardo@foundries.io
This patch optimizes the commit mentioned below by avoiding running a set of commands which useless in the case when
%s/which/which are/
size < mydata->sect_size and idx would be 0.
Thank you for reviewing the FAT coding.
Fixes: 5b3ddb17ba ("fs/fat/fat.c: Do not perform zero block reads if there are no blocks left")
Signed-off-by: Ricardo Salveti ricardo@foundries.io Co-developed-by: Oleksandr Suvorov oleksandr.suvorov@foundries.io Signed-off-by: Oleksandr Suvorov oleksandr.suvorov@foundries.io
fs/fat/fat.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 7021138b987..4a509755442 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -275,14 +275,10 @@ get_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer, unsigned long size) buffer += mydata->sect_size; size -= mydata->sect_size; }
} else {
} else if (size >= mydata->sect_size) { __u32 idx;The variable name idx is a misnomer. Can we call it sect_count?
Ok, sure.
idx = size / mydata->sect_size;if (idx == 0)ret = 0;size could be 0. In this case we still have to set ret = 0. Both for the DMA aligned and the unaligned case. You could initialize ret = 0 where it is defined.
What for? There is no chance to use uninitialized "ret" in that function.
elseret = disk_read(startsect, idx, buffer);
ret = disk_read(startsect, idx, buffer); if (ret != idx) { debug("Error reading data (got %d)\n", ret); return -1;291 idx *= mydata->sect_size; 292 buffer += idx; 293 size -= idx;
These lines should be adjusted, too: size is defined as unsigned long. We should not use the u32 variable idx but a separate variable of type unsigned long to hold the number of bytes read.
The original variant uses the same variable for a sectors counter and for the number of bytes read. I changed that part to use 2 different variables to make the code more readable. As for now, the maximum size of a cluster is 128 sectors (64Kb). This is the largest value for the parameter "size". And accordingly for the variables sect_count and bytes_read. IMHO even u16 would be enough for both these vars :) But the function disk_read() waits for __u32 as a 2nd parameter, so it would be more natural for sect_count to be __u32. What do you think?
Thanks for your detailed review!
I'm preparing the 2nd version of the patchset.
Best regards
Heinrich