| Summary: | sys-apps/pciutils - pciparm init script | ||
|---|---|---|---|
| Product: | Gentoo Linux | Reporter: | Federico Ferri (RETIRED) <mescalinum> |
| Component: | New packages | Assignee: | Gentoo's Team for Core System packages <base-system> |
| Status: | RESOLVED FIXED | ||
| Severity: | enhancement | CC: | slong |
| Priority: | Lowest | ||
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | Linux | ||
| URL: | http://gentoo.org/doc/en/articles/hardware-stability-p2.xml#doc_chap3_pre2 | ||
| Whiteboard: | |||
| Package list: | Runtime testing required: | --- | |
| Attachments: |
/etc/init.d/pciparm
/etc/conf.d/pciparm /etc/init.d/pciparm diff (proposed fix) /etc/init.d/pciparm-r1 /etc/conf.d/pciparm-r1 /etc/init.d/pciparm-r2 /etc/conf.d/pciparm-r2 pci-parm.patch init.d-pciparm.patch |
||
|
Description
Federico Ferri (RETIRED)
2007-04-04 13:17:57 UTC
Created attachment 115443 [details]
/etc/init.d/pciparm
Created attachment 115444 [details]
/etc/conf.d/pciparm
You cannot use bashism in init.d scripts. (In reply to comment #3) > You cannot use bashism in init.d scripts. could you be more precise? if you refer to the FLAT_ID="${pci_id//[:.]/_}", it is a plain substitution with globbing, used also in other init scripts (netmount, net.lo, localmount, ...) parameter name evaluation (that is: ${!VAR_THAT_HOLDS_IT}) also is used in other init scripts maybe I don't get what bashism means....? > could you be more precise?
>
> if you refer to the FLAT_ID="${pci_id//[:.]/_}", it is a plain substitution
> with globbing, used also in other init scripts (netmount, net.lo, localmount,
> ...)
>
> parameter name evaluation (that is: ${!VAR_THAT_HOLDS_IT}) also is used in
> other init scripts
>
> maybe I don't get what bashism means....?
>
No you got it; now fix it ;) Basically you have to write portable shell in init scripts, unlike ebuilds. #shells on irc.freenode.org i understand is a good place to ask.
(In reply to comment #4) > (In reply to comment #3) > > You cannot use bashism in init.d scripts. > > could you be more precise? > > if you refer to the FLAT_ID="${pci_id//[:.]/_}", it is a plain substitution > with globbing, used also in other init scripts (netmount, net.lo, localmount, > ...) You'll have to change that to sed :) > parameter name evaluation (that is: ${!VAR_THAT_HOLDS_IT}) also is used in > other init scripts And you could write eval foo=\$VAR_THAT_HOLDS_IT > maybe I don't get what bashism means....? It means don't use features specific to bash. baselayout-2 will use /bin/sh as the shell to run init scripts with, which although will default to bash on Linux, could be dash or busybox. Created attachment 115475 [details, diff] /etc/init.d/pciparm diff (proposed fix) okay I got it. Let me say that the concept of `shell portability' is a bit vague: to make things work for zsh is one thing, but make them work for tcsh is another business =) I used to refer to http://www.gnu.org/software/autoconf/manual/autoconf-2.57/html_node/autoconf_114.html some tips could go in the devmanual (attached diff with fixes) this is too specific ... it should be a "set pci options for pci devices", not "set latency timings for pci devices" (In reply to comment #8) > this is too specific ... it should be a "set pci options for pci devices", I I concur. The script doesn't look bash-specific to me. Question is: does it work? If so, maybe it'll go in after the 2007.0 release has bedded in.. ie at least a month or two after release. Other params would be useful in the meantime. (In reply to comment #8) > this is too specific ... it should be a "set pci options for pci devices", not > "set latency timings for pci devices" OK I've done generalized version of this script (going to upload it, naming it -r1). I'm sorry previous script wasn't working cause eval foo=\$VAR_THAT_HOLDS_IT doesn't work. this time I used ${!USE_VAR_FROM_MY_VALUE} wich is also prettier also conf.d syntax has changed to something more sane: PCIPARM_ALL="latency_timer=b0" PCIPARM_01_08_0="latency_timer=ff" PCIPARM_00_04_0="latency_timer=ff" PCIPARM_01_04_0="latency_timer=ff" VERBOSE="no" you can specify any pci register you want Created attachment 116247 [details]
/etc/init.d/pciparm-r1
generalized version. tested.
Created attachment 116248 [details]
/etc/conf.d/pciparm-r1
config for new generalized version. removed ugly TWEAK_CARDS variable
Are there any other changes needed for this to be included? The syntax looks cool to me FWIW. (In reply to comment #13) > Are there any other changes needed for this to be included? The syntax looks > cool to me FWIW. it's the script currently I am using on my systems. I'm fine with it (no corrections since 2 months! should be enough :)) that script is better, but still restricts things to the bus view without allowing vendor:device as well at this point, it might be easier to just have a syntax like: PCI_OPTS_BUS=" [[[[<domain>]:]<bus>]:][<slot>][.[<func>]] <options> " PCI_OPTS_VENDOR=" [<vendor>]:[<device>] <options> " (In reply to comment #15) sorry for the delay. I'll be busy with exams til next week... > at this point, it might be easier to just have a syntax like: > PCI_OPTS_BUS=" > [[[[<domain>]:]<bus>]:][<slot>][.[<func>]] <options> > " > > PCI_OPTS_VENDOR=" > [<vendor>]:[<device>] <options> > " how do you mean that syntax working? perhaps not being parsed by a shell interpreter (like bash) but instead parsed with sed/awk? what I mean is: that way you can specify just one option. maybe you mean to write config as: PCI_OPTS_BUS[0]="..." PCI_OPTS_BUS[1]="..." PCI_OPTS_VENDOR[0]="..." PCI_OPTS_VENDOR[1]="..." ? Created attachment 124859 [details]
/etc/init.d/pciparm-r2
this one implements the bus/vendor specification.
syntax is the following:
PCIPARM_BUS_#="[[[[<domain>]:]<bus>]:][<slot>][.[<func>]] <options>"
PCIPARM_VENDOR_#="[<vendor>]:[<device>] <options>"
it will follow an updated example of /etc/conf.d/pciparm
Created attachment 124861 [details]
/etc/conf.d/pciparm-r2
I updated the script. The same conf syntax it is used by /etc/init.d/lm_sensor, so I decided to borrow its code, not introducing anything new. let me know .... This construct is horrible:
VENDOR_OPT=`eval echo '$'PCIPARM_VENDOR_${SEQ_VENDOR}`
Here's a MUCH better one:
v="PCIPARM_VENDOR_${SEQ_VENDOR}"
VENDOR_OPT="${!v}"
Please fix.
Also, could you please either fix or document that the variables must be sequentially numbered from zero?
Your examples should also all be commented out.
the problem with the latter is you'll get people complaining about non-POSIX incvs (In reply to comment #20) > This construct is horrible: > VENDOR_OPT=`eval echo '$'PCIPARM_VENDOR_${SEQ_VENDOR}` > > Here's a MUCH better one: > v="PCIPARM_VENDOR_${SEQ_VENDOR}" > VENDOR_OPT="${!v}" > What Roy gave in comment #6 is the correct way to do this (given that we don't have bash): eval foo=\$VAR_THAT_HOLDS_IT so: eval VENDOR_OPT=\$PCIPARM_VENDOR_$SEQ_VENDOR You're right the above is horrible, and forks a subshell for no purpose. Created attachment 167638 [details, diff]
pci-parm.patch
Here's a patch, should be a bit faster, perhaps, and more extensible. (Didn't bother error checking array names as passed in from literal in script.) Bit more error-checking elsewhere (not sure if eerror is right, you get the gist Xaero;)
SteveL: I got the gist of your changes, but in future, please patch against what's the newest side, in this case what I had in the tree had already diverged significantly from the submission here on the bug. Created attachment 167962 [details, diff] init.d-pciparm.patch Whoops my bad, robbat; just got curious about the code what with the discussion on sh arrays. Here's a patch against cvs; there were some POSIX incompatibilities, all use of [[; bash as sh doesn't run in true POSIX mode by default. == is another no-no unless it's in $((. Also [[ -z "$@" ]] might work in bash but it definitely won't work in sh if we change it to [, as "$@" expands to multiple parameters. The best ref is: http://www.opengroup.org/onlinepubs/009695399/utilities/contents.html (chapter 2 for syntax, but knowing the utility options is obviously very useful too.) While I was there I added back in the stuff to make the array loop a bit faster (it is an initscript after all.) Using 'foo' as opposed to "foo" is in the same vein. I didn't change the calls to do_setpci_array back to an && sequence but I left the code in place to deal with error (need to uncomment: # || return 1 on l. 63) My thinking was to bail as quickly as possible if there were an error, but I can see why a user might want the ones which can be set, to be done (eg for a stability fix.) Regards, igli. > While I was there I added back in the stuff to make the array loop a bit faster No, your stuff is worse now. Readability trumps fast. If you'd hoisted the case statement entirely out to the do_setpci_array, and removed it from do_setpci I might have believed you, but you didn't. What's with your crazy style of ;; on the next line, with no pattern in whitespace? It should be on the end of branch of the case statement, on the same line if it's one line, or if it's multiple, all indented. > if [ "$VERBOSE" = yes ]; then Why do you keep adding this? It was moved to making the user add -v in the conf.d, and you're introducing a useless variable now AND clobbering any SETPCI_OPTS they might have. Also, the changes you made don't work at all. do_setpci_array needs the start of the variable name as the first argument, not the switch. It looks like you blindly ported the patch without any testing or actually understanding the code. Regardless, some of the changes are there. (In reply to comment #27) > > While I was there I added back in the stuff to make the array loop a bit faster > No, your stuff is worse now. Readability trumps fast. If you'd hoisted the > case statement entirely out to the do_setpci_array, and removed it from > do_setpci I might have believed you, but you didn't. > Eh? What on Earth does belief have to do with anything? It either works or it doesn't. > What's with your crazy style of ;; on the next line, with no pattern in > whitespace? It should be on the end of branch of the case statement, on the > same line if it's one line, or if it's multiple, all indented. > Says who? It's simply easier to maintain (and I _do_ know what I'm talking about in that regard, believe it or not;) I'm not sure what you mean by "no pattern in whitespace" unless you mean you'd like an extra space here or there. > > if [ "$VERBOSE" = yes ]; then > Why do you keep adding this? It was moved to making the user add -v in the > conf.d, and you're introducing a useless variable now AND clobbering any > SETPCI_OPTS they might have. > Ah well thanks for noting that. > Also, the changes you made don't work at all. do_setpci_array needs the start > of the variable name as the first argument, not the switch. It looks like you > blindly ported the patch without any testing or actually understanding the > code. > Yes that's exactly what I did for several reasons: 1) I don't use this. 2) I have no interest in it. I was just curious code-wise, as noted above and in my earlier comment. 3) You'd have to test it and understand what it was doing in any event. Based on your comments above, you have done, or at least more than before. I was just re-integrating stuff that I thought useful into a throwaway patch on bugzilla, and explaining why. > Regardless, some of the changes are there. > Great. Glad I could be of assistance. You'll forgive me for not wanting to do a cvs browse to check it atm, I'm knackered and I really don't give an aerial intercourse about it. |
