here's a slighly updated version without any whitespace diffs. cheers. alex On Fri Oct 15 10, Alexander Best wrote: > hi there, > > i sent this patch to mav_at_, but he seems rather busy atm. > > maybe somebody else would like to take a look at it and see if it improves > camcontrol's current behavior. > > cheers. > alex > > ----- Forwarded message from Alexander Best <arundel_at_freebsd.org> ----- > > Date: Mon, 27 Sep 2010 00:35:41 +0000 > From: Alexander Best <arundel_at_freebsd.org> > To: Alexander Motin <mav_at_freebsd.org> > Subject: some camcontrol(8) cleanups > > hi there, > > since you're the most active committer to camcontrol i thought i'd mail you > this patch which does some whitespace cleaning up in camcontrol.c along with > some 'camcontrol identify' improvements (imo). > > the only real change is that i removed this check: > > if (parm->satacapabilities && parm->satacapabilities != 0xffff) > > i've run the patched camcontrol against a few devices and none seemed to return > parm->satacapabilities == 0xffff. > so i don't think this check is needed in order to prevent 'camcontrol identify' > to falsely report NCQ supported/enabled. > > cheers. > alex > > -- > a13x > > diff --git a/sbin/camcontrol/camcontrol.c b/sbin/camcontrol/camcontrol.c > index 9f26906..b822282 100644 > --- a/sbin/camcontrol/camcontrol.c > +++ b/sbin/camcontrol/camcontrol.c > _at__at_ -116,7 +116,7 _at__at_ typedef enum { > } cam_argmask; > > struct camcontrol_opts { > - const char *optname; > + const char *optname; > cam_cmdmask cmdnum; > cam_argmask argnum; > const char *subopt; > _at__at_ -204,7 +204,7 _at__at_ static int readdefects(struct cam_device *device, int argc, char **argv, > char *combinedopt, int retry_count, int timeout); > static void modepage(struct cam_device *device, int argc, char **argv, > char *combinedopt, int retry_count, int timeout); > -static int scsicmd(struct cam_device *device, int argc, char **argv, > +static int scsicmd(struct cam_device *device, int argc, char **argv, > char *combinedopt, int retry_count, int timeout); > static int tagcontrol(struct cam_device *device, int argc, char **argv, > char *combinedopt); > _at__at_ -234,7 +234,7 _at__at_ static int atapm(struct cam_device *device, int argc, char **argv, > #endif > > camcontrol_optret > -getoption(char *arg, cam_cmdmask *cmdnum, cam_argmask *argnum, > +getoption(char *arg, cam_cmdmask *cmdnum, cam_argmask *argnum, > const char **subopt) > { > struct camcontrol_opts *opts; > _at__at_ -622,7 +622,7 _at__at_ scsistart(struct cam_device *device, int startstop, int loadeject, > else > fprintf(stdout, > "Error received from stop unit command\n"); > - > + > if (arglist & CAM_ARG_VERBOSE) { > cam_error_print(device, ccb, CAM_ESF_ALL, > CAM_EPF_ALL, stderr); > _at__at_ -688,7 +688,7 _at__at_ scsiinquiry(struct cam_device *device, int retry_count, int timeout) > union ccb *ccb; > struct scsi_inquiry_data *inq_buf; > int error = 0; > - > + > ccb = cam_getccb(device); > > if (ccb == NULL) { > _at__at_ -721,13 +721,13 _at__at_ scsiinquiry(struct cam_device *device, int retry_count, int timeout) > * scsi_inquiry() will convert an inq_len (which is passed in as > * a u_int32_t, but the field in the CDB is only 1 byte) of 256 > * to 0. Evidently, very few devices meet the spec in that > - * regard. Some devices, like many Seagate disks, take the 0 as > + * regard. Some devices, like many Seagate disks, take the 0 as > * 0, and don't return any data. One Pioneer DVD-R drive > * returns more data than the command asked for. > * > * So, since there are numerous devices that just don't work > * right with the full inquiry size, we don't send the full size. > - * > + * > * - The second reason not to use the full inquiry data length is > * that we don't need it here. The only reason we issue a > * standard inquiry is to get the vendor name, device name, > _at__at_ -1181,7 +1181,7 _at__at_ atacapprint(struct ata_params *parm) > } > > printf("\nFeature " > - "Support Enable Value Vendor\n"); > + "Support Enabled Value Vendor\n"); > printf("read ahead %s %s\n", > parm->support.command1 & ATA_SUPPORT_LOOKAHEAD ? "yes" : "no", > parm->enabled.command1 & ATA_SUPPORT_LOOKAHEAD ? "yes" : "no"); > _at__at_ -1201,16 +1201,13 _at__at_ atacapprint(struct ata_params *parm) > ATA_QUEUE_LEN(parm->queue) + 1); > } else > printf("\n"); > - if (parm->satacapabilities && parm->satacapabilities != 0xffff) { > - printf("Native Command Queuing (NCQ) %s ", > - parm->satacapabilities & ATA_SUPPORT_NCQ ? > - "yes" : "no"); > + printf("Native Command Queuing (NCQ) %s", > + parm->satacapabilities & ATA_SUPPORT_NCQ ? "yes" : "no"); > if (parm->satacapabilities & ATA_SUPPORT_NCQ) { > - printf(" %d tags\n", > + printf(" %d tags\n", > ATA_QUEUE_LEN(parm->queue) + 1); > } else > printf("\n"); > - } > printf("SMART %s %s\n", > parm->support.command1 & ATA_SUPPORT_SMART ? "yes" : "no", > parm->enabled.command1 & ATA_SUPPORT_SMART ? "yes" : "no"); > _at__at_ -1223,28 +1220,39 _at__at_ atacapprint(struct ata_params *parm) > printf("power management %s %s\n", > parm->support.command1 & ATA_SUPPORT_POWERMGT ? "yes" : "no", > parm->enabled.command1 & ATA_SUPPORT_POWERMGT ? "yes" : "no"); > - printf("advanced power management %s %s %d/0x%02X\n", > + printf("advanced power management %s %s", > parm->support.command2 & ATA_SUPPORT_APM ? "yes" : "no", > - parm->enabled.command2 & ATA_SUPPORT_APM ? "yes" : "no", > - parm->apm_value, parm->apm_value); > - printf("automatic acoustic management %s %s " > - "%d/0x%02X %d/0x%02X\n", > + parm->enabled.command2 & ATA_SUPPORT_APM ? "yes" : "no"); > + if (parm->support.command2 & ATA_SUPPORT_APM) { > + printf(" %d/0x%02X\n", > + parm->apm_value, parm->apm_value); > + } else > + printf("\n"); > + printf("automatic acoustic management %s %s", > parm->support.command2 & ATA_SUPPORT_AUTOACOUSTIC ? "yes" :"no", > - parm->enabled.command2 & ATA_SUPPORT_AUTOACOUSTIC ? "yes" :"no", > - ATA_ACOUSTIC_CURRENT(parm->acoustic), > - ATA_ACOUSTIC_CURRENT(parm->acoustic), > - ATA_ACOUSTIC_VENDOR(parm->acoustic), > - ATA_ACOUSTIC_VENDOR(parm->acoustic)); > + parm->enabled.command2 & ATA_SUPPORT_AUTOACOUSTIC ? "yes" :"no"); > + if (parm->support.command2 & ATA_SUPPORT_AUTOACOUSTIC) { > + printf(" %d/0x%02X %d/0x%02X\n", > + ATA_ACOUSTIC_CURRENT(parm->acoustic), > + ATA_ACOUSTIC_CURRENT(parm->acoustic), > + ATA_ACOUSTIC_VENDOR(parm->acoustic), > + ATA_ACOUSTIC_VENDOR(parm->acoustic)); > + } else > + printf("\n"); > printf("media status notification %s %s\n", > parm->support.command2 & ATA_SUPPORT_NOTIFY ? "yes" : "no", > parm->enabled.command2 & ATA_SUPPORT_NOTIFY ? "yes" : "no"); > printf("power-up in Standby %s %s\n", > parm->support.command2 & ATA_SUPPORT_STANDBY ? "yes" : "no", > parm->enabled.command2 & ATA_SUPPORT_STANDBY ? "yes" : "no"); > - printf("write-read-verify %s %s %d/0x%x\n", > + printf("write-read-verify %s %s", > parm->support2 & ATA_SUPPORT_WRITEREADVERIFY ? "yes" : "no", > - parm->enabled2 & ATA_SUPPORT_WRITEREADVERIFY ? "yes" : "no", > - parm->wrv_mode, parm->wrv_mode); > + parm->enabled2 & ATA_SUPPORT_WRITEREADVERIFY ? "yes" : "no"); > + if (parm->support2 & ATA_SUPPORT_WRITEREADVERIFY) { > + printf(" %d/0x%x\n", > + parm->wrv_mode, parm->wrv_mode); > + } else > + printf("\n"); > printf("unload %s %s\n", > parm->support.extension & ATA_SUPPORT_UNLOAD ? "yes" : "no", > parm->enabled.extension & ATA_SUPPORT_UNLOAD ? "yes" : "no"); > _at__at_ -1255,7 +1263,6 _at__at_ atacapprint(struct ata_params *parm) > parm->support_dsm & ATA_SUPPORT_DSM_TRIM ? "yes" : "no"); > } > > - > static int > ataidentify(struct cam_device *device, int retry_count, int timeout) > { > _at__at_ -1902,7 +1909,7 _at__at_ readdefects(struct cam_device *device, int argc, char **argv, > > /* > * XXX KDM I should probably clean up the printout format for the > - * disk defects. > + * disk defects. > */ > switch (returned_format & SRDDH10_DLIST_FORMAT_MASK){ > case SRDDH10_PHYSICAL_SECTOR_FORMAT: > _at__at_ -2011,7 +2018,7 _at__at_ void > reassignblocks(struct cam_device *device, u_int32_t *blocks, int num_blocks) > { > union ccb *ccb; > - > + > ccb = cam_getccb(device); > > cam_freeccb(ccb); > _at__at_ -2114,7 +2121,7 _at__at_ mode_select(struct cam_device *device, int save_pages, int retry_count, > err(1, "error sending mode select command"); > else > errx(1, "error sending mode select command"); > - > + > } > > cam_freeccb(ccb); > _at__at_ -2294,7 +2301,7 _at__at_ scsicmd(struct cam_device *device, int argc, char **argv, char *combinedopt, > if (arglist & CAM_ARG_CMD_IN) { > warnx("command must either be " > "read or write, not both"); > - error = 1; > + error = 1; > goto scsicmd_bailout; > } > arglist |= CAM_ARG_CMD_OUT; > _at__at_ -2611,7 +2618,7 _at__at_ camdebug(int argc, char **argv, char *combinedopt) > warnx("bus:target, or bus:target:lun to debug"); > } > } > - > + > if (error == 0) { > > ccb.ccb_h.func_code = XPT_DEBUG; > _at__at_ -2874,7 +2881,7 _at__at_ cts_print(struct cam_device *device, struct ccb_trans_settings *cts) > } > > /* > - * Get a path inquiry CCB for the specified device. > + * Get a path inquiry CCB for the specified device. > */ > static int > get_cpi(struct cam_device *device, struct ccb_pathinq *cpi) > _at__at_ -2913,7 +2920,7 _at__at_ get_cpi_bailout: > } > > /* > - * Get a get device CCB for the specified device. > + * Get a get device CCB for the specified device. > */ > static int > get_cgd(struct cam_device *device, struct ccb_getdev *cgd) > _at__at_ -3764,9 +3771,9 _at__at_ doreport: > fprintf(stdout, > "\rFormatting: %ju.%02u %% " > "(%d/%d) done", > - (uintmax_t)(percentage / > + (uintmax_t)(percentage / > (0x10000 * 100)), > - (unsigned)((percentage / > + (unsigned)((percentage / > 0x10000) % 100), > val, 0x10000); > fflush(stdout); > _at__at_ -3956,7 +3963,7 _at__at_ retry: > case RPL_LUNDATA_ATYP_PERIPH: > if ((lundata->luns[i].lundata[j] & > RPL_LUNDATA_PERIPH_BUS_MASK) != 0) > - fprintf(stdout, "%d:", > + fprintf(stdout, "%d:", > lundata->luns[i].lundata[j] & > RPL_LUNDATA_PERIPH_BUS_MASK); > else if ((j == 0) > _at__at_ -3994,7 +4001,7 _at__at_ retry: > field_len_code = (lundata->luns[i].lundata[j] & > RPL_LUNDATA_EXT_LEN_MASK) >> 4; > field_len = field_len_code * 2; > - > + > if ((eam_code == RPL_LUNDATA_EXT_EAM_WK) > && (field_len_code == 0x00)) { > fprintf(stdout, "%d", > _at__at_ -4352,7 +4359,7 _at__at_ bailout: > > #endif /* MINIMALISTIC */ > > -void > +void > usage(int verbose) > { > fprintf(verbose ? stdout : stderr, > _at__at_ -4494,7 +4501,7 _at__at_ usage(int verbose) > #endif /* MINIMALISTIC */ > } > > -int > +int > main(int argc, char **argv) > { > int c; > _at__at_ -4544,7 +4551,7 _at__at_ main(int argc, char **argv) > * this. getopt is kinda braindead, so you end up having to run > * through the options twice, and give each invocation of getopt > * the option string for the other invocation. > - * > + * > * You would think that you could just have two groups of options. > * The first group would get parsed by the first invocation of > * getopt, and the second group would get parsed by the second > _at__at_ -4553,13 +4560,13 _at__at_ main(int argc, char **argv) > * to the argument _after_ the first argument in the second group. > * So when the second invocation of getopt comes around, it doesn't > * recognize the first argument it gets and then bails out. > - * > + * > * A nice alternative would be to have a flag for getopt that says > * "just keep parsing arguments even when you encounter an unknown > * argument", but there isn't one. So there's no real clean way to > * easily parse two sets of arguments without having one invocation > * of getopt know about the other. > - * > + * > * Without this hack, the first invocation of getopt would work as > * long as the generic arguments are first, but the second invocation > * (in the subfunction) would fail in one of two ways. In the case > _at__at_ -4573,14 +4580,14 _at__at_ main(int argc, char **argv) > * whether optind had been incremented one option too far. The > * mechanics of that, however, are more daunting than just giving > * both invocations all of the expect options for either invocation. > - * > + * > * Needless to say, I wouldn't mind if someone invented a better > * (non-GPL!) command line parsing interface than getopt. I > * wouldn't mind if someone added more knobs to getopt to make it > * work better. Who knows, I may talk myself into doing it someday, > * if the standards weenies let me. As it is, it just leads to > * hackery like this and causes people to avoid it in some cases. > - * > + * > * KDM, September 8th, 1998 > */ > if (subopt != NULL) > _at__at_ -4607,7 +4614,7 _at__at_ main(int argc, char **argv) > > /* > * First catch people who try to do things like: > - * camcontrol tur /dev/da0 > + * camcontrol tur /dev/da0 > * camcontrol doesn't take device nodes as arguments. > */ > if (argv[2][0] == '/') { > > > ----- End forwarded message ----- > > -- > a13x -- a13x
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:08 UTC