<div dir="ltr"><div>Definitely a bug. And the correct solution is definitely to check the return value of sysctlbyname() and only check errno if it returned < 0. Pre-clearing errno is always a bad idea because most programmers assume that it is a cumulative error (that is, as error conditions propagate upward code is often run that makes additional system calls, and you don't want the contents of errno to suddenly become 0 during that).<br><br><br></div><div>This one is really easy to fix, I'll just go ahead and do it now that you have pointed out the locations of the bad code. No reason to ask you to go through all the hand-waving of a bug report.<br></div><div><br></div>-Matt<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jun 8, 2016 at 7:24 PM, Stephen Welker <span dir="ltr"><<a href="mailto:stephen.welker@nemostar.com.au" target="_blank">stephen.welker@nemostar.com.au</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I have discovered several potential coding issues in several file system utilities related to SSD/TRIM. These have been discovered whilst I have been debugging SSD/TRIM issues on boot disks (see other email on this subject).<br>
<br>
They all relate to calls to sysctlbyname(3) and the lack of checking the return value of the function combined with unconditional checking of "errno".<br>
<br>
My understanding is that you should only check "errno" iff the function call indicates that an error has occurred.<br>
<br>
/sbin/swapon/swapon.c<br>
<br>
372 sysctlbyname(sysctl_name, &trim_enabled, &olen, NULL, 0);<br>
373 if (errno == ENOENT) {<br>
374 if (qflag == 0) {<br>
375 printf("TRIM not supported on %s, "<br>
376 "ignoring\n",<br>
377 name);<br>
378 }<br>
379 errno = 0;<br>
380 } else if (!trim_enabled) {<br>
381 if (qflag == 0) {<br>
382 printf("TRIM not enabled on %s (%s), "<br>
383 "ignoring\n",<br>
384 name, sysctl_name);<br>
385 }<br>
386 } else {<br>
387 trim_volume(name);<br>
388 }<br>
<br>
Interestingly, in the above source code file, other invocations of sysctlbyname() have the return value checked.<br>
<br>
<br>
/sbin/mount_ufs/mount_ufs.c<br>
<br>
108 sysctlbyname(sysctl_name, &trim_enabled, &olen, NULL, 0);<br>
109 if(errno == ENOENT) {<br>
110 printf("Device:%s does not support the TRIM command\n",<br>
111 args.fspec);<br>
112 ufs_usage();<br>
113 }<br>
114 if(!trim_enabled) {<br>
115 printf("Online TRIM selected, but sysctl (%s) "<br>
116 "is not enabled\n",sysctl_name);<br>
117 ufs_usage();<br>
118 }<br>
<br>
<br>
/sbin/fdisk/fdisk.c<br>
<br>
786 sysctlbyname(sysctl_name, &trim_enabled, &olen, NULL, 0);<br>
787 if (errno == ENOENT) {<br>
788 printf("Device:%s does not support the TRIM command\n", disk);<br>
789 usage();<br>
790 }<br>
791 if (!trim_enabled) {<br>
792 printf("Erase device option selected, but sysctl (%s) "<br>
793 "is not enabled\n",sysctl_name);<br>
794 usage();<br>
795 }<br>
<br>
<br>
/sbin/newfs/newfs.c<br>
<br>
435 sysctlbyname(sysctl_name, &trim_enabled, &olen, NULL, 0);<br>
436<br>
437 if(errno == ENOENT) {<br>
438 printf("Device:%s does not support the TRIM command\n",<br>
439 special);<br>
440 usage();<br>
441 }<br>
442 if(!trim_enabled) {<br>
443 printf("Erase device option selected, but sysctl (%s) "<br>
444 "is not enabled\n",sysctl_name);<br>
445 usage();<br>
446<br>
447 }<br>
<br>
<br>
Interestingly, in the following code, "errno" is explicitly set to zero to work around the problem.<br>
<br>
/sbin/newfs_hammer/newfs_hammer.c<br>
<br>
221 errno=0;<br>
222 sysctlbyname(sysctl_name, &trim_enabled, &olen, NULL, 0);<br>
223 if(errno == ENOENT) {<br>
224 printf("%s %s (%s) does not support the TRIM "<br>
225 "command\n",<br>
226 vol->type, vol->name, sysctl_name);<br>
227 usage();<br>
228 }<br>
229 if(!trim_enabled) {<br>
230 printf("Erase device option selected, but "<br>
231 "sysctl (%s) is not enabled\n", sysctl_name);<br>
232 usage();<br>
233<br>
234 }<br>
235 trim_volume(vol);<br>
<br>
<br>
PS: there may be other instances of this issue that I have not discovered.<br>
<br>
I am not sure which coding method is best (DragonFly BSD realm):<br>
<br>
1. Explicitly set "errno" (is this thread/SMP save?), or<br>
2. Code an explicit test of the return value.<br>
<br>
I have not filed a bug report (or reports) yet. I will take the advice of this list first.<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
regards,<br>
Stephen Welker.<br>
<br>
</font></span></blockquote></div><br></div>