Coding issue in some filesystem utilities in relation to SSD/TRIM

[Matthew Dillon]

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).


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.

-Matt

On Wed, Jun 8, 2016 at 7:24 PM, Stephen Welker <
stephen.welker at nemostar.com.au> wrote:

> 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).
>
> 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".
>
> My understanding is that you should only check "errno" iff the function
> call indicates that an error has occurred.
>
> /sbin/swapon/swapon.c
>
>  372         sysctlbyname(sysctl_name, &trim_enabled, &olen, NULL, 0);
>  373         if (errno == ENOENT) {
>  374                 if (qflag == 0) {
>  375                         printf("TRIM not supported on %s, "
>  376                                "ignoring\n",
>  377                                name);
>  378                 }
>  379                 errno = 0;
>  380          } else if (!trim_enabled) {
>  381                 if (qflag == 0) {
>  382                         printf("TRIM not enabled on %s (%s), "
>  383                                "ignoring\n",
>  384                                name, sysctl_name);
>  385                 }
>  386          } else {
>  387                 trim_volume(name);
>  388          }
>
> Interestingly, in the above source code file, other invocations of
> sysctlbyname() have the return value checked.
>
>
> /sbin/mount_ufs/mount_ufs.c
>
>  108         sysctlbyname(sysctl_name, &trim_enabled, &olen, NULL, 0);
>  109         if(errno == ENOENT) {
>  110                 printf("Device:%s does not support the TRIM
> command\n",
>  111                     args.fspec);
>  112                 ufs_usage();
>  113         }
>  114         if(!trim_enabled) {
>  115                 printf("Online TRIM selected, but sysctl (%s) "
>  116                     "is not enabled\n",sysctl_name);
>  117                 ufs_usage();
>  118         }
>
>
> /sbin/fdisk/fdisk.c
>
>  786         sysctlbyname(sysctl_name, &trim_enabled, &olen, NULL, 0);
>  787         if (errno == ENOENT) {
>  788                 printf("Device:%s does not support the TRIM
> command\n", disk);
>  789                 usage();
>  790         }
>  791         if (!trim_enabled) {
>  792                 printf("Erase device option selected, but sysctl (%s)
> "
>  793                     "is not enabled\n",sysctl_name);
>  794                 usage();
>  795         }
>
>
> /sbin/newfs/newfs.c
>
>  435         sysctlbyname(sysctl_name, &trim_enabled, &olen, NULL, 0);
>  436
>  437         if(errno == ENOENT) {
>  438                 printf("Device:%s does not support the TRIM
> command\n",
>  439                     special);
>  440                 usage();
>  441         }
>  442         if(!trim_enabled) {
>  443                 printf("Erase device option selected, but sysctl (%s)
> "
>  444                     "is not enabled\n",sysctl_name);
>  445                 usage();
>  446
>  447         }
>
>
> Interestingly, in the following code, "errno" is explicitly set to zero to
> work around the problem.
>
> /sbin/newfs_hammer/newfs_hammer.c
>
>  221         errno=0;
>  222         sysctlbyname(sysctl_name, &trim_enabled, &olen, NULL, 0);
>  223         if(errno == ENOENT) {
>  224                   printf("%s %s (%s) does not support the TRIM "
>  225                       "command\n",
>  226                       vol->type, vol->name, sysctl_name);
>  227                   usage();
>  228         }
>  229         if(!trim_enabled) {
>  230                   printf("Erase device option selected, but "
>  231                       "sysctl (%s) is not enabled\n", sysctl_name);
>  232                   usage();
>  233
>  234         }
>  235         trim_volume(vol);
>
>
> PS: there may be other instances of this issue that I have not discovered.
>
> I am not sure which coding method is best (DragonFly BSD realm):
>
> 1. Explicitly set "errno" (is this thread/SMP save?), or
> 2. Code an explicit test of the return value.
>
> I have not filed a bug report (or reports) yet. I will take the advice of
> this list first.
>
> --
> regards,
> Stephen Welker.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.dragonflybsd.org/pipermail/users/attachments/20160608/5d278631/attachment-0003.html>