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

[Stephen Welker]

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.