<html><head><style id="outgoing-font-settings">#response_container_BBPPID{font-family: initial; font-size:initial; color: initial;}</style></head><body style="background-color: rgb(255, 255, 255); background-image: initial; line-height: initial;"><div id="response_container_BBPPID" style="outline:none;" dir="auto" contenteditable="false"> <div name="BB10" id="BB10_response_div_BBPPID" dir="auto" style="width:100%;">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".</div><div name="BB10" id="BB10_response_div_BBPPID" dir="auto" style="width:100%;"><br></div><div name="BB10" id="BB10_response_div_BBPPID" dir="auto" style="width:100%;">Micro-optimizations as this will cause more problems when things actually do fail than they are worth in the long run.</div><div name="BB10" id="BB10_response_div_BBPPID" dir="auto" style="width:100%;"><span style="color: initial; font-family: initial; font-size: initial;"><br></span></div><div name="BB10" id="BB10_response_div_BBPPID" dir="auto" style="width:100%;"><span style="color: initial; font-family: initial; font-size: initial;">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.</span></div><div name="BB10" id="BB10_response_div_BBPPID" dir="auto" style="width:100%;"><span style="color: initial; font-family: initial; font-size: initial;"><br></span></div><div name="BB10" id="BB10_response_div_BBPPID" dir="auto" style="width:100%;"><span style="color: initial; font-family: initial; font-size: initial;">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.</span></div><div name="BB10" id="BB10_response_div_BBPPID" dir="auto" style="width:100%;"><span style="color: initial; font-family: initial; font-size: initial;"><br></span></div><div name="BB10" id="BB10_response_div_BBPPID" dir="auto" style="width:100%;"><span style="color: initial; font-family: initial; font-size: initial;">//Markus</span></div> <div name="BB10" id="response_div_spacer_BBPPID" dir="auto" style="width:100%;"> <br style="display:initial"></div> <div id="blackberry_signature_BBPPID" name="BB10" dir="auto"> <div id="_signaturePlaceholder_BBPPID" name="BB10" dir="auto">Sent from my BlackBerry - the most secure mobile device</div> </div></div><div id="_original_msg_header_BBPPID" dir="auto"> <table width="100%" style="border-spacing: 0px; display: table; outline: none;" contenteditable="false"><tbody><tr><td colspan="2" style="padding: initial; font-size: initial; text-align: initial;"> <div style="border-right: none; border-bottom: none; border-left: none; border-image: initial; border-top: 1pt solid rgb(181, 196, 223); padding: 3pt 0in 0in; font-family: Tahoma, "BB Alpha Sans", "Slate Pro"; font-size: 10pt;"> <div id="from"><b>From:</b> martin.lewis.x84@gmail.com</div><div id="sent"><b>Sent:</b> July 9, 2020 15:14</div><div id="to"><b>To:</b> nietzsche@lysator.liu.se</div><div id="cc"><b>Cc:</b> busybox@busybox.net</div><div id="subject"><b>Subject:</b> Re: [PATCH 3/4] lsscsi: code shrink and refactor</div></div></td></tr></tbody></table> <br> </div><!--start of _originalContent --><div name="BB10" dir="auto" style="background-image: initial; line-height: initial; outline: none;" contenteditable="false"><div dir="ltr">Here's the struct as specified in the kernel:<br>/* From /drivers/scsi/scsi_sysfs.c in the kernel:<br> * sdev_rd_attr (type, "%d\n");<br> * sdev_rd_attr (vendor, "%.8s\n");<br> * sdev_rd_attr (model, "%.16s\n");<br> * sdev_rd_attr (rev, "%.4s\n");<br> */<br>As you can see, type is always printed with %d which should always be a valid signed integer.<br><br>Could you please give an example of a type for which atoi is not sufficient?<br>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.<br><br>Martin<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 3 Jul 2020 at 14:45, Markus Gothe <<a href="mailto:nietzsche@lysator.liu.se">nietzsche@lysator.liu.se</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb( 204 , 204 , 204 );padding-left:1ex">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").<br>
<br>
Please use bb_stroi() for signed integers.<br>
<br>
BR,<br>
Markus<br>
<br>
Sent from my BlackBerry - the most secure mobile device<br>
<br>
<br>
Original Message <br>
<br>
<br>
From: <a href="mailto:martin.lewis.x84@gmail.com">martin.lewis.x84@gmail.com</a><br>
Sent: June 11, 2020 15:45<br>
To: <a href="mailto:busybox@busybox.net">busybox@busybox.net</a><br>
Subject: [PATCH 3/4] lsscsi: code shrink and refactor<br>
<br>
<br>
Remove the use of strou because scsi device types are signed (as seen in the kernel's code).<br>
Improve the representation of SCSI_DEVICE_LIST.<br>
<br>
function old new delta<br>
.rodata 159915 159920 +5<br>
lsscsi_main 364 349 -15<br>
------------------------------------------------------------------------------<br>
(add/remove: 0/0 grow/shrink: 1/1 up/down: 5/-15) Total: -10 bytes<br>
text data bss dec hex filename<br>
981332 <a href="tel:16915">16915</a> 1872 1000119 f42b7 busybox_old<br>
981322 <a href="tel:16915">16915</a> 1872 1000109 f42ad busybox_unstripped<br>
<br>
Signed-off-by: Martin Lewis <<a href="mailto:martin.lewis.x84@gmail.com">martin.lewis.x84@gmail.com</a>><br>
---<br>
miscutils/lsscsi.c | 50 ++++++++++++++++++++++++++++++++++----------------<br>
1 file changed, 34 insertions(+), 16 deletions(-)<br>
<br>
diff --git a/miscutils/lsscsi.c b/miscutils/lsscsi.c<br>
index f737d33d9..e29bedd77 100644<br>
--- a/miscutils/lsscsi.c<br>
+++ b/miscutils/lsscsi.c<br>
@@ -25,6 +25,28 @@<br>
<br>
#include "libbb.h"<br>
<br>
+#define SCSI_DEVICE_TYPE(type, name) type name "\0"<br>
+#define SCSI_DEVICE_TYPE_LIST \<br>
+ SCSI_DEVICE_TYPE("\x00", "disk") \<br>
+ SCSI_DEVICE_TYPE("\x01", "tape") \<br>
+ SCSI_DEVICE_TYPE("\x02", "printer") \<br>
+ SCSI_DEVICE_TYPE("\x03", "process") \<br>
+ SCSI_DEVICE_TYPE("\x04", "worm") \<br>
+ SCSI_DEVICE_TYPE("\x06", "scanner") \<br>
+ SCSI_DEVICE_TYPE("\x07", "optical") \<br>
+ SCSI_DEVICE_TYPE("\x08", "mediumx") \<br>
+ SCSI_DEVICE_TYPE("\x09", "comms") \<br>
+ SCSI_DEVICE_TYPE("\x0c", "storage") \<br>
+ SCSI_DEVICE_TYPE("\x0d", "enclosu") \<br>
+ SCSI_DEVICE_TYPE("\x0e", "sim dsk") \<br>
+ SCSI_DEVICE_TYPE("\x0f", "opti rd") \<br>
+ SCSI_DEVICE_TYPE("\x10", "bridge") \<br>
+ SCSI_DEVICE_TYPE("\x11", "osd") \<br>
+ SCSI_DEVICE_TYPE("\x12", "adi") \<br>
+ SCSI_DEVICE_TYPE("\x1e", "wlun") \<br>
+ SCSI_DEVICE_TYPE("\x1f", "no dev")<br>
+<br>
+<br>
static const char scsi_dir[] ALIGN1 = "/sys/bus/scsi/devices";<br>
<br>
static char *get_line(const char *filename, char *buf, unsigned *bufsize_p)<br>
@@ -59,6 +81,13 @@ int lsscsi_main(int argc UNUSED_PARAM, char **argv UNUSED_PARAM)<br>
<br>
dir = xopendir(".");<br>
while ((de = readdir(dir)) != NULL) {<!-- --><br>
+ /*<br>
+ * From /drivers/scsi/scsi_sysfs.c in the kernel:<br>
+ * sdev_rd_attr (type, "%d\n");<br>
+ * sdev_rd_attr (vendor, "%.8s\n");<br>
+ * sdev_rd_attr (model, "%.16s\n");<br>
+ * sdev_rd_attr (rev, "%.4s\n");<br>
+ */<br>
char buf[256];<br>
char *ptr;<br>
unsigned bufsize;<br>
@@ -67,7 +96,7 @@ int lsscsi_main(int argc UNUSED_PARAM, char **argv UNUSED_PARAM)<br>
const char *type_name;<br>
const char *model;<br>
const char *rev;<br>
- unsigned type;<br>
+ int type;<br>
<br>
if (!isdigit(de->d_name[0]))<br>
continue;<br>
@@ -88,23 +117,12 @@ int lsscsi_main(int argc UNUSED_PARAM, char **argv UNUSED_PARAM)<br>
<br>
printf("[%s]\t", de->d_name);<br>
<br>
-#define scsi_device_types \<br>
- "disk\0" "tape\0" "printer\0" "process\0" \<br>
- "worm\0" "\0" "scanner\0" "optical\0" \<br>
- "mediumx\0" "comms\0" "\0" "\0" \<br>
- "storage\0" "enclosu\0" "sim dsk\0" "opti rd\0" \<br>
- "bridge\0" "osd\0" "adi\0" "\0" \<br>
- "\0" "\0" "\0" "\0" \<br>
- "\0" "\0" "\0" "\0" \<br>
- "\0" "\0" "wlun\0" "no dev"<br>
- type = bb_strtou(type_str, NULL, 10);<br>
- if (errno<br>
- || type >= 0x20<br>
- || (type_name = nth_string(scsi_device_types, type))[0] == '\0'<br>
- ) {<!-- --><br>
+ type = atoi(type_str); /* type_str is "%d\n" so atoi is sufficient */<br>
+ if (type >= 0x20 ||<br>
+ (type_name = memchr(SCSI_DEVICE_TYPE_LIST, type, sizeof(SCSI_DEVICE_TYPE_LIST))) == NULL) {<!-- --><br>
printf("(%s)\t", type_str);<br>
} else {<!-- --><br>
- printf("%s\t", type_name);<br>
+ printf("%s\t", type_name + 1);<br>
}<br>
<br>
printf("%s\t""%s\t""%s\n",<br>
--<br>
2.11.0<br>
<br>
_______________________________________________<br>
busybox mailing list<br>
<a href="mailto:busybox@busybox.net">busybox@busybox.net</a><br>
<a href="http://lists.busybox.net/mailman/listinfo/busybox">http://lists.busybox.net/mailman/listinfo/busybox</a><br>
</blockquote></div>
<!--end of _originalContent --></div></body></html>