On Sat, Jul 23, 2011 at 7:43 PM, Tim Bisson <biss...@mac.com> wrote:
> Hi,
>
> Here are the trim patches. I tried to break them up into functional units so
> that a review will hopefully be easier.
>
> I also added support for fdisk, so you can trim a whole device with -I, or
> just a partition using -u.
>
Functionally this looks pretty good to me, pretty simple and
straightforward -- I just have a few notes, mostly whitespace and etc.
related, which I think probably says something (read: it looks pretty
solid to me).
Missing spaces:
sys/bus/cam/scsi/scsi_da.c: bp->b_bio1.bio_offset =bytes_start;
sys/bus/cam/scsi/scsi_da.c: if(softc->disk.d_info.d_trimflag) {
sys/bus/cam/scsi/scsi_da.c: if(!softc->trim_running &&
and more following this...
In dastart:
+ do {
+ uint64_t lba;
+ int count;
... this is probably fine, but I think we generally try to avoid this
and declare all variables at the top of the function, but for infinite
loops we definitely prefer while (1) {} or for (;;) {} instead of do
{} while(1);
in scsi_da.h:
+//#define IOCATADELETE _IOW('a', 104, off_t[2])
+
Should be removed?
-----
In hammer diff,
+ printf("Erase device option selected, but
sysctl (%s) "
+ "is not enabled\n",sysctl_name);
Space after ,
Ioctl can return EIO or EINVAL at least, check for an error return in
the hammer commands trim_volume()?
-----
swap patch,
missing space:
+static int swap_on_off(char *name, int doingall,int trim);
+ if(!trim_enabled) {
-----
ufs patch (7) has inconsistent spacing (tabs vs spaces).
-----
fdisk patch, check ioctl return?
Best,
Sam
