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

[Goldwyn Rodrigues]

On 04/13/2017 04:52 PM, Christian Boltz wrote:
> 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' ;-)

I think apparmor json version makes sense, though aa-status --json
supplies version as
"version": "1"

Would you prefer to be uniform?

> 
> 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.

Yes, that's right.

> 
> 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.)

I would prefer to update this one. It is best if we have everything
sorted out before we put this in.

> 
> 
>> 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?

Yeah, vim setcindent set as default :(

> 
> 
> 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.)

Hmm, that makes sense. I will update this patch, but it will be quite a
while because I will have to make the other interface as well. But I
think it is best to get things right.

> 
> 
> 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 ;-)
> 

I'd prefer v4.

Thanks for the review.

-- 
Goldwyn