Coding issue in some filesystem utilities in relation to SSD/TRIM
Matthew Dillon
dillon at backplane.com
Wed Jun 8 20:12:53 PDT 2016
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-0002.html>
More information about the Users
mailing list