Hello, Am Dienstag, 28. Februar 2017, 03:39:39 CET schrieb Goldwyn Rodrigues: > From: Goldwyn Rodrigues <rgold...@suse.com> > > This adds JSON support for tools in order to be able to talk to > other utilities such as Yast. > > The json is one per line, in order to differentiate between multiple > records. This is based on work presented by Christian Boltz some time > back. > > Signed-off-by: Goldwyn Rodrigues <rgold...@suse.com>
Let me add some comments in addition to Seth's feedback ;-)
> ---
> utils/aa-genprof | 4 ++
> utils/aa-logprof | 7 ++++
> utils/apparmor/ui.py | 110
> +++++++++++++++++++++++++++------------------------ 3 files changed,
> 70 insertions(+), 51 deletions(-)
>
> diff --git a/utils/aa-genprof b/utils/aa-genprof
> index 3fe72bb..296db1b 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()
>
> +if args.json:
> + aaui.UI_mode = 'json'
See below.
> diff --git a/utils/aa-logprof b/utils/aa-logprof
> index 05ebbd9..711fc62 100755
> --- a/utils/aa-logprof
> +++ b/utils/aa-logprof
> @@ -16,6 +16,7 @@ import argparse
> import os
>
> import apparmor.aa as apparmor
> +import apparmor.ui as aaui
>
> # setup exception handling
> from apparmor.fail import enable_aa_exception_handler
> @@ -29,8 +30,14 @@ 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()
>
> +if args.json:
> + aaui.UI_mode = 'json'
> +else:
> + aaui.UI_mode = 'text'
See below.
> diff --git a/utils/apparmor/ui.py b/utils/apparmor/ui.py
> index bfbde8c..fe8a9c5 100644
> --- a/utils/apparmor/ui.py
> +++ b/utils/apparmor/ui.py
> @@ -26,7 +30,10 @@ _ = init_translation()
> debug_logger = DebugLogger('UI')
>
> # The operating mode: yast or text, text by default
> -UI_mode = 'text'
> +try:
> + UI_mode
> +except NameError:
> + UI_mode = 'text'
[ This is "below" ;-) ]
What about
UI_mode = 'text'
def set_json_mode():
global UI_mode
UI_mode = 'json'
This would also mean that aa-logprof and aa-genprof will need to call
set_json_mode() instead if modifying the UI_mode variable.
I tend to prefer this solution because it reduces the risk of typos in
the variable content and avoids the try/except magic (which IMHO isn't
too different from simply setting UI_mode = 'text' because the global
code is executed on importing a module, before you can set any variable
in it. Did you ever hit a case where the except was _not_ hit? I doubt.)
> @@ -47,18 +54,18 @@ def UI_Info(text):
> debug_logger.info(text)
> if UI_mode == 'text':
> sys.stdout.write(text + '\n')
> - else:
> - yastLog(text)
> + elif UI_mode == 'json':
> + jsonout = {'info': text}
> + sys.stdout.write(json.dumps(jsonout, sort_keys=False,
> separators=(',', ': ')) + '\n')
As Seth already mentioned, you should probably move this into a helper
function which just expects jsonout as parameter.
I'm not sure about flush()ing, but my first try would be a simple print()
;-)
> def UI_GetFile(file):
> - debug_logger.debug('UI_GetFile: %s' % UI_mode)
> + debug_logger.debug('UI_Mode: %s' % UI_mode)
Having the function name in the debug log is probably more helpful, so
please revert this change or change it to "UI_GetFile UI_Mode".
> @@ -300,7 +295,7 @@ class PromptQuestion(object):
> def promptUser(self, params=''):
> cmd = None
> arg = None
> - if UI_mode == 'text':
> + if UI_mode == 'text' or UI_mode == 'json':
This can be simplified to "if True:" ;-) - which means you don't need the
if condition at all.
> cmd, arg = self.Text_PromptUser()
> else:
> self.type = 'wizard'
This 'else:' branch will never be hit, so it's probably a good idea to
remove it (especially if you remove the "if" as recommended above ;-)
> @@ -388,6 +392,7 @@ class PromptQuestion(object):
> while header_copy:
> header = header_copy.pop(0)
> value = header_copy.pop(0)
> + hdict[header] = value
> prompt += formatstr % (header + ':', value)
> prompt += '\n'
It's unlikely, but in theory a header could appear more than once. With
your code, you'll run into a "last one wins" situation, while text mode
will print all.
> @@ -405,7 +410,10 @@ class PromptQuestion(object):
>
> prompt += ' / '.join(menu_items)
>
> - sys.stdout.write(prompt + '\n')
> + if UI_mode == 'json':
> + sys.stdout.write(json.dumps(jsonprompt,
> sort_keys=False, separators=(',', ': ')) + '\n') + elif
> UI_mode == 'text':
A plain "else:" would be better.
That all said - I'm happy to see that you made quite some progress
during HackWeek.
One thing I'd like to see are some tests for json mode. This could
either be some simple tests that "talk" to some of the UI_* functions,
or a full aa-logprof run (with expected JSON and predefined answers,
based on a given profile and log sniplet - and finally comparing the
updated profile to the expected profile [1]). Having such a test would
even ensure that we can't accidently break YaST by changing the JSON
output in a way YaST doesn't expect ;-)
Regards,
Christian Boltz
[1] the test profile should probably avoid using abstractions to make
it less likely that we have to update the test regularly ;-)
--
Let me know when you once find some actual outcome of SUCH a thread...
[Dominique Leuenberger in opensuse-factory]
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
