[installer] Fix variable inside machine.conf caused install.sh error #6600
Conversation
* When the value contains space in machine.conf and does not have double quote, "install.sh" can't correctly read it. * "install.sh" uses dot command(i.e. source) to apply the settings in machine.conf. If the value in machine.conf are separated by space without double quotes between them, it will return failure when install.sh try to source machine.conf. ``` machine.conf example: ... onie_disco_ntpsrv=10.254.141.10 10.254.141.131 ... install.sh returns: ... /tmp/tmp.A7wHuAMSQV/installer/install.sh: 11: /host/machine.conf: 10.254.141.131: not found ``` * Add a new function 'read_conf_file' to read the settings from machine.conf when the value in machine.conf are separated by space without double quotes between them.
* Originally, the script will failed if the line of machine.conf is comment line or blank line. After this patch, it can correctly handle these type of content.
installer/x86_64/install.sh
Outdated
| local conf_file=$1 | ||
| while IFS='=' read -r var value | ||
| do | ||
| f_char=$(echo $var | cut -c1) |
There was a problem hiding this comment.
Do you assume the # appear as first char of a line? If yes, can you assume it could appear anywhere on a line? #Closed
There was a problem hiding this comment.
Your suggestion is helpful. I modify this part to remove the string behind #.
| tmp_val=${value#\"} | ||
| # remove double quote in the end | ||
| value=${tmp_val%\"} | ||
| eval "$var=\"$value\"" |
There was a problem hiding this comment.
Is it possible to use declare instead of eval ? #Closed
There was a problem hiding this comment.
I am not familiar with declare. What is the advantage to use declare instead of eval?
There was a problem hiding this comment.
declare is safer than eval. I know it in bash, not sure about this environment (sh).
In reply to: 568218112 [](ancestors = 568218112)
There was a problem hiding this comment.
I tried it in sh environment. There is no declare command.
Handle following types of line * Whole comment line * Comment string behind declaration * Comment string behind some spaces * Blank line
| @@ -20,6 +20,25 @@ _trap_push() { | |||
| } | |||
| _trap_push true | |||
|
|
|||
| read_conf_file() { | |||
There was a problem hiding this comment.
read_conf_file [](start = 0, length = 14)
Could you write a unit test to cover old cases and your new feature? #Closed
There was a problem hiding this comment.
Sure. But what kind of the test will be better and suitable?
Add a test inside sonic-mgmt and be independent from building. Or, add additional process to test this during building.
I haven't found similar case of this type of unit test.
There was a problem hiding this comment.
read_conf_file() is complex. I expect you have tested it locally. Could you just automate the test in another shell script or a python test runner.
In future we may either improve the code or the testcases.
In reply to: 568342160 [](ancestors = 568342160)
There was a problem hiding this comment.
I just copied the function to another script and test it with a sample machine.conf.
The test can only be triggered manually for now.
* To run this test, `cd` into the tests directory and `./test_read_conf.sh` If pass, it will return 0 and show "PASS". If fail, it will return 1 and show "[ERR] Expect value(x86_64) is not equal to input value()".
kuanyu99
force-pushed
the
ky_fix-installer
branch
from
08ab846
to
ec36f85
Compare
- Why I did it
Encounter error when install SONiC image if there are some onie_discovery variables assigned with multiple values inside machine.conf
- How I did it
Replace original ". /machine.conf" method and add another function to do the same thing.
- How to verify it
onie_disco_ntpsrv=10.254.141.10 10.254.141.131sonic_installer installto check if any error occurs- Which release branch to backport (provide reason below if selected)
- Description for the changelog
Add a new function 'read_conf_file' to read the settings from machine.conf when the value in machine.conf are separated by space without double quotes between them.
- A picture of a cute animal (not mandatory but encouraged)