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

[Dmitry V. L. <ld...@al...>]

Hi YangMin,

On Sun, Jun 29, 2014 at 09:23:12PM +0800, yangmin zhu wrote:
[...]
> 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.

Yes, these are valid points, but I think in the long run we will have to
rewrite parsers in a more structured manner anyway.

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

Yes, this way it looks less complicated.

In this example, since va_list processing is easier and more efficient
than string parsing, I'd recommend
	json_printf(JSON_ARG, JSON_SEP, 0L);
instead of
	json_printf("arg sepa");

Also, when a function name ends with "printf", it is expected to take a
printf-style format string.  If it doesn't, give it a different name.


-- 
ldv