-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OS-8136 Add DISCARD/TRIM support to bhyve (illumos specific bits) #270
Conversation
@@ -630,6 +650,10 @@ blockif_open(const char *optstr, const char *ident) | |||
} | |||
} | |||
} | |||
|
|||
if (nodelete == 0 && ioctl(fd, DKIOC_CANFREE, &candelete)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ioctl()
does not return a bool
or boolean_t
, so its return value should be compared to 0 or -1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extant freebsd code uses this style, so I opted for consistency with it (though I agree that being explicit would be better). We don't upstream the stuff in #ifndef __FreeBSD__
blocks AFAIK, so there's no upstream issue though that would cause conflicts there, so I don't feel too strongly either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this isn't the only place where I hold my nose in the bhyve code.
@@ -630,6 +650,10 @@ blockif_open(const char *optstr, const char *ident) | |||
} | |||
} | |||
} | |||
|
|||
if (nodelete == 0 && ioctl(fd, DKIOC_CANFREE, &candelete)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this isn't the only place where I hold my nose in the bhyve code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be sure the smartos-live changes make it into the same biweekly build. Please hold integration of this until the smartos-live changes are ready to land.
No description provided.