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 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
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");
+ * 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
|