[apparmor] [PATCH 2/2] json support for tools (logprof and genprof)

Christian Boltz apparmor at cboltz.de
Thu Apr 13 21:52:59 UTC 2017


Hello,

Am Donnerstag, 13. April 2017, 05:55:54 CEST schrieb Goldwyn Rodrigues:
> From: Goldwyn Rodrigues <rgoldwyn at 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 <rgoldwyn at 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.
> 
> Changes since v2:
>  - use if UI_mode == json else, to make sure some output is returned
> in case of faulty UI_mode.
>  - spelling correction yesnocancal
>  - Added default_key
>  - added dialog entry in all communication, to identify the kind of
> json output. 

We just discussed this a bit on IRC. 'dialog' is a very good idea and 
can be helpful to ensure we don't break anything.

I'd even say we should never change the parameters of existing 'dialog' 
types. If changes are needed, we should create a new 'dialog' type 
instead. (I'm speaking about released versions here - changes for 
dialogs only in bzr are not that problematic.)


Also, it would be helpful to print out the json scheme version at the 
beginning. This can help clients to break early (at startup, if the json 
version is too new for them) instead of breaking later when receiving a 
new ("unknown") 'dialog' type.

Just as an idea, set_json_mode() could do

    jsonout = {'dialog': 'apparmor-json-version', 'data': '2.12'}
    write_json(jsonout)

'2.12' obviously matches the next AppArmor release. It will only be 
changed if the json interface changes, so if it turns out your patch is 
good enough for the next 50 years, it will stay at '2.12' forever ;-)

If you think a simple number is easier to handle - I'm also fine with 
version '1' ;-)

IMHO the client (YaST) should error out if the version is newer than 
what it supports - that's better than breaking somewhere in the middle 
of an aa-logprof run.

BTW: Adding a summary of the above as comment to ui.py would be a good 
idea.


You can do this in a follow-up patch - it wouldn't be the first time we 
see a 3/2 patch ;-)
(If you prefer to update this patch, please tell me.)


> diff --git a/utils/apparmor/ui.py b/utils/apparmor/ui.py
> index f25fff3..c4462b0 100644
> --- a/utils/apparmor/ui.py
> +++ b/utils/apparmor/ui.py

> @@ -44,12 +54,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

Minor nitpicking: python coding styles expect _two_ spaces between code 
and comments, so this line should be

+    else:  # text mode

(Applies to several comments in your patch.)

> @@ -324,6 +348,17 @@ class PromptQuestion(object):
>          function_regexp += ')$'
> 
>          ans = 'XXXINVALIDXXX'
> +        hdict = dict()
> +        jsonprompt = {
> +            'dialog': 'promptuser',
> +            'title': title,
> +            'headers': hdict,
> +            'explanation': explanation,
> +            'options': options,
> +            'menu_items': menu_items,
> +	    'default_key': default_key,

The last line has a whitespace issue - tab instead of spaces?


BTW: Don't forget to let YaST send the selected option in this dialog, 
and also make sure it works if more than 9 options are available.
Hint: It might be a good idea to provide the response as json {'option': 
<selected option>, 'menu_item': <pressed button>} in this case.
(Again, this could be a follow-up patch.)


The attached file contains your patch with the whitespace issues fixed
(no other changes).

If you promise to send a follow-up patch that adds the json version info 
(and the comments about interface stability), I'm ready to give this 
patch (with the whitespace fixes as listed above) my
    Acked-by: Christian Boltz <apparmor at cboltz.de>

If you prefer to send a v4 of this patch, tell me ;-)


Note: The patch is based on some code I wrote, so an additional review 
(Steve? Seth?) would be nice ;-)


Regards,

Christian Boltz
-- 
We need to make sure that the direction we're going and the decisions
we're taking are made by people who will suffer the consequences of
them. [Henne Vogelsang in opensuse-project]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: aa-tools-add-json-support-v3a.diff
Type: text/x-patch
Size: 12953 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20170413/4b7fa640/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20170413/4b7fa640/attachment.pgp>


More information about the AppArmor mailing list