On 04/02/2017 08:48 AM, Christian Boltz wrote: > Hello, > > Am Mittwoch, 22. März 2017, 19:24:04 CEST 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> >> >> --- >> Changes since v1: >> - implementation of set_json_mode(), write_json() >> - Changed the way output is provided to keep input the same. This >> would write in either of two formats: text or json, but will keep >> input the same. This helps in keeping localizations in place. I so >> wish UI was a class.. - Removed all yast calls. > > A generic notice first: make check complained about some whitespace > issues. Please always run "make check" in the utils directory before > sending a patch ;-) (even if your changes are probably not covered by > unittests) > > Speaking about tests - I'd still like to see an unittest for the json > interface ;-) (maybe a sample to-be-updated profile, an audit.log sniplet, > the expected JSON, predefined answers and finally the expected profile.) > > > I tried to use aa-logprof --json, but all I get is: > > python3 aa-logprof --json > Traceback (most recent call last): > File "aa-logprof", line 56, in <module> > apparmor.do_logprof_pass(logmark) > File "/home/cb/apparmor/HEAD-clean/utils/apparmor/aa.py", line 1905, in > do_logprof_pass > aaui.UI_Info(_('Reading log entries from %s.') % logfile) > File "/home/cb/apparmor/HEAD-clean/utils/apparmor/ui.py", line 61, in > UI_Info > write_json(jsonout) > File "/home/cb/apparmor/HEAD-clean/utils/apparmor/ui.py", line 43, in > write_json > sys.stdout.write(json.dumps(jsonout, soft_keys=False, separators=(',', ': > ')) + '\n') > File "/usr/lib64/python3.6/json/__init__.py", line 238, in dumps > **kw).encode(obj) > TypeError: __init__() got an unexpected keyword argument 'soft_keys' > > Is that meant to be sort_keys instead of soft_keys? > > After changing that, it finally worked. I get a prompt like (linebreaks > added for readability) > > { > "title": null, > "headers": {"Profil": "/usr/bin/konversation","Pfad": > "/sys/devices/pci0000:00/0000:00:02.0/vendor","Neuer Modus": > "r","Schweregrad": 4}, > "explanation": null, > "options": ["/sys/devices/pci0000:00/0000:00:02.0/vendor > r,","/sys/devices/pci*/*/vendor r,"], > "menu_items": ["Erl(a)uben","[A(b)lehnen]","(I)gnorieren","(G)lob","Glob with > (E)xtension","(N)eu","Audi(t)","Abb(r)echen","En(d)e"] > } > > I'm missing one thing here - the preselected option. > In most cases, the first option is preselected - but after using (G)lob, > Glob with (E)xtension or (N)ew, the newly added option will be > selected, and the new option is in most (not all!) cases the last one. > >> diff --git a/utils/apparmor/ui.py b/utils/apparmor/ui.py >> index bfbde8c..e6fcd1c 100644 >> --- a/utils/apparmor/ui.py >> +++ b/utils/apparmor/ui.py > >> -from apparmor.yasti import yastLog, SendDataToYast, GetDataFromYast > > It seems ui.py was the only user of yastLog(), so you can (and should) > drop it from yasti.py. > > SendDataToYast and GetDataFromYastcheck if you need to change something there. > > From a quick look, the most important affected functions are: > - get_profile() - you'll see this in action with aa-genprof if a profile > exists in /usr/share/apparmor/extra-profiles/ and you select > "view profile" > - yast_select_and_upload_profiles() - dead code because we don't have > a profile repo currently > - save_profiles() - SendDataToYaST is used for "view changes" > > There are some more functions that check for YaST - simply search aa.py > for "Yast" and "yast". > > Note: Ideally, aa.py should know nothing about the UI used, so it might > be a good idea to move this code to ui.py unless that makes things > terribly difficult. > > I'd guess a (to-be-written) UI_view_in_pager() function or extending > UI_LongMessage() to handle text mode (instead of deleting it) could > get the job done ;-) > >> @@ -47,18 +56,17 @@ 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} >> + write_json(jsonout) > > What happens if someone accidently sets UI_mode to "foo"? (Hint: no output, > and you'll have a hard time to find out _why_ there isn't any output.) > I know this is unlikely, but I'd still prefer to be on the safe side. > > What about: > > if UI_mode == 'json': > ... > else: # text mode > ... > > You could also add an else branch that raises an exception("unknown > UI_mode") - but that might be too much ;-) > > This comment applies to several sections, so please adjust this in the > whole file. > >> @@ -138,36 +140,31 @@ def UI_YesNoCancel(text, default): >> ... >> + jsonout = {'dialog': 'yesnocancal', 'text': text, 'default': >> default} > > As Seth already noted in v1, this should be yesnocanc_e_l ;-) > >> @@ -405,7 +388,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') > > Please use write_json() ;-) > > > That all said - v2 looks much better than v1 :-) >
This is getting more complicated than can be handled. Do you think converting UI module to an abstract class, and deriving JSONUI and TextUI will be a better option? Though it would need more work, it would simplify a lot of things, be more flexible to changes and would be more robust than the hackish way it is being performed now. Finally, is it a principle to not "reply to all" on the mailing list? While the mail does reach me, The ML filters move the mails to respctive folders. -- Goldwyn -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor