diff mbox series

[LEDE-DEV,2/2] fstools: Fix some errors detected by cppcheck

Message ID 1512881682-28288-2-git-send-email-rosenp@gmail.com
State Changes Requested
Headers show
Series [LEDE-DEV,1/2] fstools: Replace strerror(errno) with %m format. | expand

Commit Message

Rosen Penev Dec. 10, 2017, 4:54 a.m. UTC 
Mainly plugging memory leaks. Size reduction as well. The calloc change accounts for 272 bytes on this machine for some reason...

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 block.c                  |  6 +++---
 blockd.c                 |  3 +++
 libfstools/overlay.c     |  2 +-
 libfstools/rootdisk.c    |  7 ++++---
 libfstools/snapshot.c    | 11 +++++++----
 libfstools/ubi.c         | 13 +++++++------
 libubi/libubi.c          |  4 ++--
 libubi/ubiutils-common.c |  2 +-
 8 files changed, 28 insertions(+), 20 deletions(-)

Comments

Arjen de Korte Dec. 10, 2017, 6:17 p.m. UTC | #1 
Citeren Rosen Penev <rosenp@gmail.com>:

> Mainly plugging memory leaks. Size reduction as well. The calloc  
> change accounts for 272 bytes on this machine for some reason...

Comments inline.

> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  block.c                  |  6 +++---
>  blockd.c                 |  3 +++
>  libfstools/overlay.c     |  2 +-
>  libfstools/rootdisk.c    |  7 ++++---
>  libfstools/snapshot.c    | 11 +++++++----
>  libfstools/ubi.c         | 13 +++++++------
>  libubi/libubi.c          |  4 ++--
>  libubi/ubiutils-common.c |  2 +-
>  8 files changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/block.c b/block.c
> index ab130b9..6117701 100644
> --- a/block.c
> +++ b/block.c
> @@ -316,8 +316,7 @@ static int swap_add(struct uci_section *s)
>  	if (!tb[SWAP_UUID] && !tb[SWAP_LABEL] && !tb[SWAP_DEVICE])
>  		return -1;
>
> -	m = malloc(sizeof(struct mount));
> -	memset(m, 0, sizeof(struct mount));
> +	m = calloc(1, sizeof(struct mount));
>  	m->type = TYPE_SWAP;
>  	m->uuid = blobmsg_get_strdup(tb[SWAP_UUID]);
>  	m->label = blobmsg_get_strdup(tb[SWAP_LABEL]);

This fails to address the real issue here, there is no check for a  
NULL pointer after allocating memory.

