Hello, the patch looks good in general. I noticed some very small issues which should be easy to fix:
Am Dienstag, 6. Juni 2017, 17:35:44 CEST schrieb Goldwyn Rodrigues:
> From: Goldwyn Rodrigues <rgold...@suse.com>
>
> Provides json support to tools in order to interact with other
> utilities such as Yast.
>
> The JSON output is one per line, in order to differentiate between
> multiple records. Each JSON record has a "dialog" entry which defines
> the type of message passed. A response must contain the "dialog"
> entry. "info" message does not require a response.
>
> "apparmor-json-version" added in order to identify the communication
> protocol version for future updates.
>
> This is based on work done by Christian Boltz.
>
> Signed-off-by: Goldwyn Rodrigues <rgold...@suse.com>
...
> diff --git a/utils/aa-genprof b/utils/aa-genprof
> index e2e65442..9a8783d5 100755
> --- a/utils/aa-genprof
> +++ b/utils/aa-genprof
> @@ -61,8 +61,12 @@ parser =
> argparse.ArgumentParser(description=_('Generate profile for the given
> p parser.add_argument('-d', '--dir', type=str, help=_('path to
> profiles')) parser.add_argument('-f', '--file', type=str,
> help=_('path to logfile')) parser.add_argument('program', type=str,
> help=_('name of program to profile')) +parser.add_argument('-j',
> '--json', action="store_true", help=_('provide output in json
> format')) args = parser.parse_args()
[sorry for the funny KMail quoting behaviour]
Now that the input also has to be in json, the help text doesn't look
100% correct ;-)
> diff --git a/utils/aa-logprof b/utils/aa-logprof
> index c05cbef3..a00cfeac 100755
> --- a/utils/aa-logprof
> +++ b/utils/aa-logprof
...
> @@ -29,8 +30,12 @@ parser =
> argparse.ArgumentParser(description=_('Process log entries to
> generate parser.add_argument('-d', '--dir', type=str, help=_('path to
> profiles')) parser.add_argument('-f', '--file', type=str,
> help=_('path to logfile')) parser.add_argument('-m', '--mark',
> type=str, help=_('mark in the log to start processing after'))
> +parser.add_argument('-j', '--json', action='store_true',
> help=_('provide the output in json format')) args =
> parser.parse_args()
Same help text here.
> diff --git a/utils/apparmor/ui.py b/utils/apparmor/ui.py
> index f25fff31..b227056d 100644
> --- a/utils/apparmor/ui.py
> +++ b/utils/apparmor/ui.py
...
> +def json_response(dialog_type):
> + string = raw_input('\n')
> + rh = json.loads(string.strip())
> + if rh["dialog"] != dialog_type:
> + raise AppArmorException('Expected response %s got %s.' %
> (dialog_type, rh["dialog"]))
> + return rh
I'd include the full json string in the exception - if something goes
wrong, having the full information is always helpful.
> @@ -44,12 +66,18 @@ def getkey():
>
> def UI_Info(text):
> debug_logger.info(text)
> - if UI_mode == 'text':
> + if UI_mode == 'json':
> + jsonout = {'dialog': 'info', 'data': text}
> + write_json(jsonout)
> + else: # text mode
Please use _two_ spaces in front of the #
(that's python coding style, and while it sounds funny at the beginning,
it really makes reading the code a bit easier)
> sys.stdout.write(text + '\n')
>
> def UI_Important(text):
> debug_logger.debug(text)
> - if UI_mode == 'text':
> + if UI_mode == 'json':
> + jsonout = {'dialog': 'important', 'data': text}
> + write_json(jsonout)
> + else: # text mode:
Another place that wants one more space ;-)
> @@ -67,14 +95,18 @@ def get_translated_hotkey(translated, cmsg=''):
> def UI_YesNo(text, default):
> debug_logger.debug('UI_YesNo: %s: %s %s' % (UI_mode, text,
> default)) default = default.lower()
> - ans = None
> - if UI_mode == 'text':
> - yes = CMDS['CMD_YES']
> - no = CMDS['CMD_NO']
> - yeskey = get_translated_hotkey(yes).lower()
> - nokey = get_translated_hotkey(no).lower()
> - ans = 'XXXINVALIDXXX'
> - while ans not in ['y', 'n']:
> + yes = CMDS['CMD_YES']
> + no = CMDS['CMD_NO']
> + yeskey = get_translated_hotkey(yes).lower()
> + nokey = get_translated_hotkey(no).lower()
> + ans = 'XXXINVALIDXXX'
> + while ans not in ['y', 'n']:
> + if UI_mode == 'json':
> + jsonout = {'dialog': 'yesno', 'text': text, 'default':
> default} + write_json(jsonout)
> + hm = json_response('yesno')
> + ans = hm['response_key']
> + else: # text mode:
One more space, please.
> @@ -102,18 +134,22 @@ def UI_YesNo(text, default):
> def UI_YesNoCancel(text, default):
> debug_logger.debug('UI_YesNoCancel: %s: %s %s' % (UI_mode, text,
> default)) default = default.lower()
> - ans = None
> - if UI_mode == 'text':
> - yes = CMDS['CMD_YES']
> - no = CMDS['CMD_NO']
> - cancel = CMDS['CMD_CANCEL']
> -
> - yeskey = get_translated_hotkey(yes).lower()
> - nokey = get_translated_hotkey(no).lower()
> - cancelkey = get_translated_hotkey(cancel).lower()
> -
> - ans = 'XXXINVALIDXXX'
> - while ans not in ['c', 'n', 'y']:
> + yes = CMDS['CMD_YES']
> + no = CMDS['CMD_NO']
> + cancel = CMDS['CMD_CANCEL']
> +
> + yeskey = get_translated_hotkey(yes).lower()
> + nokey = get_translated_hotkey(no).lower()
> + cancelkey = get_translated_hotkey(cancel).lower()
> +
> + ans = 'XXXINVALIDXXX'
> + while ans not in ['c', 'n', 'y']:
> + if UI_mode == 'json':
> + jsonout = {'dialog': 'yesnocancel', 'text': text,
> 'default': default} + write_json(jsonout)
> + hm = json_response('yesnocancel')
> + ans = hm['response_key']
> + else: # text mode:
One more space please ;-)
> @@ -148,7 +184,11 @@ def UI_YesNoCancel(text, default):
> def UI_GetString(text, default):
> debug_logger.debug('UI_GetString: %s: %s %s' % (UI_mode, text,
> default)) string = default
> - if UI_mode == 'text':
> + if UI_mode == 'json':
> + jsonout = {'dialog': 'getstring', 'text': text, 'default':
> default} + write_json(jsonout)
> + string = json_response('getstring')["response"]
> + else: # text mode:
And another space ;-)
> @@ -161,15 +201,18 @@ def UI_GetString(text, default):
> def UI_GetFile(file):
> debug_logger.debug('UI_GetFile: %s' % UI_mode)
> filename = None
> - if UI_mode == 'text':
> + if UI_mode == 'json':
> + jsonout = {'dialog': 'getfile', 'text': file['description']}
> + write_json(jsonout)
> + filename = json_response('getfile')["response"]
> + else: # text mode:
One more space please ;-)
> @@ -352,7 +406,12 @@ class PromptQuestion(object):
>
> prompt += ' / '.join(menu_items)
>
> - sys.stdout.write(prompt + '\n')
> + if UI_mode == 'json':
> + write_json(jsonprompt)
> + hm = json_response('promptuser')
> + return keys[hm["response_key"]], hm["selected"]
This "return" is bypassing the validation if answer and selected option
contain valid values. Please make sure this gets checked - otherwise
invalid answers or non-existing options might lead to funny bugs
elsewhere.
> + else: # text mode:
Last request for an additional space ;-)
As already mentioned a while ago, I won't object if you also add some
tests - but this can be done in a follow-up patch.
BTW: We branched off the 2.11 maintenance branch some weeks ago, so I
finally commited your "Remove yast from utils" patch to bzr trunk.
Regards,
Christian Boltz
--
/* Oopsie, we appear to be missing an EOL marker. If we
* were *smart*, we could work around it. Since we're
* obviously not smart, we'll just punt with a more
* sensible error. */
[from apparmor parser_yacc.y]
signature.asc
Description: This is a digitally signed message part.
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
