----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/670/#review845 -----------------------------------------------------------
openlmi-mof-register http://reviewboard-openlmi.rhcloud.com/r/670/#comment453
Bash has native print option to print current time: printf "%(%Y-%m-%d)T" -1
openlmi-mof-register http://reviewboard-openlmi.rhcloud.com/r/670/#comment450
log "COMMAND:" "$@" looks more natural for me. Using this, you will pass as 1st argument the type and other arguments will be command (with parameters) what you like.
openlmi-mof-register http://reviewboard-openlmi.rhcloud.com/r/670/#comment451
Using eval is very error prone; the command injection is very easy to do, we must be careful.
We must also take care that parameters doesn't have any shell functionality.
To be honest, I don't like it.
openlmi-mof-register http://reviewboard-openlmi.rhcloud.com/r/670/#comment452
Also split to log "CODE:" "$?"
openlmi-mof-register http://reviewboard-openlmi.rhcloud.com/r/670/#comment454
This will not work. You are checking return value from log_command, it's return value is return value from last executed command, which is log, in turn it is printf.
openlmi-mof-register http://reviewboard-openlmi.rhcloud.com/r/670/#comment455
log STARTING "$0" "$@"
openlmi-providers.spec http://reviewboard-openlmi.rhcloud.com/r/670/#comment456
You can encapsulate the all commands to {} and redirect once. Something like: if ... { } >>... fi
or do the redirection for the if command: if... fi >>...
It's simpler, readable and less error prone.
- Roman Rakus
On Aug. 1, 2013, 1:19 p.m., Radek Novacek wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/670/
(Updated Aug. 1, 2013, 1:19 p.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Add logging to the openlmi-mof-register script
Each command in the registration script is now logged into /var/log/openlmi-install.log file.
Diffs
openlmi-mof-register 4e60e31786e22b063e4c3a42116e05038f5275c3 openlmi-providers.spec 0552a8cc01c8c3979a2304463f0a663fcc993a95
Diff: http://reviewboard-openlmi.rhcloud.com/r/670/diff/
Testing
Thanks,
Radek Novacek
_______________________________________________ openlmi-reviews mailing list openlmi-reviews@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/openlmi-reviews