* [PATCH 0/2] GCC 7 fixes, still need help with smc.c
@ 2017-07-18 20:52 Andy Grover
2017-07-18 20:52 ` [PATCH 1/2] Fix header warning with GCC 7 for major() Andy Grover
2017-07-18 20:52 ` [PATCH 2/2] Remove incorrect size specifier in spc_lu_init Andy Grover
0 siblings, 2 replies; 6+ messages in thread
From: Andy Grover @ 2017-07-18 20:52 UTC (permalink / raw)
To: stgt
tgt doesn't build on Fedora 26 using GCC 7, which has added some
additional snprintf compilation checks. These two patches fix some
minor ones, but there are also a number in smc.c. I don't have enough
familiarity with that code or the SMC spec to be confident I'd fix
them properly. Maybe someone else could take a look? Here they are:
smc.c: In function ‘build_element_descriptors’:
smc.c:164:41: error: ‘snprintf’ output truncated before the last format character [-Werror=format-truncation=]
snprintf((char *)&data[i], 32, "%-32s", s->volume_tag);
^
smc.c:164:4: note: ‘snprintf’ output 33 bytes into a destination of size 32
snprintf((char *)&data[i], 32, "%-32s", s->volume_tag);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
smc.c:166:41: error: ‘snprintf’ output truncated before the last format character [-Werror=format-truncation=]
snprintf((char *)&data[i], 32, "%-32s", s->barcode);
^
smc.c:166:4: note: ‘snprintf’ output 33 bytes into a destination of size 32
snprintf((char *)&data[i], 32, "%-32s", s->barcode);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
smc.c:184:40: error: ‘%-10s’ directive output may be truncated writing between 10 and 36 bytes into a region of size 11 [-Werror=format-truncation=]
snprintf((char *)&data[i + 28], 11, "%-10s", attr->scsi_sn);
^~~~~
smc.c:184:3: note: ‘snprintf’ output between 11 and 37 bytes into a destination of size 11
snprintf((char *)&data[i + 28], 11, "%-10s", attr->scsi_sn);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:94: smc.o] Error 1
make: *** [Makefile:24: programs] Error 2
Thanks -- Regards -- Andy
Andy Grover (2):
Fix header warning with GCC 7 for major()
Remove incorrect size specifier in spc_lu_init
usr/bs_sg.c | 1 +
usr/spc.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
--
2.9.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] Fix header warning with GCC 7 for major()
2017-07-18 20:52 [PATCH 0/2] GCC 7 fixes, still need help with smc.c Andy Grover
@ 2017-07-18 20:52 ` Andy Grover
2017-07-18 20:52 ` [PATCH 2/2] Remove incorrect size specifier in spc_lu_init Andy Grover
1 sibling, 0 replies; 6+ messages in thread
From: Andy Grover @ 2017-07-18 20:52 UTC (permalink / raw)
To: stgt
With GCC 7, there is a warning to include sys/sysmacros.h for "major"
instead of sys/types.h.
Signed-off-by: Andy Grover <agrover@redhat.com>
---
usr/bs_sg.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/usr/bs_sg.c b/usr/bs_sg.c
index 66f4a3b..6d03aa4 100644
--- a/usr/bs_sg.c
+++ b/usr/bs_sg.c
@@ -35,6 +35,7 @@
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/epoll.h>
+#include <sys/sysmacros.h>
#include <scsi/sg.h>
#include "bsg.h" /* Copied from include/linux/bsg.h */
--
2.9.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] Remove incorrect size specifier in spc_lu_init
2017-07-18 20:52 [PATCH 0/2] GCC 7 fixes, still need help with smc.c Andy Grover
2017-07-18 20:52 ` [PATCH 1/2] Fix header warning with GCC 7 for major() Andy Grover
@ 2017-07-18 20:52 ` Andy Grover
2017-07-27 7:11 ` FUJITA Tomonori
1 sibling, 1 reply; 6+ messages in thread
From: Andy Grover @ 2017-07-18 20:52 UTC (permalink / raw)
To: stgt
The vendor_id field is only 9 bytes, so GCC 7 complains when a size
specifier of 16 is given.
Signed-off-by: Andy Grover <agrover@redhat.com>
---
usr/spc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/usr/spc.c b/usr/spc.c
index cdb8a2a..82a6ec9 100644
--- a/usr/spc.c
+++ b/usr/spc.c
@@ -2069,7 +2069,7 @@ int spc_lu_init(struct scsi_lu *lu)
lu->attrs.sense_format = 0;
snprintf(lu->attrs.vendor_id, sizeof(lu->attrs.vendor_id),
- "%-16s", VENDOR_ID);
+ "%-s", VENDOR_ID);
snprintf(lu->attrs.product_rev, sizeof(lu->attrs.product_rev),
"%s", "0001");
snprintf(lu->attrs.scsi_id, sizeof(lu->attrs.scsi_id),
--
2.9.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Remove incorrect size specifier in spc_lu_init
2017-07-18 20:52 ` [PATCH 2/2] Remove incorrect size specifier in spc_lu_init Andy Grover
@ 2017-07-27 7:11 ` FUJITA Tomonori
2017-07-27 19:16 ` Andy Grover
0 siblings, 1 reply; 6+ messages in thread
From: FUJITA Tomonori @ 2017-07-27 7:11 UTC (permalink / raw)
To: agrover; +Cc: stgt
Sorry about the delay,
On Tue, 18 Jul 2017 13:52:58 -0700
Andy Grover <agrover@redhat.com> wrote:
> The vendor_id field is only 9 bytes, so GCC 7 complains when a size
> specifier of 16 is given.
>
> Signed-off-by: Andy Grover <agrover@redhat.com>
> ---
> usr/spc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/usr/spc.c b/usr/spc.c
> index cdb8a2a..82a6ec9 100644
> --- a/usr/spc.c
> +++ b/usr/spc.c
> @@ -2069,7 +2069,7 @@ int spc_lu_init(struct scsi_lu *lu)
> lu->attrs.sense_format = 0;
>
> snprintf(lu->attrs.vendor_id, sizeof(lu->attrs.vendor_id),
> - "%-16s", VENDOR_ID);
> + "%-s", VENDOR_ID);
> snprintf(lu->attrs.product_rev, sizeof(lu->attrs.product_rev),
snprintf() writes a trailing null. the size of lu->attrs.vendor_id is
exactly 16 bytes. Seems that snprintf on some platforms writes only 15
bytes of the string at a maximum.
What we need here is writing 16 bytes of the string at a maximum
(without a trailing null). Seems that no way to tell snprintf not to
write a tailing null... A helper function like the following might
help?
Thought?
diff --git a/usr/spc.c b/usr/spc.c
index cdb8a2a..b6a77e6 100644
--- a/usr/spc.c
+++ b/usr/spc.c
@@ -2068,7 +2068,7 @@ int spc_lu_init(struct scsi_lu *lu)
lu->attrs.swp = 0;
lu->attrs.sense_format = 0;
- snprintf(lu->attrs.vendor_id, sizeof(lu->attrs.vendor_id),
+ scsi_sprintf(lu->attrs.vendor_id, sizeof(lu->attrs.vendor_id),
"%-16s", VENDOR_ID);
snprintf(lu->attrs.product_rev, sizeof(lu->attrs.product_rev),
"%s", "0001");
diff --git a/usr/util.h b/usr/util.h
index 256bdb8..191d76e 100644
--- a/usr/util.h
+++ b/usr/util.h
@@ -240,4 +240,18 @@ static __always_inline int test_bit(unsigned int nr, const unsigned long *addr)
(((unsigned long *)addr)[nr / BITS_PER_LONG])) != 0;
}
+static inline int scsi_sprintf(char *str, size_t size, const char *format, ...)
+{
+ va_list args;
+ char buf[size + 1];
+ int n;
+
+ memset(buf, 0, sizeof(buf));
+ va_start(args, format);
+ n = snprintf(buf, size, format, args);
+ va_end(args);
+ memcpy(str, buf, size);
+ return n;
+}
+
#endif
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Remove incorrect size specifier in spc_lu_init
2017-07-27 7:11 ` FUJITA Tomonori
@ 2017-07-27 19:16 ` Andy Grover
2017-07-27 22:59 ` FUJITA Tomonori
0 siblings, 1 reply; 6+ messages in thread
From: Andy Grover @ 2017-07-27 19:16 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: stgt
On 07/27/2017 12:11 AM, FUJITA Tomonori wrote:
> Sorry about the delay,
>
> On Tue, 18 Jul 2017 13:52:58 -0700
> Andy Grover <agrover@redhat.com> wrote:
>
>> The vendor_id field is only 9 bytes, so GCC 7 complains when a size
>> specifier of 16 is given.
>>
>> Signed-off-by: Andy Grover <agrover@redhat.com>
>> ---
>> usr/spc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/usr/spc.c b/usr/spc.c
>> index cdb8a2a..82a6ec9 100644
>> --- a/usr/spc.c
>> +++ b/usr/spc.c
>> @@ -2069,7 +2069,7 @@ int spc_lu_init(struct scsi_lu *lu)
>> lu->attrs.sense_format = 0;
>>
>> snprintf(lu->attrs.vendor_id, sizeof(lu->attrs.vendor_id),
>> - "%-16s", VENDOR_ID);
>> + "%-s", VENDOR_ID);
>> snprintf(lu->attrs.product_rev, sizeof(lu->attrs.product_rev),
>
> snprintf() writes a trailing null. the size of lu->attrs.vendor_id is
> exactly 16 bytes. Seems that snprintf on some platforms writes only 15
> bytes of the string at a maximum.
> What we need here is writing 16 bytes of the string at a maximum
> (without a trailing null). Seems that no way to tell snprintf not to
> write a tailing null... A helper function like the following might
> help?
>
> Thought?
>
> diff --git a/usr/spc.c b/usr/spc.c
> index cdb8a2a..b6a77e6 100644
> --- a/usr/spc.c
> +++ b/usr/spc.c
> @@ -2068,7 +2068,7 @@ int spc_lu_init(struct scsi_lu *lu)
> lu->attrs.swp = 0;
> lu->attrs.sense_format = 0;
>
> - snprintf(lu->attrs.vendor_id, sizeof(lu->attrs.vendor_id),
> + scsi_sprintf(lu->attrs.vendor_id, sizeof(lu->attrs.vendor_id),
> "%-16s", VENDOR_ID);
> snprintf(lu->attrs.product_rev, sizeof(lu->attrs.product_rev),
> "%s", "0001");
Well I think there are two cases. First is the above case, where we're
just filling in our own private fixed width data field. In this case
this line is fine except the minimum width specifier (16) is larger than
the actual sizeof(lu->attrs.vendor_id), which is 9. One was added to
VENDOR_ID_LEN for the field to ensure there was space to null-terminate
the string, so this is one spot where I think snprintf is fine, once the
"16" is removed.
But for all other cases, especially those in smc.c, yes I think we need
scsi_snprintf(), for properly writing fixed width, NOT null-terminated,
left-aligned and maybe padded with spaces fields, which are common in SCSI.
Regards -- Andy
> diff --git a/usr/util.h b/usr/util.h
> index 256bdb8..191d76e 100644
> --- a/usr/util.h
> +++ b/usr/util.h
> @@ -240,4 +240,18 @@ static __always_inline int test_bit(unsigned int nr, const unsigned long *addr)
> (((unsigned long *)addr)[nr / BITS_PER_LONG])) != 0;
> }
>
> +static inline int scsi_sprintf(char *str, size_t size, const char *format, ...)
> +{
> + va_list args;
> + char buf[size + 1];
> + int n;
> +
> + memset(buf, 0, sizeof(buf));
> + va_start(args, format);
> + n = snprintf(buf, size, format, args);
> + va_end(args);
> + memcpy(str, buf, size);
> + return n;
> +}
> +
> #endif
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Remove incorrect size specifier in spc_lu_init
2017-07-27 19:16 ` Andy Grover
@ 2017-07-27 22:59 ` FUJITA Tomonori
0 siblings, 0 replies; 6+ messages in thread
From: FUJITA Tomonori @ 2017-07-27 22:59 UTC (permalink / raw)
To: agrover; +Cc: tomof, stgt
On Thu, 27 Jul 2017 12:16:19 -0700
Andy Grover <agrover@redhat.com> wrote:
>> diff --git a/usr/spc.c b/usr/spc.c
>> index cdb8a2a..b6a77e6 100644
>> --- a/usr/spc.c
>> +++ b/usr/spc.c
>> @@ -2068,7 +2068,7 @@ int spc_lu_init(struct scsi_lu *lu)
>> lu->attrs.swp = 0;
>> lu->attrs.sense_format = 0;
>> - snprintf(lu->attrs.vendor_id, sizeof(lu->attrs.vendor_id),
>> + scsi_sprintf(lu->attrs.vendor_id, sizeof(lu->attrs.vendor_id),
>> "%-16s", VENDOR_ID);
>> snprintf(lu->attrs.product_rev, sizeof(lu->attrs.product_rev),
>> "%s", "0001");
>
> Well I think there are two cases. First is the above case, where we're
> just filling in our own private fixed width data field. In this case
> this line is fine except the minimum width specifier (16) is larger
> than the actual sizeof(lu->attrs.vendor_id), which is 9. One was added
> to VENDOR_ID_LEN for the field to ensure there was space to
> null-terminate the string, so this is one spot where I think snprintf
> is fine, once the "16" is removed.
Oops, I overlooked that sizeof(lu->attrs.vendor_id) is 9. We were wise
enough to allocate VENDOR_ID_LEN + 1 here :)
As you said, just removing "16" looks fine.
I've pushed your patches, thanks!
> But for all other cases, especially those in smc.c, yes I think we
> need scsi_snprintf(), for properly writing fixed width, NOT
> null-terminated, left-aligned and maybe padded with spaces fields,
> which are common in SCSI.
I'll look at smc.c again later.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-27 22:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-18 20:52 [PATCH 0/2] GCC 7 fixes, still need help with smc.c Andy Grover
2017-07-18 20:52 ` [PATCH 1/2] Fix header warning with GCC 7 for major() Andy Grover
2017-07-18 20:52 ` [PATCH 2/2] Remove incorrect size specifier in spc_lu_init Andy Grover
2017-07-27 7:11 ` FUJITA Tomonori
2017-07-27 19:16 ` Andy Grover
2017-07-27 22:59 ` FUJITA Tomonori
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).