[PATCH 0/2] Fix build issues

[PATCH 0/2] Fix build issues

[Guoqing Jiang]

Hi Jes,

The first patch resolve the build issue of 'make raid6check', and the second
one is for the issue which appeared during 'make everything'. Maybe it is
better to resolve the issues before release 4.2.

BTW, compile test only.

Thanks,
Guoqing

Guoqing Jiang (2):
  uuid: split uuid.c from util.c
  restripe: fix ignoring return value of ‘read’

 Makefile   |  6 ++--
 restripe.c |  6 +++-
 util.c     | 87 ---------------------------------------------------
 uuid.c     | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 100 insertions(+), 91 deletions(-)
 create mode 100644 uuid.c

-- 
2.17.1

[PATCH 1/2] uuid.c: split uuid stuffs from util.c

[Guoqing Jiang]

Currently, 'make raid6check' is build broken since commit b06815989
("mdadm: load default sysfs attributes after assemblation").

/usr/bin/ld: sysfs.o: in function `sysfsline':
sysfs.c:(.text+0x2707): undefined reference to `parse_uuid'
/usr/bin/ld: sysfs.c:(.text+0x271a): undefined reference to `uuid_zero'
/usr/bin/ld: sysfs.c:(.text+0x2721): undefined reference to `uuid_zero'

Apparently, the compile of mdadm or raid6check are coupled with uuid
functions inside util.c. However, we can't just add util.o to CHECK_OBJS
which raid6check is needed, because it caused other worse problems.

So, let's introduce a uuid.c file which is indenpended file to fix the
problem, all the contents are splitted from util.c.

Cc: Piergiorgio Sartor <piergiorgio.sartor@nexgo.de>
Cc: Wolfgang Denk <wd@denx.de>
Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 Makefile |  6 ++--
 util.c   | 87 -----------------------------------------------------
 uuid.c   | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+), 90 deletions(-)
 create mode 100644 uuid.c

diff --git a/Makefile b/Makefile
index a33319a..d13030c 100644
--- a/Makefile
+++ b/Makefile
@@ -139,7 +139,7 @@ else
 	ECHO=:
 endif
 
-OBJS =  mdadm.o config.o policy.o mdstat.o  ReadMe.o util.o maps.o lib.o \
+OBJS =  mdadm.o config.o policy.o mdstat.o  ReadMe.o uuid.o util.o maps.o lib.o \
 	Manage.o Assemble.o Build.o \
 	Create.o Detail.o Examine.o Grow.o Monitor.o dlink.o Kill.o Query.o \
 	Incremental.o Dump.o \
@@ -148,13 +148,13 @@ OBJS =  mdadm.o config.o policy.o mdstat.o  ReadMe.o util.o maps.o lib.o \
 	restripe.o sysfs.o sha1.o mapfile.o crc32.o sg_io.o msg.o xmalloc.o \
 	platform-intel.o probe_roms.o crc32c.o
 
-CHECK_OBJS = restripe.o sysfs.o maps.o lib.o xmalloc.o dlink.o
+CHECK_OBJS = restripe.o uuid.o sysfs.o maps.o lib.o xmalloc.o dlink.o
 
 SRCS =  $(patsubst %.o,%.c,$(OBJS))
 
 INCL = mdadm.h part.h bitmap.h
 
-MON_OBJS = mdmon.o monitor.o managemon.o util.o maps.o mdstat.o sysfs.o \
+MON_OBJS = mdmon.o monitor.o managemon.o uuid.o util.o maps.o mdstat.o sysfs.o \
 	policy.o lib.o \
 	Kill.o sg_io.o dlink.o ReadMe.o super-intel.o \
 	super-mbr.o super-gpt.o \
diff --git a/util.c b/util.c
index 07f9dc3..579dd42 100644
--- a/util.c
+++ b/util.c
@@ -306,43 +306,6 @@ int md_get_disk_info(int fd, struct mdu_disk_info_s *disk)
 	return ioctl(fd, GET_DISK_INFO, disk);
 }
 
-/*
- * Parse a 128 bit uuid in 4 integers
- * format is 32 hexx nibbles with options :.<space> separator
- * If not exactly 32 hex digits are found, return 0
- * else return 1
- */
-int parse_uuid(char *str, int uuid[4])
-{
-	int hit = 0; /* number of Hex digIT */
-	int i;
-	char c;
-	for (i = 0; i < 4; i++)
-		uuid[i] = 0;
-
-	while ((c = *str++) != 0) {
-		int n;
-		if (c >= '0' && c <= '9')
-			n = c-'0';
-		else if (c >= 'a' && c <= 'f')
-			n = 10 + c - 'a';
-		else if (c >= 'A' && c <= 'F')
-			n = 10 + c - 'A';
-		else if (strchr(":. -", c))
-			continue;
-		else return 0;
-
-		if (hit<32) {
-			uuid[hit/8] <<= 4;
-			uuid[hit/8] += n;
-		}
-		hit++;
-	}
-	if (hit == 32)
-		return 1;
-	return 0;
-}
-
 int get_linux_version()
 {
 	struct utsname name;
@@ -611,56 +574,6 @@ int enough(int level, int raid_disks, int layout, int clean, char *avail)
 	}
 }
 
-const int uuid_zero[4] = { 0, 0, 0, 0 };
-
-int same_uuid(int a[4], int b[4], int swapuuid)
-{
-	if (swapuuid) {
-		/* parse uuids are hostendian.
-		 * uuid's from some superblocks are big-ending
-		 * if there is a difference, we need to swap..
-		 */
-		unsigned char *ac = (unsigned char *)a;
-		unsigned char *bc = (unsigned char *)b;
-		int i;
-		for (i = 0; i < 16; i += 4) {
-			if (ac[i+0] != bc[i+3] ||
-			    ac[i+1] != bc[i+2] ||
-			    ac[i+2] != bc[i+1] ||
-			    ac[i+3] != bc[i+0])
-				return 0;
-		}
-		return 1;
-	} else {
-		if (a[0]==b[0] &&
-		    a[1]==b[1] &&
-		    a[2]==b[2] &&
-		    a[3]==b[3])
-			return 1;
-		return 0;
-	}
-}
-
-void copy_uuid(void *a, int b[4], int swapuuid)
-{
-	if (swapuuid) {
-		/* parse uuids are hostendian.
-		 * uuid's from some superblocks are big-ending
-		 * if there is a difference, we need to swap..
-		 */
-		unsigned char *ac = (unsigned char *)a;
-		unsigned char *bc = (unsigned char *)b;
-		int i;
-		for (i = 0; i < 16; i += 4) {
-			ac[i+0] = bc[i+3];
-			ac[i+1] = bc[i+2];
-			ac[i+2] = bc[i+1];
-			ac[i+3] = bc[i+0];
-		}
-	} else
-		memcpy(a, b, 16);
-}
-
 char *__fname_from_uuid(int id[4], int swap, char *buf, char sep)
 {
 	int i, j;
diff --git a/uuid.c b/uuid.c
new file mode 100644
index 0000000..5b29e16
--- /dev/null
+++ b/uuid.c
@@ -0,0 +1,92 @@
+/*
+ * Splited from util.c, so uuid.c shares the same copyright of it,
+ */
+
+#include	<string.h>
+
+const int uuid_zero[4] = { 0, 0, 0, 0 };
+
+int same_uuid(int a[4], int b[4], int swapuuid)
+{
+	if (swapuuid) {
+		/* parse uuids are hostendian.
+		 * uuid's from some superblocks are big-ending
+		 * if there is a difference, we need to swap..
+		 */
+		unsigned char *ac = (unsigned char *)a;
+		unsigned char *bc = (unsigned char *)b;
+		int i;
+		for (i = 0; i < 16; i += 4) {
+			if (ac[i+0] != bc[i+3] ||
+			    ac[i+1] != bc[i+2] ||
+			    ac[i+2] != bc[i+1] ||
+			    ac[i+3] != bc[i+0])
+				return 0;
+		}
+		return 1;
+	} else {
+		if (a[0]==b[0] &&
+		    a[1]==b[1] &&
+		    a[2]==b[2] &&
+		    a[3]==b[3])
+			return 1;
+		return 0;
+	}
+}
+
+void copy_uuid(void *a, int b[4], int swapuuid)
+{
+	if (swapuuid) {
+		/* parse uuids are hostendian.
+		 * uuid's from some superblocks are big-ending
+		 * if there is a difference, we need to swap..
+		 */
+		unsigned char *ac = (unsigned char *)a;
+		unsigned char *bc = (unsigned char *)b;
+		int i;
+		for (i = 0; i < 16; i += 4) {
+			ac[i+0] = bc[i+3];
+			ac[i+1] = bc[i+2];
+			ac[i+2] = bc[i+1];
+			ac[i+3] = bc[i+0];
+		}
+	} else
+		memcpy(a, b, 16);
+}
+
+/*
+ * Parse a 128 bit uuid in 4 integers
+ * format is 32 hexx nibbles with options :.<space> separator
+ * If not exactly 32 hex digits are found, return 0
+ * else return 1
+ */
+int parse_uuid(char *str, int uuid[4])
+{
+	int hit = 0; /* number of Hex digIT */
+	int i;
+	char c;
+	for (i = 0; i < 4; i++)
+		uuid[i] = 0;
+
+	while ((c = *str++) != 0) {
+		int n;
+		if (c >= '0' && c <= '9')
+			n = c-'0';
+		else if (c >= 'a' && c <= 'f')
+			n = 10 + c - 'a';
+		else if (c >= 'A' && c <= 'F')
+			n = 10 + c - 'A';
+		else if (strchr(":. -", c))
+			continue;
+		else return 0;
+
+		if (hit<32) {
+			uuid[hit/8] <<= 4;
+			uuid[hit/8] += n;
+		}
+		hit++;
+	}
+	if (hit == 32)
+		return 1;
+	return 0;
+}
-- 
2.17.1

[PATCH 2/2] restripe: fix ignoring return value of ‘read’

[Guoqing Jiang]

Got below error when run "make everything".

restripe.c: In function ‘test_stripes’:
restripe.c:870:4: error: ignoring return value of ‘read’, declared with attribute warn_unused_result [-Werror=unused-result]
    read(source[i], stripes[i], chunk_size);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Fix it by set the return value of ‘read’ to diskP, which should be
harmless since diskP will be set again before it is used.

Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 restripe.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/restripe.c b/restripe.c
index 31b07e8..21c90f5 100644
--- a/restripe.c
+++ b/restripe.c
@@ -867,7 +867,11 @@ int test_stripes(int *source, unsigned long long *offsets,
 
 		for (i = 0 ; i < raid_disks ; i++) {
 			lseek64(source[i], offsets[i]+start, 0);
-			read(source[i], stripes[i], chunk_size);
+			/*
+			 * To resolve "ignoring return value of ‘read’", it
+			 * should be harmless since diskP will be again later.
+			 */
+			diskP = read(source[i], stripes[i], chunk_size);
 		}
 		for (i = 0 ; i < data_disks ; i++) {
 			int disk = geo_map(i, start/chunk_size, raid_disks,
-- 
2.17.1

Re: [PATCH 1/2] uuid.c: split uuid stuffs from util.c

[Jes Sorensen]

On 5/15/20 9:40 AM, Guoqing Jiang wrote:
> Currently, 'make raid6check' is build broken since commit b06815989
> ("mdadm: load default sysfs attributes after assemblation").
> 
> /usr/bin/ld: sysfs.o: in function `sysfsline':
> sysfs.c:(.text+0x2707): undefined reference to `parse_uuid'
> /usr/bin/ld: sysfs.c:(.text+0x271a): undefined reference to `uuid_zero'
> /usr/bin/ld: sysfs.c:(.text+0x2721): undefined reference to `uuid_zero'
> 
> Apparently, the compile of mdadm or raid6check are coupled with uuid
> functions inside util.c. However, we can't just add util.o to CHECK_OBJS
> which raid6check is needed, because it caused other worse problems.
> 
> So, let's introduce a uuid.c file which is indenpended file to fix the
> problem, all the contents are splitted from util.c.
> 
> Cc: Piergiorgio Sartor <piergiorgio.sartor@nexgo.de>
> Cc: Wolfgang Denk <wd@denx.de>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
> ---
>  Makefile |  6 ++--
>  util.c   | 87 -----------------------------------------------------
>  uuid.c   | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 95 insertions(+), 90 deletions(-)
>  create mode 100644 uuid.c

I am fine with this change, but uuid.c needs to respect the license
header that was in util.c

Jes

Re: [PATCH 2/2] restripe: fix ignoring return value of ‘read’

[Jes Sorensen]

On 5/15/20 9:40 AM, Guoqing Jiang wrote:
> Got below error when run "make everything".
> 
> restripe.c: In function ‘test_stripes’:
> restripe.c:870:4: error: ignoring return value of ‘read’, declared with attribute warn_unused_result [-Werror=unused-result]
>     read(source[i], stripes[i], chunk_size);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Fix it by set the return value of ‘read’ to diskP, which should be
> harmless since diskP will be set again before it is used.
> 
> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
> ---
>  restripe.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/restripe.c b/restripe.c
> index 31b07e8..21c90f5 100644
> --- a/restripe.c
> +++ b/restripe.c
> @@ -867,7 +867,11 @@ int test_stripes(int *source, unsigned long long *offsets,
>  
>  		for (i = 0 ; i < raid_disks ; i++) {
>  			lseek64(source[i], offsets[i]+start, 0);
> -			read(source[i], stripes[i], chunk_size);
> +			/*
> +			 * To resolve "ignoring return value of ‘read’", it
> +			 * should be harmless since diskP will be again later.
> +			 */
> +			diskP = read(source[i], stripes[i], chunk_size);

It doesn't complain on Fedora 32, however checking the return value of
lseek64 and read is a good thing.

However what you have done is to just masking the return value and
throwing it away, is not OK. Please do it properly.

Thanks,
Jes

Re: [PATCH 2/2] restripe: fix ignoring return value of ‘read’

[Guoqing Jiang]

On 5/18/20 7:22 PM, Jes Sorensen wrote:
> On 5/15/20 9:40 AM, Guoqing Jiang wrote:
>> Got below error when run "make everything".
>>
>> restripe.c: In function ‘test_stripes’:
>> restripe.c:870:4: error: ignoring return value of ‘read’, declared with attribute warn_unused_result [-Werror=unused-result]
>>      read(source[i], stripes[i], chunk_size);
>>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Fix it by set the return value of ‘read’ to diskP, which should be
>> harmless since diskP will be set again before it is used.
>>
>> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
>> ---
>>   restripe.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/restripe.c b/restripe.c
>> index 31b07e8..21c90f5 100644
>> --- a/restripe.c
>> +++ b/restripe.c
>> @@ -867,7 +867,11 @@ int test_stripes(int *source, unsigned long long *offsets,
>>   
>>   		for (i = 0 ; i < raid_disks ; i++) {
>>   			lseek64(source[i], offsets[i]+start, 0);
>> -			read(source[i], stripes[i], chunk_size);
>> +			/*
>> +			 * To resolve "ignoring return value of ‘read’", it
>> +			 * should be harmless since diskP will be again later.
>> +			 */
>> +			diskP = read(source[i], stripes[i], chunk_size);
> It doesn't complain on Fedora 32, however checking the return value of
> lseek64 and read is a good thing.
>
> However what you have done is to just masking the return value and
> throwing it away, is not OK. Please do it properly.

Yes, it is used to suppress the warning. And set the return value to a 
new variable
could cause unused-but-set-variable, not sure if there is better way now.

Thanks,
Guoqing

Re: [PATCH 1/2] uuid.c: split uuid stuffs from util.c

[Guoqing Jiang]

On 5/18/20 7:18 PM, Jes Sorensen wrote:
> On 5/15/20 9:40 AM, Guoqing Jiang wrote:
>> Currently, 'make raid6check' is build broken since commit b06815989
>> ("mdadm: load default sysfs attributes after assemblation").
>>
>> /usr/bin/ld: sysfs.o: in function `sysfsline':
>> sysfs.c:(.text+0x2707): undefined reference to `parse_uuid'
>> /usr/bin/ld: sysfs.c:(.text+0x271a): undefined reference to `uuid_zero'
>> /usr/bin/ld: sysfs.c:(.text+0x2721): undefined reference to `uuid_zero'
>>
>> Apparently, the compile of mdadm or raid6check are coupled with uuid
>> functions inside util.c. However, we can't just add util.o to CHECK_OBJS
>> which raid6check is needed, because it caused other worse problems.
>>
>> So, let's introduce a uuid.c file which is indenpended file to fix the
>> problem, all the contents are splitted from util.c.
>>
>> Cc: Piergiorgio Sartor <piergiorgio.sartor@nexgo.de>
>> Cc: Wolfgang Denk <wd@denx.de>
>> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
>> ---
>>   Makefile |  6 ++--
>>   util.c   | 87 -----------------------------------------------------
>>   uuid.c   | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 95 insertions(+), 90 deletions(-)
>>   create mode 100644 uuid.c
> I am fine with this change, but uuid.c needs to respect the license
> header that was in util.c

Ok, will copy it from util.c instead of just the below comment.

+/*
+ * Splited from util.c, so uuid.c shares the same copyright of it,
+ */
+


Thanks,
Guoqing

Re: [PATCH 2/2] restripe: fix ignoring return value of ‘read’

[Jes Sorensen]

On 5/18/20 1:32 PM, Guoqing Jiang wrote:
> On 5/18/20 7:22 PM, Jes Sorensen wrote:
>> On 5/15/20 9:40 AM, Guoqing Jiang wrote:
>>> Got below error when run "make everything".
>>>
>>> restripe.c: In function ‘test_stripes’:
>>> restripe.c:870:4: error: ignoring return value of ‘read’, declared
>>> with attribute warn_unused_result [-Werror=unused-result]
>>>      read(source[i], stripes[i], chunk_size);
>>>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Fix it by set the return value of ‘read’ to diskP, which should be
>>> harmless since diskP will be set again before it is used.
>>>
>>> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
>>> ---
>>>   restripe.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/restripe.c b/restripe.c
>>> index 31b07e8..21c90f5 100644
>>> --- a/restripe.c
>>> +++ b/restripe.c
>>> @@ -867,7 +867,11 @@ int test_stripes(int *source, unsigned long long
>>> *offsets,
>>>             for (i = 0 ; i < raid_disks ; i++) {
>>>               lseek64(source[i], offsets[i]+start, 0);
>>> -            read(source[i], stripes[i], chunk_size);
>>> +            /*
>>> +             * To resolve "ignoring return value of ‘read’", it
>>> +             * should be harmless since diskP will be again later.
>>> +             */
>>> +            diskP = read(source[i], stripes[i], chunk_size);
>> It doesn't complain on Fedora 32, however checking the return value of
>> lseek64 and read is a good thing.
>>
>> However what you have done is to just masking the return value and
>> throwing it away, is not OK. Please do it properly.
> 
> Yes, it is used to suppress the warning. And set the return value to a
> new variable
> could cause unused-but-set-variable, not sure if there is better way now.

The correct way is to check the return values and take appropriate
action if an error is returned.

Jes

Re: [PATCH 2/2] restripe: fix ignoring return value of ‘read’

[Guoqing Jiang]

On 5/18/20 7:50 PM, Jes Sorensen wrote:
> On 5/18/20 1:32 PM, Guoqing Jiang wrote:
>> On 5/18/20 7:22 PM, Jes Sorensen wrote:
>>> On 5/15/20 9:40 AM, Guoqing Jiang wrote:
>>>> Got below error when run "make everything".
>>>>
>>>> restripe.c: In function ‘test_stripes’:
>>>> restripe.c:870:4: error: ignoring return value of ‘read’, declared
>>>> with attribute warn_unused_result [-Werror=unused-result]
>>>>       read(source[i], stripes[i], chunk_size);
>>>>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> Fix it by set the return value of ‘read’ to diskP, which should be
>>>> harmless since diskP will be set again before it is used.
>>>>
>>>> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
>>>> ---
>>>>    restripe.c | 6 +++++-
>>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/restripe.c b/restripe.c
>>>> index 31b07e8..21c90f5 100644
>>>> --- a/restripe.c
>>>> +++ b/restripe.c
>>>> @@ -867,7 +867,11 @@ int test_stripes(int *source, unsigned long long
>>>> *offsets,
>>>>              for (i = 0 ; i < raid_disks ; i++) {
>>>>                lseek64(source[i], offsets[i]+start, 0);
>>>> -            read(source[i], stripes[i], chunk_size);
>>>> +            /*
>>>> +             * To resolve "ignoring return value of ‘read’", it
>>>> +             * should be harmless since diskP will be again later.
>>>> +             */
>>>> +            diskP = read(source[i], stripes[i], chunk_size);
>>> It doesn't complain on Fedora 32, however checking the return value of
>>> lseek64 and read is a good thing.
>>>
>>> However what you have done is to just masking the return value and
>>> throwing it away, is not OK. Please do it properly.
>> Yes, it is used to suppress the warning. And set the return value to a
>> new variable
>> could cause unused-but-set-variable, not sure if there is better way now.
> The correct way is to check the return values and take appropriate
> action if an error is returned.

Thanks, just find other places in restripe.c has check like this.

"read(source[dnum], buf+disk * chunk_size, chunk_size) != chunk_size)

Will send below changes if it is ok.

diff --git a/restripe.c b/restripe.c
index 31b07e8..48c6506 100644
--- a/restripe.c
+++ b/restripe.c
@@ -867,7 +867,15 @@ int test_stripes(int *source, unsigned long long 
*offsets,

                 for (i = 0 ; i < raid_disks ; i++) {
                         lseek64(source[i], offsets[i]+start, 0);
-                       read(source[i], stripes[i], chunk_size);
+                       if (read(source[i], stripes[i], chunk_size) !=
+                           chunk_size) {
+                               free(q);
+                               free(p);
+                               free(blocks);
+                               free(stripes);
+                               free(stripe_buf);
+                               return -1;
+                       }
                 }

Thanks,
Guoqing

Re: [PATCH 2/2] restripe: fix ignoring return value of ‘read’

[Jes Sorensen]

On 5/18/20 2:08 PM, Guoqing Jiang wrote:
> On 5/18/20 7:50 PM, Jes Sorensen wrote:
>> On 5/18/20 1:32 PM, Guoqing Jiang wrote:
>>> On 5/18/20 7:22 PM, Jes Sorensen wrote:
>>>> On 5/15/20 9:40 AM, Guoqing Jiang wrote:
>>>>> Got below error when run "make everything".
>>>>>
>>>>> restripe.c: In function ‘test_stripes’:
>>>>> restripe.c:870:4: error: ignoring return value of ‘read’, declared
>>>>> with attribute warn_unused_result [-Werror=unused-result]
>>>>>       read(source[i], stripes[i], chunk_size);
>>>>>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>
>>>>> Fix it by set the return value of ‘read’ to diskP, which should be
>>>>> harmless since diskP will be set again before it is used.
>>>>>
>>>>> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
>>>>> ---
>>>>>    restripe.c | 6 +++++-
>>>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/restripe.c b/restripe.c
>>>>> index 31b07e8..21c90f5 100644
>>>>> --- a/restripe.c
>>>>> +++ b/restripe.c
>>>>> @@ -867,7 +867,11 @@ int test_stripes(int *source, unsigned long long
>>>>> *offsets,
>>>>>              for (i = 0 ; i < raid_disks ; i++) {
>>>>>                lseek64(source[i], offsets[i]+start, 0);
>>>>> -            read(source[i], stripes[i], chunk_size);
>>>>> +            /*
>>>>> +             * To resolve "ignoring return value of ‘read’", it
>>>>> +             * should be harmless since diskP will be again later.
>>>>> +             */
>>>>> +            diskP = read(source[i], stripes[i], chunk_size);
>>>> It doesn't complain on Fedora 32, however checking the return value of
>>>> lseek64 and read is a good thing.
>>>>
>>>> However what you have done is to just masking the return value and
>>>> throwing it away, is not OK. Please do it properly.
>>> Yes, it is used to suppress the warning. And set the return value to a
>>> new variable
>>> could cause unused-but-set-variable, not sure if there is better way
>>> now.
>> The correct way is to check the return values and take appropriate
>> action if an error is returned.
> 
> Thanks, just find other places in restripe.c has check like this.
> 
> "read(source[dnum], buf+disk * chunk_size, chunk_size) != chunk_size)
> 
> Will send below changes if it is ok.
> 
> diff --git a/restripe.c b/restripe.c
> index 31b07e8..48c6506 100644
> --- a/restripe.c
> +++ b/restripe.c
> @@ -867,7 +867,15 @@ int test_stripes(int *source, unsigned long long
> *offsets,
> 
>                 for (i = 0 ; i < raid_disks ; i++) {
>                         lseek64(source[i], offsets[i]+start, 0);
> -                       read(source[i], stripes[i], chunk_size);
> +                       if (read(source[i], stripes[i], chunk_size) !=
> +                           chunk_size) {
> +                               free(q);
> +                               free(p);
> +                               free(blocks);
> +                               free(stripes);
> +                               free(stripe_buf);
> +                               return -1;
> +                       }

That should work, however you really should check the return value of
lseek as well, while looking at this.

Cheers,
Jes

Re: [PATCH 2/2] restripe: fix ignoring return value of ‘read’

[Guoqing Jiang]

On 5/18/20 11:06 PM, Jes Sorensen wrote:
> On 5/18/20 2:08 PM, Guoqing Jiang wrote:
>> On 5/18/20 7:50 PM, Jes Sorensen wrote:
>>> On 5/18/20 1:32 PM, Guoqing Jiang wrote:
>>>> On 5/18/20 7:22 PM, Jes Sorensen wrote:
>>>>> On 5/15/20 9:40 AM, Guoqing Jiang wrote:
>>>>>> Got below error when run "make everything".
>>>>>>
>>>>>> restripe.c: In function ‘test_stripes’:
>>>>>> restripe.c:870:4: error: ignoring return value of ‘read’, declared
>>>>>> with attribute warn_unused_result [-Werror=unused-result]
>>>>>>        read(source[i], stripes[i], chunk_size);
>>>>>>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>
>>>>>> Fix it by set the return value of ‘read’ to diskP, which should be
>>>>>> harmless since diskP will be set again before it is used.
>>>>>>
>>>>>> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
>>>>>> ---
>>>>>>     restripe.c | 6 +++++-
>>>>>>     1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/restripe.c b/restripe.c
>>>>>> index 31b07e8..21c90f5 100644
>>>>>> --- a/restripe.c
>>>>>> +++ b/restripe.c
>>>>>> @@ -867,7 +867,11 @@ int test_stripes(int *source, unsigned long long
>>>>>> *offsets,
>>>>>>               for (i = 0 ; i < raid_disks ; i++) {
>>>>>>                 lseek64(source[i], offsets[i]+start, 0);
>>>>>> -            read(source[i], stripes[i], chunk_size);
>>>>>> +            /*
>>>>>> +             * To resolve "ignoring return value of ‘read’", it
>>>>>> +             * should be harmless since diskP will be again later.
>>>>>> +             */
>>>>>> +            diskP = read(source[i], stripes[i], chunk_size);
>>>>> It doesn't complain on Fedora 32, however checking the return value of
>>>>> lseek64 and read is a good thing.
>>>>>
>>>>> However what you have done is to just masking the return value and
>>>>> throwing it away, is not OK. Please do it properly.
>>>> Yes, it is used to suppress the warning. And set the return value to a
>>>> new variable
>>>> could cause unused-but-set-variable, not sure if there is better way
>>>> now.
>>> The correct way is to check the return values and take appropriate
>>> action if an error is returned.
>> Thanks, just find other places in restripe.c has check like this.
>>
>> "read(source[dnum], buf+disk * chunk_size, chunk_size) != chunk_size)
>>
>> Will send below changes if it is ok.
>>
>> diff --git a/restripe.c b/restripe.c
>> index 31b07e8..48c6506 100644
>> --- a/restripe.c
>> +++ b/restripe.c
>> @@ -867,7 +867,15 @@ int test_stripes(int *source, unsigned long long
>> *offsets,
>>
>>                  for (i = 0 ; i < raid_disks ; i++) {
>>                          lseek64(source[i], offsets[i]+start, 0);
>> -                       read(source[i], stripes[i], chunk_size);
>> +                       if (read(source[i], stripes[i], chunk_size) !=
>> +                           chunk_size) {
>> +                               free(q);
>> +                               free(p);
>> +                               free(blocks);
>> +                               free(stripes);
>> +                               free(stripe_buf);
>> +                               return -1;
>> +                       }
> That should work, however you really should check the return value of
> lseek as well, while looking at this.

Okay, though I did not get complain about it =-O. Anyway, will send v3.

Thanks,
Guoqing

Re: [PATCH 2/2] restripe: fix ignoring return value of ‘read’

[antlists]

On 18/05/2020 22:37, Guoqing Jiang wrote:
>> That should work, however you really should check the return value of
>> lseek as well, while looking at this.
> 
> Okay, though I did not get complain about it =-O. Anyway, will send v3.

Just for info - I'm currently building my new system, which means my old 
system will become a raid testbed. And I'm playing with dm-integrity, 
which I believe returns an integrity error not a read error when things 
go wrong.

So yes, if errors aren't handled correctly I'm quite likely to find 
myself falling over them ...

Cheers,
Wol