Date: Fri, 9 Mar 2012 18:38:35 -0000 From: "Steven Hartland" <killing@multiplay.co.uk> To: "Kenneth D. Merry" <ken@freebsd.org> Cc: freebsd-scsi@freebsd.org, Eygene Ryabinkin <rea@freebsd.org> Subject: Re: Looking for a committer for cam fixes / enhancements Message-ID: <1DD65E0CC5144356A161732587F26266@multiplay.co.uk> References: <02B04E968B8648CC83F274B32090B937@multiplay.co.uk> <20111024171039.GA39194@nargothrond.kdm.org> <7E5239236AA04319B4C090E5F199DC64@multiplay.co.uk> <20111025173148.GA93047@nargothrond.kdm.org>
next in thread | previous in thread | raw e-mail | index | archive | help
----- Original Message -----
From: "Kenneth D. Merry" <ken@freebsd.org>
Sorry its been a quite a while, had no free time to look at this until
now, but I have got a few questions if you would be so kind:-
>> Could you give me some points on the things you spotted weren't "KNR"
>> I've tried to stick with what I saw as the current formatting but clearly
>> missed something's ;-)
>
> To match the rest of the file this:
>
> static int
> atasecurity_set_password(struct cam_device *device, union ccb *ccb, int retry_count,
> u_int32_t timeout, struct ata_security_password *pwd, int quiet)
> {
>
> Should look like this:
> static int
> atasecurity_set_password(struct cam_device *device, union ccb *ccb,
> int retry_count, u_int32_t timeout,
> struct ata_security_password *pwd, int quiet)
> {
>
> And this:
>
> cam_fill_ataio(&ccb->ataio,
> retry_count,
> NULL,
> /*flags*/CAM_DIR_OUT,
> MSG_SIMPLE_Q_TAG,
> /*data_ptr*/(u_int8_t *)pwd,
> /*dxfer_len*/sizeof(struct ata_security_password),
> timeout);
>
> Shoud look like this:
>
> cam_fill_ataio(&ccb->ataio,
> retry_count,
> NULL,
> /*flags*/CAM_DIR_OUT,
> MSG_SIMPLE_Q_TAG,
> /*data_ptr*/(u_int8_t *)pwd,
> /*dxfer_len*/sizeof(struct ata_security_password),
> timeout);
>
>
So are you saying that all wrapped function definitions should have their
wrapped lines indented via tabs (using ts=8) + spaces such that the wrapped
lines align with the opening bracket of the arg list?
If this is the case can you explain why as it means that all formatting
needs to be done manually which seems needless?
> Everything that exceeds 80 columns should not exceed that. Make sure the
> tab stop in your editor is set to 8 when editing code.
Again why in this day and age limit to 80? 132 I could just about understand
but not 80 chars, does anyone really have a screen which can only display
this amount of characters which they use to code on in this day and age
especially given 8 char tab stops?
The file already doesn't comply with this e.g.
ata_bpack(ident_buf->revision, ident_buf->revision, sizeof(ident_buf->revision));
ata_btrim(ident_buf->serial, sizeof(ident_buf->serial));
ata_bpack(ident_buf->serial, ident_buf->serial, sizeof(ident_buf->serial));
In addition how do you deal with cases where a literal string e.g. if a
printf is longer than 80 chars does that have to be split up?
Overall this makes it harder to write compliant code (cant use standard
editor formatting cabailities) as well as creating harder to read code,
which all seems quite needless i.e. instead of:-
error = atasecurity_unlock(device, ccb, retry_count, timeout, &pwd, quiet);
I would have to have:-
error = atasecurity_unlock(device, ccb,
retry_count,
timeout, &pwd,
quiet);
> I'm not suggesting short options, but a slightly different approach.
> Instead of --option, or -r, use -o foopassword=blah.
>
> It might be interesting to do this:
>
> cd src
> find bin sbin usr.sbin usr.bin -name "*.c" -print |xargs grep getopt_long
>
> The list of programs that use getopt_long() is very short, and almost all
> of them are either GNU programs, or maintaining compatibility with GNU
> programs (e.g. tar, cpio)
Sorry I don't mean to be a pain, but while this maybe what other
programs are doing, are they also supporting as many options as
camcontrol?
Does it really make sense to have utils all do their own command line
argument parsing which is required to process -o foopassword=blah, as
suggested, instead of using the existing library designed to do this
job for you?
Seems like trying to stick with something that's no longer suitable,
which will only cause supportability issues, simply because that's
how its been done in the past?
Regards
Steve
================================================
This e.mail is private and confidential between Multiplay (UK) Ltd. and the person or entity to whom it is addressed. In the event of misdirection, the recipient is prohibited from using, copying, printing or otherwise disseminating it or any information contained in it.
In the event of misdirection, illegible or incomplete transmission please telephone +44 845 868 1337
or return the E.mail to postmaster@multiplay.co.uk.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1DD65E0CC5144356A161732587F26266>