Hi Dmitry, Thank you for your review. On Fri, Jun 27, 2014 at 10:02 AM, Dmitry V. Levin <l...@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 ------------------------------------------------------------------------------ Open source business process management suite built on Java and Eclipse Turn processes into business applications with Bonita BPM Community Edition Quickly connect people, data, and systems into organized workflows Winner of BOSSIE, CODIE, OW2 and Gartner awards http://p.sf.net/sfu/Bonitasoft _______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel