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 ----- -- a13xReceived on Fri Oct 15 2010 - 18:47:49 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:08 UTC