Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v2.2.11 - questionable split() call in "common/conf.py" #714

Closed
u6006 opened this issue May 14, 2017 · 5 comments
Closed

v2.2.11 - questionable split() call in "common/conf.py" #714

u6006 opened this issue May 14, 2017 · 5 comments
Milestone

Comments

@u6006
Copy link

u6006 commented May 14, 2017

  • Waagent version (cloned on 05/14/2017)
    grep "^AGENT_VERSION = " common/version.py
    AGENT_VERSION = '2.2.11'

  • Is there any special consideration for the following split() call at line Bugfix runscript for Gentoo Linux #43? or else it cannot handle spaces in the value 'k=" v"' or 'k="v0 v1"' (in common/conf.py)
    41 for line in content.split('\n'):
    42 if not line.startswith("#") and "=" in line:
    43 parts = line.split()[0].split('=')
    44 value = parts[1].strip("" ")

@brendandixon
Copy link
Contributor

@u6006 I'm not seeing what you're seeing. Could you provide a concrete example that fails? Admittedly, this is very simple parsing code. It is not intended to be, as with most Unix-style configuration files, a general purpose, able to handle all kinds of strings, parser. It is intended to handle only the most basic cases. If you can provide a legitimate failure, we'll gladly fix it.

@u6006
Copy link
Author

u6006 commented May 15, 2017

the code on line 43 splits the input line by spaces first, so chars after the 1st space was filtered out by "line.split()[0]...", as such, the two examples posted above will fail to be parsed

k=" v" will get empty value
k="v0 v1" will get 'v0' as the value (it can be space separated words)

If it's intentional, it's not a bug (but the comment should be added to clarify), or else it should be refactored to cover the cases that I listed.

@brendandixon brendandixon added this to the v2.2.12 milestone May 15, 2017
@brendandixon
Copy link
Contributor

@u6006 Got it. The latter case is not a bug as spaces are not allowed in all valid values. However, the first case should be addressed. I believe the best change is to remove the initial split from line #43.

@u6006
Copy link
Author

u6006 commented May 15, 2017

Agree. Removing the 1st split() will handle both my cases though the latter is not supported.

@brendandixon
Copy link
Contributor

Fixed by #729.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants