Re: [PATCH 3/4] lsscsi: code shrink and refactor
He shouldn't do anything stupid at all to begin with! Now go and sit in the corner! //Markus Sent from my BlackBerry - the most secure mobile device Original Message From: xon...@gmail.com Sent: July 11, 2020 17:57 To: martin.lewis@gmail.com Cc: busybox@busybox.net Subject: Re: [PATCH 3/4] lsscsi: code shrink and refactor Hi Martin, > Remove the use of strou because scsi device types are signed (as seen in the > kernel's code). > Improve the representation of SCSI_DEVICE_LIST. Maybe you should make those changes in two patches: one that seems nice and is not attracting much attention (improve SCSI_DEVICE_LIST), and the other to remove bb_strtou(), that seems a bit controversial. :) Cheers, Xabier Oneca_,,_ ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 3/4] lsscsi: code shrink and refactor
Hi Martin, > Remove the use of strou because scsi device types are signed (as seen in the > kernel's code). > Improve the representation of SCSI_DEVICE_LIST. Maybe you should make those changes in two patches: one that seems nice and is not attracting much attention (improve SCSI_DEVICE_LIST), and the other to remove bb_strtou(), that seems a bit controversial. :) Cheers, Xabier Oneca_,,_ ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 3/4] lsscsi: code shrink and refactor
Allowing erroneous pages to render is not inherently bad, especially with standards that can change in the future. Look at earlier HTML (< 4) vs XHTML. If a page is written in XHTML and is read by a browser that doesn't understand XHTML, the browser still attempting to render it by ignoring any junk it doesn't know how to handle (the self-closing tags, for example) would be a very desirable thing for the end user. I don't think this is a good counter-example for this reason. On July 9, 2020 4:59:09 PM EDT, Michael Conrad wrote: >On 7/9/2020 3:16 PM, Markus Gothe wrote: >> Jon Postel formulated the robustness principle decades ago. Still >> today it is a good advice to "be liberal in what you accept and >strict >> in what you send". > >Counterexample: Internet Explorer > >It allowed so much garbage to render correctly that other browser >vendors had to work overtime to accept all the same garbage and make >sure it rendered in the same way. Then, subsequently when IE was no >longer defining the standard, progress was hamstrung by needing to be >compatible with its own past allowances lest they be accused of >breaking >people intranets. So much so that they just weren't able to fix most >of >their bugs and eventually abandoned the project. If they had just >declared tighter standards and enforced the rules, web development >might >not have been a misery for an entire decade. > >___ >busybox mailing list >busybox@busybox.net >http://lists.busybox.net/mailman/listinfo/busybox -- Sent from my Android phone with K-9 Mail. Please excuse my brevity. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 3/4] lsscsi: code shrink and refactor
The robustness principle is about handling faulty behaviours and not about implementing not-defined/non-standard things just to be compatible with other vendors. However if another vendor uses something non/not-defined one should take care of the corner-case and not let the running code result in an unexpected crash. So even if Netscape didn't implemented the marquee-tag it actually rendered HTML containing it without crashing or doing something unexpected. So if the kernel gets a bug in the sysfs that gives us a number atoi() cannot handle we will represent it incorrectly. Better to use the built-in error handling instead. //Markus Sent from my BlackBerry - the most secure mobile device Original Message From: mcon...@intellitree.com Sent: July 9, 2020 23:01 To: busybox@busybox.net Subject: Re: [PATCH 3/4] lsscsi: code shrink and refactor On 7/9/2020 3:16 PM, Markus Gothe wrote: > Jon Postel formulated the robustness principle decades ago. Still > today it is a good advice to "be liberal in what you accept and strict > in what you send". Counterexample: Internet Explorer It allowed so much garbage to render correctly that other browser vendors had to work overtime to accept all the same garbage and make sure it rendered in the same way. Then, subsequently when IE was no longer defining the standard, progress was hamstrung by needing to be compatible with its own past allowances lest they be accused of breaking people intranets. So much so that they just weren't able to fix most of their bugs and eventually abandoned the project. If they had just declared tighter standards and enforced the rules, web development might not have been a misery for an entire decade. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 3/4] lsscsi: code shrink and refactor
On 7/9/2020 3:16 PM, Markus Gothe wrote: Jon Postel formulated the robustness principle decades ago. Still today it is a good advice to "be liberal in what you accept and strict in what you send". Counterexample: Internet Explorer It allowed so much garbage to render correctly that other browser vendors had to work overtime to accept all the same garbage and make sure it rendered in the same way. Then, subsequently when IE was no longer defining the standard, progress was hamstrung by needing to be compatible with its own past allowances lest they be accused of breaking people intranets. So much so that they just weren't able to fix most of their bugs and eventually abandoned the project. If they had just declared tighter standards and enforced the rules, web development might not have been a misery for an entire decade. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH 3/4] lsscsi: code shrink and refactor
Jon Postel formulated the robustness principle decades ago. Still today it is a good advice to "be liberal in what you accept and strict in what you send".Micro-optimizations as this will cause more problems when things actually do fail than they are worth in the long run.I could buy the argumentation IFF one need C89 compliance or the applet was the single user of the bb_stroi() function. However it is not and hence the footprint will just be a few bytes extra.If size is a burning matter then don't use the ls* family of applets, use the sysfs directly. They are supposed to give a clean a stable API on-top of the sysfs. Many layers might be something to consider when it's cold outside and not in tiny embedded systems.//Markus Sent from my BlackBerry - the most secure mobile device From: martin.lewis@gmail.comSent: July 9, 2020 15:14To: nietzs...@lysator.liu.seCc: busybox@busybox.netSubject: Re: [PATCH 3/4] lsscsi: code shrink and refactor Here's the struct as specified in the kernel:/* From /drivers/scsi/scsi_sysfs.c in the kernel: * sdev_rd_attr (type, "%d\n"); * sdev_rd_attr (vendor, "%.8s\n"); * sdev_rd_attr (model, "%.16s\n"); * sdev_rd_attr (rev, "%.4s\n"); */As you can see, type is always printed with %d which should always be a valid signed integer.Could you please give an example of a type for which atoi is not sufficient?The usage of errno and bb_strtoi increases the size of the binary, so unless it's required I don't think it should be used.MartinOn Fri, 3 Jul 2020 at 14:45, Markus Gothe <nietzs...@lysator.liu.se> wrote:As the original author of the applet I don't want to see atoi() calls in the code. You know what atoi() returns when it fails? Same as atoi("0").
Please use bb_stroi() for signed integers.
BR,
Markus
Sent from my BlackBerry - the most secure mobile device
Original Message
From: martin.lewis@gmail.com
Sent: June 11, 2020 15:45
To: busybox@busybox.net
Subject: [PATCH 3/4] lsscsi: code shrink and refactor
Remove the use of strou because scsi device types are signed (as seen in the kernel's code).
Improve the representation of SCSI_DEVICE_LIST.
function old new delta
.rodata 159915 159920 +5
lsscsi_main 364 349 -15
--
(add/remove: 0/0 grow/shrink: 1/1 up/down: 5/-15) Total: -10 bytes
text data bss dec hex filename
981332 16915 1872 1000119 f42b7 busybox_old
981322 16915 1872 1000109 f42ad busybox_unstripped
Signed-off-by: Martin Lewis <martin.lewis@gmail.com>
---
miscutils/lsscsi.c | 50 ++
1 file changed, 34 insertions(+), 16 deletions(-)
diff --git a/miscutils/lsscsi.c b/miscutils/lsscsi.c
index f737d33d9..e29bedd77 100644
--- a/miscutils/lsscsi.c
+++ b/miscutils/lsscsi.c
@@ -25,6 +25,28 @@
#include "libbb.h"
+#define SCSI_DEVICE_TYPE(type, name) type name "\0"
+#define SCSI_DEVICE_TYPE_LIST \
+ SCSI_DEVICE_TYPE("\x00", "disk") \
+ SCSI_DEVICE_TYPE("\x01", "tape") \
+ SCSI_DEVICE_TYPE("\x02", "printer") \
+ SCSI_DEVICE_TYPE("\x03", "process") \
+ SCSI_DEVICE_TYPE("\x04", "worm") \
+ SCSI_DEVICE_TYPE("\x06", "scanner") \
+ SCSI_DEVICE_TYPE("\x07", "optical") \
+ SCSI_DEVICE_TYPE("\x08", "mediumx") \
+ SCSI_DEVICE_TYPE("\x09", "comms") \
+ SCSI_DEVICE_TYPE("\x0c", "storage") \
+ SCSI_DEVICE_TYPE("\x0d", "enclosu") \
+ SCSI_DEVICE_TYPE("\x0e", "sim dsk") \
+ SCSI_DEVICE_TYPE("\x0f", "opti rd") \
+ SCSI_DEVICE_TYPE("\x10", "bridge") \
+ SCSI_DEVICE_TYPE("\x11", "osd") \
+ SCSI_DEVICE_TYPE("\x12", "adi") \
+ SCSI_DEVICE_TYPE("\x1e", "wlun") \
+ SCSI_DEVICE_TYPE("\x1f", "no dev")
+
+
static const char scsi_dir[] ALIGN1 = "/sys/bus/scsi/devices";
static char *get_line(const char *filename, char *buf, unsigned *bufsize_p)
@@ -59,6 +81,13 @@ int lsscsi_main(int argc UNUSED_PARAM, char **argv UNUSED_PARAM)
dir = xopendir(".");
while ((de = readdir(dir)) != NULL) {
+ /*
+ * From /drivers/scsi/scsi_sysfs.c in the kernel:
+ * sdev_rd_attr (type, "%d\n");
Re: [PATCH 3/4] lsscsi: code shrink and refactor
Here's the struct as specified in the kernel:
/* From /drivers/scsi/scsi_sysfs.c in the kernel:
* sdev_rd_attr (type, "%d\n");
* sdev_rd_attr (vendor, "%.8s\n");
* sdev_rd_attr (model, "%.16s\n");
* sdev_rd_attr (rev, "%.4s\n");
*/
As you can see, type is always printed with %d which should always be a
valid signed integer.
Could you please give an example of a type for which atoi is not sufficient?
The usage of errno and bb_strtoi increases the size of the binary, so
unless it's required I don't think it should be used.
Martin
On Fri, 3 Jul 2020 at 14:45, Markus Gothe wrote:
> As the original author of the applet I don't want to see atoi() calls in
> the code. You know what atoi() returns when it fails? Same as atoi("0").
>
> Please use bb_stroi() for signed integers.
>
> BR,
> Markus
>
> Sent from my BlackBerry - the most secure mobile device
>
>
> Original Message
>
>
> From: martin.lewis@gmail.com
> Sent: June 11, 2020 15:45
> To: busybox@busybox.net
> Subject: [PATCH 3/4] lsscsi: code shrink and refactor
>
>
> Remove the use of strou because scsi device types are signed (as seen in
> the kernel's code).
> Improve the representation of SCSI_DEVICE_LIST.
>
> function old new delta
> .rodata 159915 159920 +5
> lsscsi_main 364 349 -15
>
> --
> (add/remove: 0/0 grow/shrink: 1/1 up/down: 5/-15) Total: -10
> bytes
>textdata bss dec hex filename
> 981332 169151872 1000119 f42b7 busybox_old
> 981322 169151872 1000109 f42ad busybox_unstripped
>
> Signed-off-by: Martin Lewis
> ---
> miscutils/lsscsi.c | 50 ++
> 1 file changed, 34 insertions(+), 16 deletions(-)
>
> diff --git a/miscutils/lsscsi.c b/miscutils/lsscsi.c
> index f737d33d9..e29bedd77 100644
> --- a/miscutils/lsscsi.c
> +++ b/miscutils/lsscsi.c
> @@ -25,6 +25,28 @@
>
> #include "libbb.h"
>
> +#define SCSI_DEVICE_TYPE(type, name) type name "\0"
> +#define SCSI_DEVICE_TYPE_LIST \
> + SCSI_DEVICE_TYPE("\x00", "disk") \
> + SCSI_DEVICE_TYPE("\x01", "tape") \
> + SCSI_DEVICE_TYPE("\x02", "printer") \
> + SCSI_DEVICE_TYPE("\x03", "process") \
> + SCSI_DEVICE_TYPE("\x04", "worm") \
> + SCSI_DEVICE_TYPE("\x06", "scanner") \
> + SCSI_DEVICE_TYPE("\x07", "optical") \
> + SCSI_DEVICE_TYPE("\x08", "mediumx") \
> + SCSI_DEVICE_TYPE("\x09", "comms") \
> + SCSI_DEVICE_TYPE("\x0c", "storage") \
> + SCSI_DEVICE_TYPE("\x0d", "enclosu") \
> + SCSI_DEVICE_TYPE("\x0e", "sim dsk") \
> + SCSI_DEVICE_TYPE("\x0f", "opti rd") \
> + SCSI_DEVICE_TYPE("\x10", "bridge") \
> + SCSI_DEVICE_TYPE("\x11", "osd") \
> + SCSI_DEVICE_TYPE("\x12", "adi") \
> + SCSI_DEVICE_TYPE("\x1e", "wlun") \
> + SCSI_DEVICE_TYPE("\x1f", "no dev")
> +
> +
> static const char scsi_dir[] ALIGN1 = "/sys/bus/scsi/devices";
>
> static char *get_line(const char *filename, char *buf, unsigned *bufsize_p)
> @@ -59,6 +81,13 @@ int lsscsi_main(int argc UNUSED_PARAM, char **argv
> UNUSED_PARAM)
>
> dir = xopendir(".");
> while ((de = readdir(dir)) != NULL) {
> + /*
> + * From /drivers/scsi/scsi_sysfs.c in the kernel:
> + * sdev_rd_attr (type, "%d\n");
> + * sdev_rd_attr (vendor, "%.8s\n");
> + * sdev_rd_attr (model, "%.16s\n");
> + * sdev_rd_attr (rev, "%.4s\n");
> + */
> char buf[256];
> char *ptr;
> unsigned bufsize;
> @@ -67,7 +96,7 @@ int lsscsi_main(int argc UNUSED_PARAM, char **argv
> UNUSED_PARAM)
> const char *type_name;
> const char *model;
> const char *rev;
> - unsigned type;
> + int type;
>
> if (!isdigit(de->d_name[0]))
> continue;
> @@ -88,23 +117,12 @@ int lsscsi_main(int argc UNUSED_PARAM, char **argv
> UNUSED_PARAM)
>
> printf("[%s]\t", de->d_name);
>
> -#define scsi_device_types \
> - "disk\0""tape\0""printer\0" "process\0" \
> - "worm\0""\0""scanner\0" "optical\0" \
> - "mediumx\0" "comms\0" "\0""\0"\
> - "storage\0" "enclosu\0" "sim dsk\0" "opti rd\0" \
> - "bridge\0" "osd\0" "adi\0" "\0"\
> - "\0""\0""\0""\0"\
> - "\0""\0""\0""\0"\
> - "\0""\0""wlun\0""no dev"
> - type = bb_strtou(type_str, NULL, 10);
> - if (errno
> - || type >= 0x20
> - || (type_name = nth_string(scsi_device_types, type))[0] == '\0'
> - ) {
> + type = atoi(type_str); /* type_str is "%d\n" so atoi is sufficient */
> + if (type >= 0x20 ||
> + (type_name = memchr(SCSI_DEVICE_TYPE_LIST, type,
> sizeof(SCSI_DEVICE_TYPE_LIST))) == NULL) {
> printf("(%s)\t", type_str);
> } else {
> - printf("%s\t", type_name);
> + printf("%s\t", type_name + 1);
> }
>
> printf("%s\t""%s\t""%s\n",
> --
> 2.11.0
>
> ___
> busybox mailing list
> busybox@busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox
>
Re: [PATCH 3/4] lsscsi: code shrink and refactor
As the original author of the applet I don't want to see atoi() calls in the
code. You know what atoi() returns when it fails? Same as atoi("0").
Please use bb_stroi() for signed integers.
BR,
Markus
Sent from my BlackBerry - the most secure mobile device
Original Message
From: martin.lewis@gmail.com
Sent: June 11, 2020 15:45
To: busybox@busybox.net
Subject: [PATCH 3/4] lsscsi: code shrink and refactor
Remove the use of strou because scsi device types are signed (as seen in the
kernel's code).
Improve the representation of SCSI_DEVICE_LIST.
function old new delta
.rodata 159915 159920 +5
lsscsi_main 364 349 -15
--
(add/remove: 0/0 grow/shrink: 1/1 up/down: 5/-15) Total: -10 bytes
text data bss dec hex filename
981332 16915 1872 1000119 f42b7 busybox_old
981322 16915 1872 1000109 f42ad busybox_unstripped
Signed-off-by: Martin Lewis
---
miscutils/lsscsi.c | 50 ++
1 file changed, 34 insertions(+), 16 deletions(-)
diff --git a/miscutils/lsscsi.c b/miscutils/lsscsi.c
index f737d33d9..e29bedd77 100644
--- a/miscutils/lsscsi.c
+++ b/miscutils/lsscsi.c
@@ -25,6 +25,28 @@
#include "libbb.h"
+#define SCSI_DEVICE_TYPE(type, name) type name "\0"
+#define SCSI_DEVICE_TYPE_LIST \
+ SCSI_DEVICE_TYPE("\x00", "disk") \
+ SCSI_DEVICE_TYPE("\x01", "tape") \
+ SCSI_DEVICE_TYPE("\x02", "printer") \
+ SCSI_DEVICE_TYPE("\x03", "process") \
+ SCSI_DEVICE_TYPE("\x04", "worm") \
+ SCSI_DEVICE_TYPE("\x06", "scanner") \
+ SCSI_DEVICE_TYPE("\x07", "optical") \
+ SCSI_DEVICE_TYPE("\x08", "mediumx") \
+ SCSI_DEVICE_TYPE("\x09", "comms") \
+ SCSI_DEVICE_TYPE("\x0c", "storage") \
+ SCSI_DEVICE_TYPE("\x0d", "enclosu") \
+ SCSI_DEVICE_TYPE("\x0e", "sim dsk") \
+ SCSI_DEVICE_TYPE("\x0f", "opti rd") \
+ SCSI_DEVICE_TYPE("\x10", "bridge") \
+ SCSI_DEVICE_TYPE("\x11", "osd") \
+ SCSI_DEVICE_TYPE("\x12", "adi") \
+ SCSI_DEVICE_TYPE("\x1e", "wlun") \
+ SCSI_DEVICE_TYPE("\x1f", "no dev")
+
+
static const char scsi_dir[] ALIGN1 = "/sys/bus/scsi/devices";
static char *get_line(const char *filename, char *buf, unsigned *bufsize_p)
@@ -59,6 +81,13 @@ int lsscsi_main(int argc UNUSED_PARAM, char **argv
UNUSED_PARAM)
dir = xopendir(".");
while ((de = readdir(dir)) != NULL) {
+ /*
+ * From /drivers/scsi/scsi_sysfs.c in the kernel:
+ * sdev_rd_attr (type, "%d\n");
+ * sdev_rd_attr (vendor, "%.8s\n");
+ * sdev_rd_attr (model, "%.16s\n");
+ * sdev_rd_attr (rev, "%.4s\n");
+ */
char buf[256];
char *ptr;
unsigned bufsize;
@@ -67,7 +96,7 @@ int lsscsi_main(int argc UNUSED_PARAM, char **argv
UNUSED_PARAM)
const char *type_name;
const char *model;
const char *rev;
- unsigned type;
+ int type;
if (!isdigit(de->d_name[0]))
continue;
@@ -88,23 +117,12 @@ int lsscsi_main(int argc UNUSED_PARAM, char **argv
UNUSED_PARAM)
printf("[%s]\t", de->d_name);
-#define scsi_device_types \
- "disk\0" "tape\0" "printer\0" "process\0" \
- "worm\0" "\0" "scanner\0" "optical\0" \
- "mediumx\0" "comms\0" "\0" "\0" \
- "storage\0" "enclosu\0" "sim dsk\0" "opti rd\0" \
- "bridge\0" "osd\0" "adi\0" "\0" \
- "\0" "\0" "\0" "\0" \
- "\0" "\0" "\0" "\0" \
- "\0" "\0" "wlun\0" "no dev"
- type = bb_strtou(type_str, NULL, 10);
- if (errno
- || type >= 0x20
- || (type_name = nth_string(scsi_device_types, type))[0] == '\0'
- ) {
+ type = atoi(type_str); /* type_str is "%d\n" so atoi is sufficient */
+ if (type >= 0x20 ||
+ (type_name = memchr(SCSI_DEVICE_TYPE_LIST, type,
sizeof(SCSI_DEVICE_TYPE_LIST))) == NULL) {
printf("(%s)\t", type_str);
} else {
- printf("%s\t", type_name);
+ printf("%s\t", type_name + 1);
}
printf("%s\t""%s\t""%s\n",
--
2.11.0
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
