[GSOC 2014][PATCH 3/4] JSON: Add basic support for syscalls in io.c

[yangmin zhu]

Hi Dmitry,
Thank you for your review.

On Fri, Jun 27, 2014 at 10:02 AM, Dmitry V. Levin <ldv at altlinux.org> wrote:
> Hi YangMin,
>
> Thanks for these examples.  Let's have a look at the first one:
>
>> int
>> sys_read(struct tcb *tcp)
>> {
>>       JSON_BEGIN_META_MODE;
>>
>>       if (entering(tcp)) {
>>               JSON_META( printfd(tcp, tcp->u_arg[0]); );
>>               tprints(", ");
>>
>>               json_event(1, EVENT_ARG | SEPARATOR);
>>       } else {
>>               json_type_event first;
>>
>>               if (syserror(tcp)) {
>>                       tprintf("%#lx", tcp->u_arg[1]);
>>                       first = EVENT_ERROR;
>>               }
>>               else {
>>                       JSON_META( printstr(tcp, tcp->u_arg[1], tcp->u_rval) );
>>                       first = EVENT_ARG;
>>               }
>>               tprintf(", %lu", tcp->u_arg[2]);
>>
>>               json_event(2, first | SEPARATOR, EVENT_ARG);
>>       }
>>       return 0;
>> }
>
> Well, compared to the original code, it looks rather complicated.
> I thought it could be something easier to read, for example,
>
> int
> sys_read(struct tcb *tcp)
> {
>         if (entering(tcp)) {
>                 print_args_open();
>                 print_arg_fd(tcp->u_arg[0]);
>         } else {
>                 if (syserror(tcp))
>                         print_arg_fmt("%#lx", tcp->u_arg[1]);
>                 else
>                         print_arg_str_at(tcp->u_arg[1], tcp->u_rval);
>                 print_arg_fmt("%lu", tcp->u_arg[2]);
>                 print_args_close();
>         }
>         return 0;
> }
>
> That is, with all JSON related details hidden from syscall parsers.
> What do you think?
>

1) I had ever thought about to hide all JSON related details from
syscall parsers, such as to use functions like
print_arg_fd(tcp->u_arg[0]);
and I even implemented a simple framework for that. But I changed not
to do it in this way because I want to keep the parser code the same
as the original ones.
so we can just add some additional code to make it support json
output. you can see this in sys_read() above, I didn't delete or
modify any existing code, and I just add some output functions or use
some wrap macro to support JSON output. And other people could still
follow the old way to do the output work and do not care about the
JSON staff.

2) Another reason for my choose is that if we hide the details by
using `print_arg_fmt("%lu", tcp->u_arg[2])` to replace `tprintf(",
%lu", tcp->u_arg[2])`, we will have to implement two different output
logical code, one for the original output and one for the JSON output.
I'm afarid to broke the original format so I prefer to not to modify
it.

3) I also didn't add such specific output functions such as
`print_arg_fd()` for extensibility. Because printfd() is not only used
in such sys_* functions, it also used in decode_select(),
print_dirfd(), printcmsghdr() and so on. If I choose to use spilt all
the current logical code and the format code of the parser functions.
I'm afraid it may need much more time and I may couldn't finish it in
time.

4) I agree the code is indeed very complicated int my current
implementation, how do you think the following update:
int
sys_read(struct tcb *tcp)
{
       if (entering(tcp)) {
              printfd(tcp, tcp->u_arg[0]);
              tprints(", ");
              json_printf("arg sepa");
      } else {
              if (syserror(tcp)) {
                      tprintf("%#lx", tcp->u_arg[1]);
                      json_printf("arg");
              }
              else {
                      printstr(tcp, tcp->u_arg[1], tcp->u_rval)
                      json_printf("arg");
              }
              tprintf(", %lu", tcp->u_arg[2]);
              json_printf("arg sepa");
      }
      return 0;
}
here we can move the macon JSON_BEGIN_META_MODE out of the sys_*
functions, and we move JSON_META into the wrapped functions.
and we use json_printf(const char* format_string); to replace the json_event().

5) Another important question is that I currently simply choose to use
a marco JSON_META to wrap some detail-output functions(such as
printfd(), printstr()). It's a temporary choice because I want to
first wrap all the arguments into a double quoted string, and after
this, I back to modify these detail-output functions to be more JSON
style.

That's my current thoughts there may be wrong or unclear so if you
have any questions please just let me know. and I think more
discussions about how to use the JSON output functions or how the
interface should be is very important for my next work.

Thank you for your review!
---
YangMin