> @@ -1084,7 +1083,7 @@ static int mount_device(struct probe_info *pr,  
> int type)
>  static int umount_device(struct probe_info *pr)
>  {
>  	struct mount *m;
> -	char *device = basename(pr->dev);
> +	char *device;
>  	char *mp;
>  	int err;
>
> @@ -1098,6 +1097,7 @@ static int umount_device(struct probe_info *pr)
>  	if (!mp)
>  		return -1;
>
> +	device = basename(pr->dev);
>  	m = find_block(pr->uuid, pr->label, device, NULL);
>  	if (m && m->extroot)
>  		return -1;

What exactly is the above change supposed to fix?

> diff --git a/blockd.c b/blockd.c
> index 3af5390..f7a4dec 100644
> --- a/blockd.c
> +++ b/blockd.c
> @@ -115,6 +115,9 @@ device_free(struct device *device)
>  	char *target = NULL;
>  	char *path = NULL, _path[64], *mp;
>
> +	if (!device)
> +		return;
> +
>  	blobmsg_parse(mount_policy, __MOUNT_MAX, data,
>  		      blob_data(device->msg), blob_len(device->msg));
>

I would rather fix this in the devices_update_cb() function instead.  
Only in line 212 there is a change the device_free() function is  
called will a NULL pointer, but a couple of lines down, there is  
already a check for a NULL pointer. One might fix this in one go by  
using

         if (device_o) {
                 device_free(device_o);
                 free(device_o);
         }

> diff --git a/libfstools/overlay.c b/libfstools/overlay.c
> index 7ada5ff..d7a55e6 100644
> --- a/libfstools/overlay.c
> +++ b/libfstools/overlay.c
> @@ -284,7 +284,7 @@ enum fs_state fs_state_get(const char *dir)
>  {
>  	char *path;
>  	char valstr[16];
> -	uint32_t val;
> +	int val;
>  	ssize_t len;
>
>  	path = alloca(strlen(dir) + 1 + sizeof("/.fs_state"));

This just silences a warning, without fixing the (potential)  
underlying problem. If the result of the atoi() somehow is not a  
member of the enum fs_state (which is guaranteed > 0 numerically), the  
function will return a non-member either way.

> diff --git a/libfstools/rootdisk.c b/libfstools/rootdisk.c
> index dd00c1b..2bb16d4 100644
> --- a/libfstools/rootdisk.c
> +++ b/libfstools/rootdisk.c
> @@ -171,8 +171,10 @@ static int rootdisk_volume_identify(struct volume *v)
>
>  	fseeko(f, p->offset + 0x400, SEEK_SET);
>  	n = fread(&magic, sizeof(magic), 1, f);
> -	if (n != 1)
> +	if (n != 1) {
> +		fclose(f);
>  		return -1;
> +	}
>
>  	if (magic == cpu_to_le32(0xF2F52010))
>  		ret = FS_F2FS;
> @@ -180,13 +182,12 @@ static int rootdisk_volume_identify(struct volume *v)
>  	magic = 0;
>  	fseeko(f, p->offset + 0x438, SEEK_SET);
>  	n = fread(&magic, sizeof(magic), 1, f);
> +	fclose(f);
>  	if (n != 1)
>  		return -1;
>  	if ((le32_to_cpu(magic) & 0xffff) == 0xef53)
>  		ret = FS_EXT4;
>
> -	fclose(f);
> -
>  	return ret;
>  }
>

This actually makes sense to prevent leaking file pointers.

> diff --git a/libfstools/snapshot.c b/libfstools/snapshot.c
> index 4870cf7..58bed96 100644
> --- a/libfstools/snapshot.c
> +++ b/libfstools/snapshot.c
> @@ -203,16 +203,19 @@ snapshot_read_file(struct volume *v, int  
> block, char *file, uint32_t type)
>  		if (hdr.length < len)
>  			len = hdr.length;
>
> -		if (volume_read(v, buffer, offset, len))
> +		if (volume_read(v, buffer, offset, len)) {
> +			close(out);
>  			return -1;
> -		if (write(out, buffer, len) != len)
> +		}
> +
> +		int w = write(out, buffer, len);
> +		close(out);
> +		if (w != len)
>  			return -1;
>  		offset += len;
>  		hdr.length -= len;
>  	}
>
> -	close(out);
> -
>  	if (verify_file_hash(file, hdr.md5)) {
>  		ULOG_ERR("md5 verification failed\n");
>  		unlink(file);

Same here, this makes sense to do.

> diff --git a/libfstools/ubi.c b/libfstools/ubi.c
> index f9d6e0a..678b8bf 100644
> --- a/libfstools/ubi.c
> +++ b/libfstools/ubi.c
> @@ -60,6 +60,7 @@ static char
>  	FILE *f;
>  	char fname[BUFLEN];
>  	char buf[BUFLEN];
> +	char *s;
>  	int i;
>
>  	snprintf(fname, sizeof(fname), "%s/%s", dirname, filename);
> @@ -68,10 +69,10 @@ static char
>  	if (!f)
>  		return NULL;
>
> -	if (fgets(buf, sizeof(buf), f) == NULL)
> -		return NULL;
> -
> +	s = fgets(buf, sizeof(buf), f);
>  	fclose(f);
> +	if (s == NULL)
> +		return NULL;
>
>  	/* make sure the string is \0 terminated */
>  	buf[sizeof(buf) - 1] = '\0';

Same here, makes sense.

> @@ -84,17 +85,17 @@ static char
>  	return strdup(buf);
>  }
>
> -static unsigned int
> +static bool
>  test_open(char *filename)
>  {
>  	FILE *f;
>
>  	f = fopen(filename, "r");
>  	if (!f)
> -		return 0;
> +		return false;
>
>  	fclose(f);
> -	return 1;
> +	return true;
>  }
>
>  static int ubi_volume_init(struct volume *v)

I don't see a lot of merit here. Since this function is static, I have  
to see the first compiler that doesn't optimize this away. I would be  
surprised if this would produce different binaries.

> diff --git a/libubi/libubi.c b/libubi/libubi.c
> index 3328ac8..3082a10 100644
> --- a/libubi/libubi.c
> +++ b/libubi/libubi.c
> @@ -427,6 +427,7 @@ static int vol_node2nums(struct libubi *lib,  
> const char *node, int *dev_num,
>  		return -1;
>  	}
>
> +	close(fd);
>  	*dev_num = i;
>  	*vol_id = minor - 1;
>  	errno = 0;

Makes sense.

> @@ -1021,9 +1022,8 @@ int ubi_mkvol(libubi_t desc, const char *node,  
> struct ubi_mkvol_request *req)
>  	struct ubi_mkvol_req r;
>  	size_t n;
>
> -	memset(&r, 0, sizeof(struct ubi_mkvol_req));
> -
>  	desc = desc;
> +	memset(&r, 0, sizeof(struct ubi_mkvol_req));
>  	r.vol_id = req->vol_id;
>  	r.alignment = req->alignment;
>  	r.bytes = req->bytes;

What's the rationale here?

> diff --git a/libubi/ubiutils-common.c b/libubi/ubiutils-common.c
> index 2271927..8ea2815 100644
> --- a/libubi/ubiutils-common.c
> +++ b/libubi/ubiutils-common.c
> @@ -119,7 +119,7 @@ void ubiutils_print_bytes(long long bytes, int bracket)
>  		printf("%s%.1f GiB", p, (double)bytes / (1024 * 1024 * 1024));
>  	else if (bytes > 1024 * 1024)
>  		printf("%s%.1f MiB", p, (double)bytes / (1024 * 1024));
> -	else if (bytes > 1024 && bytes != 0)
> +	else if (bytes > 1024)
>  		printf("%s%.1f KiB", p, (double)bytes / 1024);
>  	else
>  		return;

Nice catch.
Rosen Penev Dec. 10, 2017, 8:59 p.m. UTC | #2 
Reposting since gmail sucks:

On Sun, Dec 10, 2017 at 10:17 AM, Arjen de Korte
<arjen+lede@de-korte.org> wrote:
> Citeren Rosen Penev <rosenp@gmail.com>:
>
>> Mainly plugging memory leaks. Size reduction as well. The calloc change
>> accounts for 272 bytes on this machine for some reason...
>
>
> Comments inline.
>
>> Signed-off-by: Rosen Penev <rosenp@gmail.com>
>> ---
>>  block.c                  |  6 +++---
>>  blockd.c                 |  3 +++
>>  libfstools/overlay.c     |  2 +-
>>  libfstools/rootdisk.c    |  7 ++++---
>>  libfstools/snapshot.c    | 11 +++++++----
>>  libfstools/ubi.c         | 13 +++++++------
>>  libubi/libubi.c          |  4 ++--
>>  libubi/ubiutils-common.c |  2 +-
>>  8 files changed, 28 insertions(+), 20 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index ab130b9..6117701 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -316,8 +316,7 @@ static int swap_add(struct uci_section *s)
>>         if (!tb[SWAP_UUID] && !tb[SWAP_LABEL] && !tb[SWAP_DEVICE])
>>                 return -1;
>>
>> -       m = malloc(sizeof(struct mount));
>> -       memset(m, 0, sizeof(struct mount));
>> +       m = calloc(1, sizeof(struct mount));
>>         m->type = TYPE_SWAP;
>>         m->uuid = blobmsg_get_strdup(tb[SWAP_UUID]);
>>         m->label = blobmsg_get_strdup(tb[SWAP_LABEL]);
>
>
> This fails to address the real issue here, there is no check for a NULL
> pointer after allocating memory.
>
will add a check
>> @@ -1084,7 +1083,7 @@ static int mount_device(struct probe_info *pr, int
>> type)
>>  static int umount_device(struct probe_info *pr)
>>  {
>>         struct mount *m;
>> -       char *device = basename(pr->dev);
>> +       char *device;
>>         char *mp;
>>         int err;
>>
>> @@ -1098,6 +1097,7 @@ static int umount_device(struct probe_info *pr)
>>         if (!mp)
>>                 return -1;
>>
>> +       device = basename(pr->dev);
>>         m = find_block(pr->uuid, pr->label, device, NULL);
>>         if (m && m->extroot)
>>                 return -1;
>
>
> What exactly is the above change supposed to fix?
>
null pointer dereference. The check for pr being null is after the declarations.
>> diff --git a/blockd.c b/blockd.c
>> index 3af5390..f7a4dec 100644
>> --- a/blockd.c
>> +++ b/blockd.c
>> @@ -115,6 +115,9 @@ device_free(struct device *device)
>>         char *target = NULL;
>>         char *path = NULL, _path[64], *mp;
>>
>> +       if (!device)
>> +               return;
>> +
>>         blobmsg_parse(mount_policy, __MOUNT_MAX, data,
>>                       blob_data(device->msg), blob_len(device->msg));
>>
>
> I would rather fix this in the devices_update_cb() function instead. Only in
> line 212 there is a change the device_free() function is called will a NULL
> pointer, but a couple of lines down, there is already a check for a NULL
> pointer. One might fix this in one go by using
>
>         if (device_o) {
>                 device_free(device_o);
>                 free(device_o);
>         }
>
cppcheck no longer complains after this change and removing the
previous device_free

>> diff --git a/libfstools/overlay.c b/libfstools/overlay.c
>> index 7ada5ff..d7a55e6 100644
>> --- a/libfstools/overlay.c
>> +++ b/libfstools/overlay.c
>> @@ -284,7 +284,7 @@ enum fs_state fs_state_get(const char *dir)
>>  {
>>         char *path;
>>         char valstr[16];
>> -       uint32_t val;
>> +       int val;
>>         ssize_t len;
>>
>>         path = alloca(strlen(dir) + 1 + sizeof("/.fs_state"));
>
>
> This just silences a warning, without fixing the (potential) underlying
> problem. If the result of the atoi() somehow is not a member of the enum
> fs_state (which is guaranteed > 0 numerically), the function will return a
> non-member either way.
>
>
this change just lowers compiled size by 16 bytes. not sure why.
the goal was to close as soon as possible to avoid adding an extra
fclose call. Saves a few bytes.
this is optimized away. I just saw it as a way to make the code a
little clearer. I can revert if needed.
>> diff --git a/libubi/libubi.c b/libubi/libubi.c
>> index 3328ac8..3082a10 100644
>> --- a/libubi/libubi.c
>> +++ b/libubi/libubi.c
>> @@ -427,6 +427,7 @@ static int vol_node2nums(struct libubi *lib, const
>> char *node, int *dev_num,
>>                 return -1;
>>         }
>>
>> +       close(fd);
>>         *dev_num = i;
>>         *vol_id = minor - 1;
>>         errno = 0;
>
>
> Makes sense.
>
>> @@ -1021,9 +1022,8 @@ int ubi_mkvol(libubi_t desc, const char *node,
>> struct ubi_mkvol_request *req)
>>         struct ubi_mkvol_req r;
>>         size_t n;
>>
>> -       memset(&r, 0, sizeof(struct ubi_mkvol_req));
>> -
>>         desc = desc;
>> +       memset(&r, 0, sizeof(struct ubi_mkvol_req));
>>         r.vol_id = req->vol_id;
>>         r.alignment = req->alignment;
>>         r.bytes = req->bytes;
>
>
> What's the rationale here?
>
consistency update with the other memset. no functional difference.

On Sun, Dec 10, 2017 at 11:00 AM, Rosen Penev <rosenp@gmail.com> wrote:
> On Sun, Dec 10, 2017 at 10:17 AM, Arjen de Korte
> <arjen+lede@de-korte.org> wrote:
>> Citeren Rosen Penev <rosenp@gmail.com>:
>>
>>> Mainly plugging memory leaks. Size reduction as well. The calloc change
>>> accounts for 272 bytes on this machine for some reason...
>>
>>
>> Comments inline.
>>
>>> Signed-off-by: Rosen Penev <rosenp@gmail.com>
>>> ---
>>>  block.c                  |  6 +++---
>>>  blockd.c                 |  3 +++
>>>  libfstools/overlay.c     |  2 +-
>>>  libfstools/rootdisk.c    |  7 ++++---
>>>  libfstools/snapshot.c    | 11 +++++++----
>>>  libfstools/ubi.c         | 13 +++++++------
>>>  libubi/libubi.c          |  4 ++--
>>>  libubi/ubiutils-common.c |  2 +-
>>>  8 files changed, 28 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index ab130b9..6117701 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -316,8 +316,7 @@ static int swap_add(struct uci_section *s)
>>>         if (!tb[SWAP_UUID] && !tb[SWAP_LABEL] && !tb[SWAP_DEVICE])
>>>                 return -1;
>>>
>>> -       m = malloc(sizeof(struct mount));
>>> -       memset(m, 0, sizeof(struct mount));
>>> +       m = calloc(1, sizeof(struct mount));
>>>         m->type = TYPE_SWAP;
>>>         m->uuid = blobmsg_get_strdup(tb[SWAP_UUID]);
>>>         m->label = blobmsg_get_strdup(tb[SWAP_LABEL]);
>>
>>
>> This fails to address the real issue here, there is no check for a NULL
>> pointer after allocating memory.
>>
> will add a check
>>> @@ -1084,7 +1083,7 @@ static int mount_device(struct probe_info *pr, int
>>> type)
>>>  static int umount_device(struct probe_info *pr)
>>>  {
>>>         struct mount *m;
>>> -       char *device = basename(pr->dev);
>>> +       char *device;
>>>         char *mp;
>>>         int err;
>>>
>>> @@ -1098,6 +1097,7 @@ static int umount_device(struct probe_info *pr)
>>>         if (!mp)
>>>                 return -1;
>>>
>>> +       device = basename(pr->dev);
>>>         m = find_block(pr->uuid, pr->label, device, NULL);
>>>         if (m && m->extroot)
>>>                 return -1;
>>
>>
>> What exactly is the above change supposed to fix?
>>
> null pointer dereference. The check for pr being null is after the declarations.
>>> diff --git a/blockd.c b/blockd.c
>>> index 3af5390..f7a4dec 100644
>>> --- a/blockd.c
>>> +++ b/blockd.c
>>> @@ -115,6 +115,9 @@ device_free(struct device *device)
>>>         char *target = NULL;
>>>         char *path = NULL, _path[64], *mp;
>>>
>>> +       if (!device)
>>> +               return;
>>> +
>>>         blobmsg_parse(mount_policy, __MOUNT_MAX, data,
>>>                       blob_data(device->msg), blob_len(device->msg));
>>>
>>
>> I would rather fix this in the devices_update_cb() function instead. Only in
>> line 212 there is a change the device_free() function is called will a NULL
>> pointer, but a couple of lines down, there is already a check for a NULL
>> pointer. One might fix this in one go by using
>>
>>         if (device_o) {
>>                 device_free(device_o);
>>                 free(device_o);
>>         }
>>
> cppcheck no longer complains after this change and removing the
> previous device_free
>
>>> diff --git a/libfstools/overlay.c b/libfstools/overlay.c
>>> index 7ada5ff..d7a55e6 100644
>>> --- a/libfstools/overlay.c
>>> +++ b/libfstools/overlay.c
>>> @@ -284,7 +284,7 @@ enum fs_state fs_state_get(const char *dir)
>>>  {
>>>         char *path;
>>>         char valstr[16];
>>> -       uint32_t val;
>>> +       int val;
>>>         ssize_t len;
>>>
>>>         path = alloca(strlen(dir) + 1 + sizeof("/.fs_state"));
>>
>>
>> This just silences a warning, without fixing the (potential) underlying
>> problem. If the result of the atoi() somehow is not a member of the enum
>> fs_state (which is guaranteed > 0 numerically), the function will return a
>> non-member either way.
>>
>>
> this change just lowers compiled size by 16 bytes. not sure why.
>>> diff --git a/libfstools/rootdisk.c b/libfstools/rootdisk.c
>>> index dd00c1b..2bb16d4 100644
>>> --- a/libfstools/rootdisk.c
>>> +++ b/libfstools/rootdisk.c
>>> @@ -171,8 +171,10 @@ static int rootdisk_volume_identify(struct volume *v)
>>>
>>>         fseeko(f, p->offset + 0x400, SEEK_SET);
>>>         n = fread(&magic, sizeof(magic), 1, f);
>>> -       if (n != 1)
>>> +       if (n != 1) {
>>> +               fclose(f);
>>>                 return -1;
>>> +       }
>>>
>>>         if (magic == cpu_to_le32(0xF2F52010))
>>>                 ret = FS_F2FS;
>>> @@ -180,13 +182,12 @@ static int rootdisk_volume_identify(struct volume
>>> *v)
>>>         magic = 0;
>>>         fseeko(f, p->offset + 0x438, SEEK_SET);
>>>         n = fread(&magic, sizeof(magic), 1, f);
>>> +       fclose(f);
>>>         if (n != 1)
>>>                 return -1;
>>>         if ((le32_to_cpu(magic) & 0xffff) == 0xef53)
>>>                 ret = FS_EXT4;
>>>
>>> -       fclose(f);
>>> -
>>>         return ret;
>>>  }
>>>
>>
>> This actually makes sense to prevent leaking file pointers.
>>
> the goal was to close as soon as possible to avoid adding an extra
> fclose call. Saves a few bytes.
>>
>>> diff --git a/libfstools/snapshot.c b/libfstools/snapshot.c
>>> index 4870cf7..58bed96 100644
>>> --- a/libfstools/snapshot.c
>>> +++ b/libfstools/snapshot.c
>>> @@ -203,16 +203,19 @@ snapshot_read_file(struct volume *v, int block, char
>>> *file, uint32_t type)
>>>                 if (hdr.length < len)
>>>                         len = hdr.length;
>>>
>>> -               if (volume_read(v, buffer, offset, len))
>>> +               if (volume_read(v, buffer, offset, len)) {
>>> +                       close(out);
>>>                         return -1;
>>> -               if (write(out, buffer, len) != len)
>>> +               }
>>> +
>>> +               int w = write(out, buffer, len);
>>> +               close(out);
>>> +               if (w != len)
>>>                         return -1;
>>>                 offset += len;
>>>                 hdr.length -= len;
>>>         }
>>>
>>> -       close(out);
>>> -
>>>         if (verify_file_hash(file, hdr.md5)) {
>>>                 ULOG_ERR("md5 verification failed\n");
>>>                 unlink(file);
>>
>>
>> Same here, this makes sense to do.
>>
>>> diff --git a/libfstools/ubi.c b/libfstools/ubi.c
>>> index f9d6e0a..678b8bf 100644
>>> --- a/libfstools/ubi.c
>>> +++ b/libfstools/ubi.c
>>> @@ -60,6 +60,7 @@ static char
>>>         FILE *f;
>>>         char fname[BUFLEN];
>>>         char buf[BUFLEN];
>>> +       char *s;
>>>         int i;
>>>
>>>         snprintf(fname, sizeof(fname), "%s/%s", dirname, filename);
>>> @@ -68,10 +69,10 @@ static char
>>>         if (!f)
>>>                 return NULL;
>>>
>>> -       if (fgets(buf, sizeof(buf), f) == NULL)
>>> -               return NULL;
>>> -
>>> +       s = fgets(buf, sizeof(buf), f);
>>>         fclose(f);
>>> +       if (s == NULL)
>>> +               return NULL;
>>>
>>>         /* make sure the string is \0 terminated */
>>>         buf[sizeof(buf) - 1] = '\0';
>>
>>
>> Same here, makes sense.
>>
>>> @@ -84,17 +85,17 @@ static char
>>>         return strdup(buf);
>>>  }
>>>
>>> -static unsigned int
>>> +static bool
>>>  test_open(char *filename)
>>>  {
>>>         FILE *f;
>>>
>>>         f = fopen(filename, "r");
>>>         if (!f)
>>> -               return 0;
>>> +               return false;
>>>
>>>         fclose(f);
>>> -       return 1;
>>> +       return true;
>>>  }
>>>
>>>  static int ubi_volume_init(struct volume *v)
>>
>>
>> I don't see a lot of merit here. Since this function is static, I have to
>> see the first compiler that doesn't optimize this away. I would be surprised
>> if this would produce different binaries.
>>
> this is optimized away. I just saw it as a way to make the code a
> little clearer. I can revert if needed.
>>> diff --git a/libubi/libubi.c b/libubi/libubi.c
>>> index 3328ac8..3082a10 100644
>>> --- a/libubi/libubi.c
>>> +++ b/libubi/libubi.c
>>> @@ -427,6 +427,7 @@ static int vol_node2nums(struct libubi *lib, const
>>> char *node, int *dev_num,
>>>                 return -1;
>>>         }
>>>
>>> +       close(fd);
>>>         *dev_num = i;
>>>         *vol_id = minor - 1;
>>>         errno = 0;
>>
>>
>> Makes sense.
>>
>>> @@ -1021,9 +1022,8 @@ int ubi_mkvol(libubi_t desc, const char *node,
>>> struct ubi_mkvol_request *req)
>>>         struct ubi_mkvol_req r;
>>>         size_t n;
>>>
>>> -       memset(&r, 0, sizeof(struct ubi_mkvol_req));
>>> -
>>>         desc = desc;
>>> +       memset(&r, 0, sizeof(struct ubi_mkvol_req));
>>>         r.vol_id = req->vol_id;
>>>         r.alignment = req->alignment;
>>>         r.bytes = req->bytes;
>>
>>
>> What's the rationale here?
>>
> consistency update with the other memset. no functional difference.
>>> diff --git a/libubi/ubiutils-common.c b/libubi/ubiutils-common.c
>>> index 2271927..8ea2815 100644
>>> --- a/libubi/ubiutils-common.c
>>> +++ b/libubi/ubiutils-common.c
>>> @@ -119,7 +119,7 @@ void ubiutils_print_bytes(long long bytes, int
>>> bracket)
>>>                 printf("%s%.1f GiB", p, (double)bytes / (1024 * 1024 *
>>> 1024));
>>>         else if (bytes > 1024 * 1024)
>>>                 printf("%s%.1f MiB", p, (double)bytes / (1024 * 1024));
>>> -       else if (bytes > 1024 && bytes != 0)
>>> +       else if (bytes > 1024)
>>>                 printf("%s%.1f KiB", p, (double)bytes / 1024);
>>>         else
>>>                 return;
>>
>>
>> Nice catch.
>>
>>
>> _______________________________________________
>> Lede-dev mailing list
>> Lede-dev@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/lede-dev
diff mbox series

Patch

diff --git a/block.c b/block.c
index ab130b9..6117701 100644
--- a/block.c
+++ b/block.c
@@ -316,8 +316,7 @@  static int swap_add(struct uci_section *s)
 	if (!tb[SWAP_UUID] && !tb[SWAP_LABEL] && !tb[SWAP_DEVICE])
 		return -1;
 
-	m = malloc(sizeof(struct mount));
-	memset(m, 0, sizeof(struct mount));
+	m = calloc(1, sizeof(struct mount));
 	m->type = TYPE_SWAP;
 	m->uuid = blobmsg_get_strdup(tb[SWAP_UUID]);
 	m->label = blobmsg_get_strdup(tb[SWAP_LABEL]);
@@ -1084,7 +1083,7 @@  static int mount_device(struct probe_info *pr, int type)
 static int umount_device(struct probe_info *pr)
 {
 	struct mount *m;
-	char *device = basename(pr->dev);
+	char *device;
 	char *mp;
 	int err;
 
@@ -1098,6 +1097,7 @@  static int umount_device(struct probe_info *pr)
 	if (!mp)
 		return -1;
 
+	device = basename(pr->dev);
 	m = find_block(pr->uuid, pr->label, device, NULL);
 	if (m && m->extroot)
 		return -1;
diff --git a/blockd.c b/blockd.c
index 3af5390..f7a4dec 100644
--- a/blockd.c
+++ b/blockd.c
@@ -115,6 +115,9 @@  device_free(struct device *device)
 	char *target = NULL;
 	char *path = NULL, _path[64], *mp;
 
+	if (!device)
+		return;
+
 	blobmsg_parse(mount_policy, __MOUNT_MAX, data,
 		      blob_data(device->msg), blob_len(device->msg));
 
diff --git a/libfstools/overlay.c b/libfstools/overlay.c
index 7ada5ff..d7a55e6 100644
--- a/libfstools/overlay.c
+++ b/libfstools/overlay.c
@@ -284,7 +284,7 @@  enum fs_state fs_state_get(const char *dir)
 {
 	char *path;
 	char valstr[16];
-	uint32_t val;
+	int val;
 	ssize_t len;
 
 	path = alloca(strlen(dir) + 1 + sizeof("/.fs_state"));
diff --git a/libfstools/rootdisk.c b/libfstools/rootdisk.c
index dd00c1b..2bb16d4 100644
--- a/libfstools/rootdisk.c
+++ b/libfstools/rootdisk.c
@@ -171,8 +171,10 @@  static int rootdisk_volume_identify(struct volume *v)
 
 	fseeko(f, p->offset + 0x400, SEEK_SET);
 	n = fread(&magic, sizeof(magic), 1, f);
-	if (n != 1)
+	if (n != 1) {
+		fclose(f);
 		return -1;
+	}
 
 	if (magic == cpu_to_le32(0xF2F52010))
 		ret = FS_F2FS;
@@ -180,13 +182,12 @@  static int rootdisk_volume_identify(struct volume *v)
 	magic = 0;
 	fseeko(f, p->offset + 0x438, SEEK_SET);
 	n = fread(&magic, sizeof(magic), 1, f);
+	fclose(f);
 	if (n != 1)
 		return -1;
 	if ((le32_to_cpu(magic) & 0xffff) == 0xef53)
 		ret = FS_EXT4;
 
-	fclose(f);
-
 	return ret;
 }
 
diff --git a/libfstools/snapshot.c b/libfstools/snapshot.c
index 4870cf7..58bed96 100644
--- a/libfstools/snapshot.c
+++ b/libfstools/snapshot.c
@@ -203,16 +203,19 @@  snapshot_read_file(struct volume *v, int block, char *file, uint32_t type)
 		if (hdr.length < len)
 			len = hdr.length;
 
-		if (volume_read(v, buffer, offset, len))
+		if (volume_read(v, buffer, offset, len)) {
+			close(out);
 			return -1;
-		if (write(out, buffer, len) != len)
+		}
+
+		int w = write(out, buffer, len);
+		close(out);
+		if (w != len)
 			return -1;
 		offset += len;
 		hdr.length -= len;
 	}
 
-	close(out);
-
 	if (verify_file_hash(file, hdr.md5)) {
 		ULOG_ERR("md5 verification failed\n");
 		unlink(file);
diff --git a/libfstools/ubi.c b/libfstools/ubi.c
index f9d6e0a..678b8bf 100644
--- a/libfstools/ubi.c
+++ b/libfstools/ubi.c
@@ -60,6 +60,7 @@  static char
 	FILE *f;
 	char fname[BUFLEN];
 	char buf[BUFLEN];
+	char *s;
 	int i;
 
 	snprintf(fname, sizeof(fname), "%s/%s", dirname, filename);
@@ -68,10 +69,10 @@  static char
 	if (!f)
 		return NULL;
 
-	if (fgets(buf, sizeof(buf), f) == NULL)
-		return NULL;
-
+	s = fgets(buf, sizeof(buf), f);
 	fclose(f);
+	if (s == NULL)
+		return NULL;
 
 	/* make sure the string is \0 terminated */
 	buf[sizeof(buf) - 1] = '\0';
@@ -84,17 +85,17 @@  static char
 	return strdup(buf);
 }
 
-static unsigned int
+static bool
 test_open(char *filename)
 {
 	FILE *f;
 
 	f = fopen(filename, "r");
 	if (!f)
-		return 0;
+		return false;
 
 	fclose(f);
-	return 1;
+	return true;
 }
 
 static int ubi_volume_init(struct volume *v)
diff --git a/libubi/libubi.c b/libubi/libubi.c
index 3328ac8..3082a10 100644
--- a/libubi/libubi.c
+++ b/libubi/libubi.c
@@ -427,6 +427,7 @@  static int vol_node2nums(struct libubi *lib, const char *node, int *dev_num,
 		return -1;
 	}
 
+	close(fd);
 	*dev_num = i;
 	*vol_id = minor - 1;
 	errno = 0;
@@ -1021,9 +1022,8 @@  int ubi_mkvol(libubi_t desc, const char *node, struct ubi_mkvol_request *req)
 	struct ubi_mkvol_req r;
 	size_t n;
 
-	memset(&r, 0, sizeof(struct ubi_mkvol_req));
-
 	desc = desc;
+	memset(&r, 0, sizeof(struct ubi_mkvol_req));
 	r.vol_id = req->vol_id;
 	r.alignment = req->alignment;
 	r.bytes = req->bytes;
diff --git a/libubi/ubiutils-common.c b/libubi/ubiutils-common.c
index 2271927..8ea2815 100644
--- a/libubi/ubiutils-common.c
+++ b/libubi/ubiutils-common.c
@@ -119,7 +119,7 @@  void ubiutils_print_bytes(long long bytes, int bracket)
 		printf("%s%.1f GiB", p, (double)bytes / (1024 * 1024 * 1024));
 	else if (bytes > 1024 * 1024)
 		printf("%s%.1f MiB", p, (double)bytes / (1024 * 1024));
-	else if (bytes > 1024 && bytes != 0)
+	else if (bytes > 1024)
 		printf("%s%.1f KiB", p, (double)bytes / 1024);
 	else
 		return;