| Message ID | 56D9CA6002000078000D9935@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2016-03-04 at 09:48 -0700, Jan Beulich wrote: > This is pretty simplistic for now, but I'd rather have someone better > friends with the tools improve it (if desired). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx > return 0; > } > > +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest, > + int *lower_thresh, int *upper_thresh) > +{ > + int ret; > As per libxl coding style, this wants to be 'r'. > + GC_INIT(ctx); > I don't seem to find it in CODING_STYLE, but I'd say there should be an empty line here. > + if (set) { > + ret = xc_set_log_level(ctx->xch, guest, *lower_thresh, > *upper_thresh); > + } else { > + ret = xc_get_log_level(ctx->xch, guest, lower_thresh, > upper_thresh); > + } > + if ( ret < 0 ) { > + LOGE(ERROR, "%s %slog level", > + set ? "setting" : "getting", guest ? "guest " : ""); > + GC_FREE; > + return ERROR_FAIL; > Libxl wants only one error/cleanup path out of the function, and recommends using a variable called rc for hosting the libxl error code to be returned, and goto, if necessary. > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > +int main_loglvl(int argc, char **argv) > +{ > + static const struct option opts[] = { > + {"guest", 0, 0, 'g'}, > + {"set", 0, 0, 's'}, > + COMMON_LONG_OPTS > + }; > + int opt, lower_thresh = -1, upper_thresh = -1; > + bool guest = false, set = false; > + > + SWITCH_FOREACH_OPT(opt, "gs:", opts, "loglvl", 0) { > + case 'g': > + guest = true; > + break; > + case 's': > + if (*optarg != '/') > + lower_thresh = parse_loglvl(&optarg); > + if (*optarg == '/') { > + ++optarg; > + upper_thresh = parse_loglvl(&optarg); > + } > + set = true; > + break; > + } > + > + if (libxl_log_level(ctx, set, guest, &lower_thresh, > &upper_thresh)) { > + fprintf(stderr, "cannot %s %s log level\n", > + set ? "set" : "get", guest ? "guest" : "host"); > + return 1; > This is indeed super-inconsistent in xl. But we're trying to improve it (it's half done and there are patches) and using EXIT_FAILURE and EXIT_SUCCESS for program exit codes, and this return can be classified as such. > + } > + > + if (!set) > + printf("%s log levels: %s/%s\n", guest ? "guest" : "host", > + format_loglvl(lower_thresh), > format_loglvl(upper_thresh)); > + > + return 0; > And this as well, of course. :-) Thanks and Regards, Dario
On Fri, 2016-03-04 at 09:48 -0700, Jan Beulich wrote: > This is pretty simplistic for now, but I'd rather have someone better > friends with the tools improve it (if desired). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx > return 0; > } > > +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest, > + int *lower_thresh, int *upper_thresh) > Another thing I feel like saying is that having two actual functions, one for _get and one for _set (perhaps implemented just as wrappers around this one, which then would become an internal function) is, from my point of view, a better interface, e.g., it is more in line with how things are in libxl. But this, I know, is mostly personal taste, and it's the taste of the tools maintainers that counts. :-) Regards, Dario
>>> On 04.03.16 at 19:45, <dario.faggioli@citrix.com> wrote: > On Fri, 2016-03-04 at 09:48 -0700, Jan Beulich wrote: >> This is pretty simplistic for now, but I'd rather have someone better >> friends with the tools improve it (if desired). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx >> return 0; >> } >> >> +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest, >> + int *lower_thresh, int *upper_thresh) >> +{ >> + int ret; >> > As per libxl coding style, this wants to be 'r'. This and everything else below look to be valid comments, but it's rather frustrating that simply cloning an existing function (I user the debug key ones as basis) doesn't give me valid code, the more that I did scroll up and down a few pages to see whether I just happened to pick a particularly bad example. (This adds to the reasons why I've continue to push out getting the tool stack side done for a patch the hypervisor side of which has been done a couple of months back.) Jan >> + GC_INIT(ctx); >> > I don't seem to find it in CODING_STYLE, but I'd say there should be an > empty line here. > >> + if (set) { >> + ret = xc_set_log_level(ctx->xch, guest, *lower_thresh, >> *upper_thresh); >> + } else { >> + ret = xc_get_log_level(ctx->xch, guest, lower_thresh, >> upper_thresh); >> + } >> + if ( ret < 0 ) { >> + LOGE(ERROR, "%s %slog level", >> + set ? "setting" : "getting", guest ? "guest " : ""); >> + GC_FREE; >> + return ERROR_FAIL; >> > Libxl wants only one error/cleanup path out of the function, and > recommends using a variable called rc for hosting the libxl error code > to be returned, and goto, if necessary. > >> --- a/tools/libxl/xl_cmdimpl.c >> +++ b/tools/libxl/xl_cmdimpl.c > >> +int main_loglvl(int argc, char **argv) >> +{ >> + static const struct option opts[] = { >> + {"guest", 0, 0, 'g'}, >> + {"set", 0, 0, 's'}, >> + COMMON_LONG_OPTS >> + }; >> + int opt, lower_thresh = -1, upper_thresh = -1; >> + bool guest = false, set = false; >> + >> + SWITCH_FOREACH_OPT(opt, "gs:", opts, "loglvl", 0) { >> + case 'g': >> + guest = true; >> + break; >> + case 's': >> + if (*optarg != '/') >> + lower_thresh = parse_loglvl(&optarg); >> + if (*optarg == '/') { >> + ++optarg; >> + upper_thresh = parse_loglvl(&optarg); >> + } >> + set = true; >> + break; >> + } >> + >> + if (libxl_log_level(ctx, set, guest, &lower_thresh, >> &upper_thresh)) { >> + fprintf(stderr, "cannot %s %s log level\n", >> + set ? "set" : "get", guest ? "guest" : "host"); >> + return 1; >> > This is indeed super-inconsistent in xl. But we're trying to improve it > (it's half done and there are patches) and using EXIT_FAILURE and > EXIT_SUCCESS for program exit codes, and this return can be classified > as such. > >> + } >> + >> + if (!set) >> + printf("%s log levels: %s/%s\n", guest ? "guest" : "host", >> + format_loglvl(lower_thresh), >> format_loglvl(upper_thresh)); >> + >> + return 0; >> > And this as well, of course. :-) > > Thanks and Regards, > Dario > -- > <<This happens because I choose it to happen!>> (Raistlin Majere) > ----------------------------------------------------------------- > Dario Faggioli, Ph.D, http://about.me/dario.faggioli > Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
Il 04/03/2016 17:48, Jan Beulich ha scritto: > This is pretty simplistic for now, but I'd rather have someone better > friends with the tools improve it (if desired). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx > return 0; > } > > +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest, > + int *lower_thresh, int *upper_thresh) > +{ > + int ret; > + GC_INIT(ctx); > + if (set) { > + ret = xc_set_log_level(ctx->xch, guest, *lower_thresh, *upper_thresh); > + } else { > + ret = xc_get_log_level(ctx->xch, guest, lower_thresh, upper_thresh); > + } > + if ( ret < 0 ) { > + LOGE(ERROR, "%s %slog level", > + set ? "setting" : "getting", guest ? "guest " : ""); > + GC_FREE; > + return ERROR_FAIL; > + } > + GC_FREE; > + return 0; > +} > + > libxl_xen_console_reader * > libxl_xen_console_read_start(libxl_ctx *ctx, int clear) > { > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -1765,6 +1765,8 @@ int libxl_send_trigger(libxl_ctx *ctx, u > libxl_trigger trigger, uint32_t vcpuid); > int libxl_send_sysrq(libxl_ctx *ctx, uint32_t domid, char sysrq); > int libxl_send_debug_keys(libxl_ctx *ctx, char *keys); > +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest, > + int *lower_thresh, int *upper_thresh); > > typedef struct libxl__xen_console_reader libxl_xen_console_reader; > > --- a/tools/libxl/xl.h > +++ b/tools/libxl/xl.h > @@ -81,6 +81,7 @@ int main_trigger(int argc, char **argv); > int main_sysrq(int argc, char **argv); > int main_debug_keys(int argc, char **argv); > int main_dmesg(int argc, char **argv); > +int main_loglvl(int argc, char **argv); > int main_top(int argc, char **argv); > int main_networkattach(int argc, char **argv); > int main_networklist(int argc, char **argv); > @@ -209,6 +210,8 @@ extern void printf_info_sexp(int domid, > #define XL_GLOBAL_CONFIG XEN_CONFIG_DIR "/xl.conf" > #define XL_LOCK_FILE XEN_LOCK_DIR "/xl" > > +#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0])) > + > #endif /* XL_H */ > > /* > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -6469,6 +6469,84 @@ int main_debug_keys(int argc, char **arg > return 0; > } > > +static const struct { > + int level; > + char string[8]; > +} loglvls[] = { > + { 0, "none" }, > + { 1, "error" }, > + { 2, "warning" }, > + { 3, "info" }, > + { 4, "all" }, > + { 4, "debug" }, > +}; double "4" for both all and debug seems strange to me, is it right? > + > +static int parse_loglvl(char **parg) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(loglvls); ++i) { > + size_t l = strlen(loglvls[i].string); > + > + if (!strncmp(*parg, loglvls[i].string, l)) { > + *parg += l; > + return loglvls[i].level; > + } > + } > + > + return -1; > +} > + > +static const char *format_loglvl(int loglvl) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(loglvls); ++i) { > + if (loglvl == loglvls[i].level) > + return loglvls[i].string; > + } > + > + return "<unknown>"; > +} > + > +int main_loglvl(int argc, char **argv) > +{ > + static const struct option opts[] = { > + {"guest", 0, 0, 'g'}, > + {"set", 0, 0, 's'}, > + COMMON_LONG_OPTS > + }; > + int opt, lower_thresh = -1, upper_thresh = -1; > + bool guest = false, set = false; > + > + SWITCH_FOREACH_OPT(opt, "gs:", opts, "loglvl", 0) { > + case 'g': > + guest = true; > + break; > + case 's': > + if (*optarg != '/') > + lower_thresh = parse_loglvl(&optarg); > + if (*optarg == '/') { > + ++optarg; > + upper_thresh = parse_loglvl(&optarg); > + } > + set = true; > + break; > + } > + > + if (libxl_log_level(ctx, set, guest, &lower_thresh, &upper_thresh)) { > + fprintf(stderr, "cannot %s %s log level\n", > + set ? "set" : "get", guest ? "guest" : "host"); > + return 1; > + } > + > + if (!set) > + printf("%s log levels: %s/%s\n", guest ? "guest" : "host", > + format_loglvl(lower_thresh), format_loglvl(upper_thresh)); > + > + return 0; > +} > + > int main_dmesg(int argc, char **argv) > { > unsigned int clear = 0; > --- a/tools/libxl/xl_cmdtable.c > +++ b/tools/libxl/xl_cmdtable.c > @@ -309,6 +309,13 @@ struct cmd_spec cmd_table[] = { > "[-c]", > " -c Clear dmesg buffer as well as printing it", > }, > + { "loglvl", > + &main_loglvl, 0, 1, > + "Manage Xen log levels", > + "[-g] [-s=[LOWER][/UPPER]]", > + "-g, --guest act on guest log level\n" > + "-s [LOWER][/UPPER], --set=[LOWER][/UPPER] set new log level\n" > + }, > { "top", > &main_top, 0, 0, > "Monitor a host and the domains in real time", > > >
>>> On 07.03.16 at 14:20, <fabio.fantoni@m2r.biz> wrote: > Il 04/03/2016 17:48, Jan Beulich ha scritto: >> --- a/tools/libxl/xl_cmdimpl.c >> +++ b/tools/libxl/xl_cmdimpl.c >> @@ -6469,6 +6469,84 @@ int main_debug_keys(int argc, char **arg >> return 0; >> } >> >> +static const struct { >> + int level; >> + char string[8]; >> +} loglvls[] = { >> + { 0, "none" }, >> + { 1, "error" }, >> + { 2, "warning" }, >> + { 3, "info" }, >> + { 4, "all" }, >> + { 4, "debug" }, >> +}; > > double "4" for both all and debug seems strange to me, is it right? Yes, it is both right and intentional. Jan
On Mon, 2016-03-07 at 04:46 -0700, Jan Beulich wrote: > > > > On 04.03.16 at 19:45, <dario.faggioli@citrix.com> wrote: > > On Fri, 2016-03-04 at 09:48 -0700, Jan Beulich wrote: > > > --- a/tools/libxl/libxl.c > > > +++ b/tools/libxl/libxl.c > > > @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx > > > return 0; > > > } > > > > > > +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest, > > > + int *lower_thresh, int *upper_thresh) > > > +{ > > > + int ret; > > > > > As per libxl coding style, this wants to be 'r'. > This and everything else below look to be valid comments, but > it's rather frustrating that simply cloning an existing function (I > user the debug key ones as basis) doesn't give me valid code, > the more that I did scroll up and down a few pages to see > whether I just happened to pick a particularly bad example. > Hehe, but do you understand that, saying this, you're making it very likely that people will ask *you* to fix libxl_send_debug_keys() --and perhaps more tool side code? :-P :-P No, jokes apart, I agree that inconsistency is a real bad thing... but it's an hard fight, and we do have examples spread all around the source code (both Xen and tools), AFAICT. I run into the patch, decided to have a look, and thought I better say what I found, with the aim of fighting exactly that (inconsistency in the code). If there is anything else I can do for help, feel free to ask (e.g., I guess I can send a patch to fix style of libxl_send_debug_keys() myself :-)). Regards, Dario
>>> On 07.03.16 at 19:07, <dario.faggioli@citrix.com> wrote: > On Mon, 2016-03-07 at 04:46 -0700, Jan Beulich wrote: >> > > > On 04.03.16 at 19:45, <dario.faggioli@citrix.com> wrote: >> > On Fri, 2016-03-04 at 09:48 -0700, Jan Beulich wrote: > >> > > --- a/tools/libxl/libxl.c >> > > +++ b/tools/libxl/libxl.c >> > > @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx >> > > return 0; >> > > } >> > > >> > > +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest, >> > > + int *lower_thresh, int *upper_thresh) >> > > +{ >> > > + int ret; >> > > >> > As per libxl coding style, this wants to be 'r'. >> This and everything else below look to be valid comments, but >> it's rather frustrating that simply cloning an existing function (I >> user the debug key ones as basis) doesn't give me valid code, >> the more that I did scroll up and down a few pages to see >> whether I just happened to pick a particularly bad example. >> > Hehe, but do you understand that, saying this, you're making it very > likely that people will ask *you* to fix libxl_send_debug_keys() --and > perhaps more tool side code? :-P :-P Except that it's not just that function - as said, I did scroll up and down, without finding (style wise) better examples. And no, I'm not going to put together patches to deal with style issues in the tools. > No, jokes apart, I agree that inconsistency is a real bad thing... but > it's an hard fight, and we do have examples spread all around the > source code (both Xen and tools), AFAICT. Right, and asking people myself to not follow bad examples when adding new code, I did take all of your input to adjust the patch. Just that in this case the set of bad examples is so large that in a similar case in the hypervisor I probably wouldn't have dared to ask for a style correction. Jan
On Tue, Mar 8, 2016 at 8:08 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 07.03.16 at 19:07, <dario.faggioli@citrix.com> wrote: >> On Mon, 2016-03-07 at 04:46 -0700, Jan Beulich wrote: >>> > > > On 04.03.16 at 19:45, <dario.faggioli@citrix.com> wrote: >>> > On Fri, 2016-03-04 at 09:48 -0700, Jan Beulich wrote: >> >>> > > --- a/tools/libxl/libxl.c >>> > > +++ b/tools/libxl/libxl.c >>> > > @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx >>> > > return 0; >>> > > } >>> > > >>> > > +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest, >>> > > + int *lower_thresh, int *upper_thresh) >>> > > +{ >>> > > + int ret; >>> > > >>> > As per libxl coding style, this wants to be 'r'. >>> This and everything else below look to be valid comments, but >>> it's rather frustrating that simply cloning an existing function (I >>> user the debug key ones as basis) doesn't give me valid code, >>> the more that I did scroll up and down a few pages to see >>> whether I just happened to pick a particularly bad example. >>> >> Hehe, but do you understand that, saying this, you're making it very >> likely that people will ask *you* to fix libxl_send_debug_keys() --and >> perhaps more tool side code? :-P :-P > > Except that it's not just that function - as said, I did scroll up and > down, without finding (style wise) better examples. And no, I'm > not going to put together patches to deal with style issues in the > tools. > >> No, jokes apart, I agree that inconsistency is a real bad thing... but >> it's an hard fight, and we do have examples spread all around the >> source code (both Xen and tools), AFAICT. > > Right, and asking people myself to not follow bad examples when > adding new code, I did take all of your input to adjust the patch. > Just that in this case the set of bad examples is so large that in a > similar case in the hypervisor I probably wouldn't have dared to > ask for a style correction. Well the approach of the libxl maintainers seems to have be, "Just make sure the new code adheres to the new style, and eventyally everything will be up-to-date". A few releases ago I submitted a patch where I added a new clause in the middle of a series of other very similar clauses, and I was asked to make the new clause follow the new style, but to leave the other clauses right next to it in the old style (to avoid unnecessary code churn, since it was during the development freeze). Given that the "new" style has been around for a while now, it probably would be good to set aside some time at the beginning of the next development cycle to fix things up -- it is incredibly frustrating to carefully try to copy the surrounding style, only to be told to revise it. -George
On Tue, Mar 08, 2016 at 02:05:01PM +0000, George Dunlap wrote: > On Tue, Mar 8, 2016 at 8:08 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>>> On 07.03.16 at 19:07, <dario.faggioli@citrix.com> wrote: > >> On Mon, 2016-03-07 at 04:46 -0700, Jan Beulich wrote: > >>> > > > On 04.03.16 at 19:45, <dario.faggioli@citrix.com> wrote: > >>> > On Fri, 2016-03-04 at 09:48 -0700, Jan Beulich wrote: > >> > >>> > > --- a/tools/libxl/libxl.c > >>> > > +++ b/tools/libxl/libxl.c > >>> > > @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx > >>> > > return 0; > >>> > > } > >>> > > > >>> > > +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest, > >>> > > + int *lower_thresh, int *upper_thresh) > >>> > > +{ > >>> > > + int ret; > >>> > > > >>> > As per libxl coding style, this wants to be 'r'. > >>> This and everything else below look to be valid comments, but > >>> it's rather frustrating that simply cloning an existing function (I > >>> user the debug key ones as basis) doesn't give me valid code, > >>> the more that I did scroll up and down a few pages to see > >>> whether I just happened to pick a particularly bad example. > >>> > >> Hehe, but do you understand that, saying this, you're making it very > >> likely that people will ask *you* to fix libxl_send_debug_keys() --and > >> perhaps more tool side code? :-P :-P > > > > Except that it's not just that function - as said, I did scroll up and > > down, without finding (style wise) better examples. And no, I'm > > not going to put together patches to deal with style issues in the > > tools. > > > >> No, jokes apart, I agree that inconsistency is a real bad thing... but > >> it's an hard fight, and we do have examples spread all around the > >> source code (both Xen and tools), AFAICT. > > > > Right, and asking people myself to not follow bad examples when > > adding new code, I did take all of your input to adjust the patch. > > Just that in this case the set of bad examples is so large that in a > > similar case in the hypervisor I probably wouldn't have dared to > > ask for a style correction. > > Well the approach of the libxl maintainers seems to have be, "Just > make sure the new code adheres to the new style, and eventyally > everything will be up-to-date". A few releases ago I submitted a > patch where I added a new clause in the middle of a series of other > very similar clauses, and I was asked to make the new clause follow > the new style, but to leave the other clauses right next to it in the > old style (to avoid unnecessary code churn, since it was during the > development freeze). > > Given that the "new" style has been around for a while now, it > probably would be good to set aside some time at the beginning of the > next development cycle to fix things up -- it is incredibly > frustrating to carefully try to copy the surrounding style, only to be > told to revise it. > I did fix a few hundred instances of inconsistency at the beginning of 4.7 cycle. Spatch is helpful, but it is far from perfect. What I'm afraid of is that fixing them would introduce too much noise that renders file line annotation useless. Wei. > -George
On Fri, Mar 04, 2016 at 09:48:16AM -0700, Jan Beulich wrote: > This is pretty simplistic for now, but I'd rather have someone better > friends with the tools improve it (if desired). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx > return 0; > } > > +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest, > + int *lower_thresh, int *upper_thresh) > +{ > + int ret; > + GC_INIT(ctx); > + if (set) { > + ret = xc_set_log_level(ctx->xch, guest, *lower_thresh, *upper_thresh); > + } else { > + ret = xc_get_log_level(ctx->xch, guest, lower_thresh, upper_thresh); > + } > + if ( ret < 0 ) { > + LOGE(ERROR, "%s %slog level", > + set ? "setting" : "getting", guest ? "guest " : ""); > + GC_FREE; > + return ERROR_FAIL; > + } > + GC_FREE; > + return 0; > +} > + As Dario said, libxl tends to have getter and setter pair. > libxl_xen_console_reader * > libxl_xen_console_read_start(libxl_ctx *ctx, int clear) > { [...] > > /* > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -6469,6 +6469,84 @@ int main_debug_keys(int argc, char **arg > return 0; > } > > +static const struct { > + int level; > + char string[8]; > +} loglvls[] = { > + { 0, "none" }, > + { 1, "error" }, > + { 2, "warning" }, > + { 3, "info" }, > + { 4, "all" }, > + { 4, "debug" }, The semantics of the numbers should go into libxl_types.idl. Maybe something like # Keep in sync with Xen log level. libxl_xen_log_level = Enumeration (...); Then in here static const struct { int level; char string[8]; } loglvls[] = { { LIBXL_XEN_LOG_LEVEL_NONE, "none" }, { ..., "error" }, { ..., "warning" }, { ..., "info" }, { ..., "all" }, { ..., "debug" }, Please also note that after the introduction of this API, Xen log level will become part of the stable API and subject to some compatibility constraints. Is this what you wanted? > +}; > + > +static int parse_loglvl(char **parg) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(loglvls); ++i) { > + size_t l = strlen(loglvls[i].string); > + > + if (!strncmp(*parg, loglvls[i].string, l)) { > + *parg += l; > + return loglvls[i].level; > + } > + } > + > + return -1; > +} > + > +static const char *format_loglvl(int loglvl) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(loglvls); ++i) { > + if (loglvl == loglvls[i].level) > + return loglvls[i].string; > + } > + > + return "<unknown>"; > +} > + > +int main_loglvl(int argc, char **argv) > +{ > + static const struct option opts[] = { > + {"guest", 0, 0, 'g'}, > + {"set", 0, 0, 's'}, > + COMMON_LONG_OPTS > + }; > + int opt, lower_thresh = -1, upper_thresh = -1; > + bool guest = false, set = false; > + > + SWITCH_FOREACH_OPT(opt, "gs:", opts, "loglvl", 0) { > + case 'g': > + guest = true; > + break; > + case 's': > + if (*optarg != '/') > + lower_thresh = parse_loglvl(&optarg); > + if (*optarg == '/') { > + ++optarg; > + upper_thresh = parse_loglvl(&optarg); > + } > + set = true; > + break; > + } > + > + if (libxl_log_level(ctx, set, guest, &lower_thresh, &upper_thresh)) { > + fprintf(stderr, "cannot %s %s log level\n", > + set ? "set" : "get", guest ? "guest" : "host"); > + return 1; > + } > + > + if (!set) > + printf("%s log levels: %s/%s\n", guest ? "guest" : "host", > + format_loglvl(lower_thresh), format_loglvl(upper_thresh)); > + > + return 0; > +} > + You also need to patch xl manpage. Wei.
On Tue, 2016-03-08 at 14:05 +0000, George Dunlap wrote: > On Tue, Mar 8, 2016 at 8:08 AM, Jan Beulich <JBeulich@suse.com> > wrote: > > > > Right, and asking people myself to not follow bad examples when > > adding new code, I did take all of your input to adjust the patch. > > Just that in this case the set of bad examples is so large that in > > a > > similar case in the hypervisor I probably wouldn't have dared to > > ask for a style correction. > Well the approach of the libxl maintainers seems to have be, "Just > make sure the new code adheres to the new style, and eventyally > everything will be up-to-date". > Funnily enough, basing on my experience, libxl does not look that bad to me, and every time I've been bitten by something like this, it was in Xen rather than in libxl. :-D Of course, although I've been active in both, I don't claim that my experience is statistically significant... I guess it depends on what specific areas of code one gets to work on. Anyway, I personally don't think this affect in any way the point that new code should comply as much as possible with coding style, existing best practises, etc., and that is true for this patch, as well as for all the times everyone of us may have been asked to do the same, either in xen, tools, or anywhere... In fact, especially if we decide to do this (which I'd be in favour of, and up for helping): > Given that the "new" style has been around for a while now, it > probably would be good to set aside some time at the beginning of the > next development cycle to fix things up > being strict about new code actually helps this, as it makes sure there is less --rather than more-- code to fix during such a huge fixup challenge! :-) > -- it is incredibly > frustrating to carefully try to copy the surrounding style, only to > be > told to revise it. > Yep, I fully agree. Regards, Dario
>>> On 08.03.16 at 17:20, <wei.liu2@citrix.com> wrote: > On Fri, Mar 04, 2016 at 09:48:16AM -0700, Jan Beulich wrote: >> This is pretty simplistic for now, but I'd rather have someone better >> friends with the tools improve it (if desired). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx >> return 0; >> } >> >> +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest, >> + int *lower_thresh, int *upper_thresh) >> +{ >> + int ret; >> + GC_INIT(ctx); >> + if (set) { >> + ret = xc_set_log_level(ctx->xch, guest, *lower_thresh, *upper_thresh); >> + } else { >> + ret = xc_get_log_level(ctx->xch, guest, lower_thresh, upper_thresh); >> + } >> + if ( ret < 0 ) { >> + LOGE(ERROR, "%s %slog level", >> + set ? "setting" : "getting", guest ? "guest " : ""); >> + GC_FREE; >> + return ERROR_FAIL; >> + } >> + GC_FREE; >> + return 0; >> +} >> + > > As Dario said, libxl tends to have getter and setter pair. "Tends to have" still leaves room for me to get away without adjustments. Could you please clearly state whether splitting this is required for acceptance? >> --- a/tools/libxl/xl_cmdimpl.c >> +++ b/tools/libxl/xl_cmdimpl.c >> @@ -6469,6 +6469,84 @@ int main_debug_keys(int argc, char **arg >> return 0; >> } >> >> +static const struct { >> + int level; >> + char string[8]; >> +} loglvls[] = { >> + { 0, "none" }, >> + { 1, "error" }, >> + { 2, "warning" }, >> + { 3, "info" }, >> + { 4, "all" }, >> + { 4, "debug" }, > > The semantics of the numbers should go into libxl_types.idl. Maybe > something like > > # Keep in sync with Xen log level. > libxl_xen_log_level = Enumeration (...); > > Then in here > > static const struct { > int level; > char string[8]; > } loglvls[] = { > { LIBXL_XEN_LOG_LEVEL_NONE, "none" }, > { ..., "error" }, > { ..., "warning" }, > { ..., "info" }, > { ..., "all" }, > { ..., "debug" }, > > Please also note that after the introduction of this API, Xen log level > will become part of the stable API and subject to some compatibility > constraints. Is this what you wanted? No, and I actually I'm having trouble following your request: This lives in xl_cmdimpl.c, which I didn't think is subject to any stability requirements in the mapping of strings (from the xl command line) to numbers. It is _specifically_ that I do not want to fix those mappings. Jan
On Mon, Mar 14, 2016 at 09:23:35AM -0600, Jan Beulich wrote: > >>> On 08.03.16 at 17:20, <wei.liu2@citrix.com> wrote: > > On Fri, Mar 04, 2016 at 09:48:16AM -0700, Jan Beulich wrote: > >> This is pretty simplistic for now, but I'd rather have someone better > >> friends with the tools improve it (if desired). > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> > >> --- a/tools/libxl/libxl.c > >> +++ b/tools/libxl/libxl.c > >> @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx > >> return 0; > >> } > >> > >> +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest, > >> + int *lower_thresh, int *upper_thresh) > >> +{ > >> + int ret; > >> + GC_INIT(ctx); > >> + if (set) { > >> + ret = xc_set_log_level(ctx->xch, guest, *lower_thresh, *upper_thresh); > >> + } else { > >> + ret = xc_get_log_level(ctx->xch, guest, lower_thresh, upper_thresh); > >> + } > >> + if ( ret < 0 ) { > >> + LOGE(ERROR, "%s %slog level", > >> + set ? "setting" : "getting", guest ? "guest " : ""); > >> + GC_FREE; > >> + return ERROR_FAIL; > >> + } > >> + GC_FREE; > >> + return 0; > >> +} > >> + > > > > As Dario said, libxl tends to have getter and setter pair. > > "Tends to have" still leaves room for me to get away without > adjustments. Could you please clearly state whether splitting > this is required for acceptance? > Yes, please make a pair of getter and setter. > >> --- a/tools/libxl/xl_cmdimpl.c > >> +++ b/tools/libxl/xl_cmdimpl.c > >> @@ -6469,6 +6469,84 @@ int main_debug_keys(int argc, char **arg > >> return 0; > >> } > >> > >> +static const struct { > >> + int level; > >> + char string[8]; > >> +} loglvls[] = { > >> + { 0, "none" }, > >> + { 1, "error" }, > >> + { 2, "warning" }, > >> + { 3, "info" }, > >> + { 4, "all" }, > >> + { 4, "debug" }, > > > > The semantics of the numbers should go into libxl_types.idl. Maybe > > something like > > > > # Keep in sync with Xen log level. > > libxl_xen_log_level = Enumeration (...); > > > > Then in here > > > > static const struct { > > int level; > > char string[8]; > > } loglvls[] = { > > { LIBXL_XEN_LOG_LEVEL_NONE, "none" }, > > { ..., "error" }, > > { ..., "warning" }, > > { ..., "info" }, > > { ..., "all" }, > > { ..., "debug" }, > > > > Please also note that after the introduction of this API, Xen log level > > will become part of the stable API and subject to some compatibility > > constraints. Is this what you wanted? > > No, and I actually I'm having trouble following your request: This > lives in xl_cmdimpl.c, which I didn't think is subject to any stability > requirements in the mapping of strings (from the xl command line) > to numbers. It is _specifically_ that I do not want to fix those > mappings. > The new API libxl_log_level relies on the semantics of these mappings. To make the new API useful to all clients, the semantics of log levels needs to go into libxl_types.idl (as mentioned a few lines above), hence it becomes part of the stable API. Otherwise you end up with an API accepting arbitrary magic numbers. Wei. > Jan >
>>> On 14.03.16 at 16:36, <wei.liu2@citrix.com> wrote: > On Mon, Mar 14, 2016 at 09:23:35AM -0600, Jan Beulich wrote: >> >>> On 08.03.16 at 17:20, <wei.liu2@citrix.com> wrote: >> > On Fri, Mar 04, 2016 at 09:48:16AM -0700, Jan Beulich wrote: >> >> --- a/tools/libxl/xl_cmdimpl.c >> >> +++ b/tools/libxl/xl_cmdimpl.c >> >> @@ -6469,6 +6469,84 @@ int main_debug_keys(int argc, char **arg >> >> return 0; >> >> } >> >> >> >> +static const struct { >> >> + int level; >> >> + char string[8]; >> >> +} loglvls[] = { >> >> + { 0, "none" }, >> >> + { 1, "error" }, >> >> + { 2, "warning" }, >> >> + { 3, "info" }, >> >> + { 4, "all" }, >> >> + { 4, "debug" }, >> > >> > The semantics of the numbers should go into libxl_types.idl. Maybe >> > something like >> > >> > # Keep in sync with Xen log level. >> > libxl_xen_log_level = Enumeration (...); >> > >> > Then in here >> > >> > static const struct { >> > int level; >> > char string[8]; >> > } loglvls[] = { >> > { LIBXL_XEN_LOG_LEVEL_NONE, "none" }, >> > { ..., "error" }, >> > { ..., "warning" }, >> > { ..., "info" }, >> > { ..., "all" }, >> > { ..., "debug" }, >> > >> > Please also note that after the introduction of this API, Xen log level >> > will become part of the stable API and subject to some compatibility >> > constraints. Is this what you wanted? >> >> No, and I actually I'm having trouble following your request: This >> lives in xl_cmdimpl.c, which I didn't think is subject to any stability >> requirements in the mapping of strings (from the xl command line) >> to numbers. It is _specifically_ that I do not want to fix those >> mappings. > > The new API libxl_log_level relies on the semantics of these mappings. > To make the new API useful to all clients, the semantics of log levels > needs to go into libxl_types.idl (as mentioned a few lines above), hence > it becomes part of the stable API. Otherwise you end up with an API > accepting arbitrary magic numbers. Well - what do you suggest? I'm meanwhile willing to give up on this, as baking the mapping into some ABI is clearly not what we should do or what I want. Moving the translation into libxl also wouldn't help. Which would leave the option of doing this in libxc, but then again I'm generally opposed to passing around strings instead of numbers, as the latter are better to work with at all layers. Jan
On Mon, Mar 14, 2016 at 09:49:11AM -0600, Jan Beulich wrote: > >>> On 14.03.16 at 16:36, <wei.liu2@citrix.com> wrote: > > On Mon, Mar 14, 2016 at 09:23:35AM -0600, Jan Beulich wrote: > >> >>> On 08.03.16 at 17:20, <wei.liu2@citrix.com> wrote: > >> > On Fri, Mar 04, 2016 at 09:48:16AM -0700, Jan Beulich wrote: > >> >> --- a/tools/libxl/xl_cmdimpl.c > >> >> +++ b/tools/libxl/xl_cmdimpl.c > >> >> @@ -6469,6 +6469,84 @@ int main_debug_keys(int argc, char **arg > >> >> return 0; > >> >> } > >> >> > >> >> +static const struct { > >> >> + int level; > >> >> + char string[8]; > >> >> +} loglvls[] = { > >> >> + { 0, "none" }, > >> >> + { 1, "error" }, > >> >> + { 2, "warning" }, > >> >> + { 3, "info" }, > >> >> + { 4, "all" }, > >> >> + { 4, "debug" }, > >> > > >> > The semantics of the numbers should go into libxl_types.idl. Maybe > >> > something like > >> > > >> > # Keep in sync with Xen log level. > >> > libxl_xen_log_level = Enumeration (...); > >> > > >> > Then in here > >> > > >> > static const struct { > >> > int level; > >> > char string[8]; > >> > } loglvls[] = { > >> > { LIBXL_XEN_LOG_LEVEL_NONE, "none" }, > >> > { ..., "error" }, > >> > { ..., "warning" }, > >> > { ..., "info" }, > >> > { ..., "all" }, > >> > { ..., "debug" }, > >> > > >> > Please also note that after the introduction of this API, Xen log level > >> > will become part of the stable API and subject to some compatibility > >> > constraints. Is this what you wanted? > >> > >> No, and I actually I'm having trouble following your request: This > >> lives in xl_cmdimpl.c, which I didn't think is subject to any stability > >> requirements in the mapping of strings (from the xl command line) > >> to numbers. It is _specifically_ that I do not want to fix those > >> mappings. > > > > The new API libxl_log_level relies on the semantics of these mappings. > > To make the new API useful to all clients, the semantics of log levels > > needs to go into libxl_types.idl (as mentioned a few lines above), hence > > it becomes part of the stable API. Otherwise you end up with an API > > accepting arbitrary magic numbers. > > Well - what do you suggest? I'm meanwhile willing to give up on this, > as baking the mapping into some ABI is clearly not what we should do > or what I want. Moving the translation into libxl also wouldn't help. > Which would leave the option of doing this in libxc, but then again I'm > generally opposed to passing around strings instead of numbers, as > the latter are better to work with at all layers. > I don't have a very clear idea on what to suggest at the moment, sorry. What kind of changes to xen log level might happen in the future? Wei. > Jan >
>>> On 14.03.16 at 17:01, <wei.liu2@citrix.com> wrote: > On Mon, Mar 14, 2016 at 09:49:11AM -0600, Jan Beulich wrote: >> >>> On 14.03.16 at 16:36, <wei.liu2@citrix.com> wrote: >> > On Mon, Mar 14, 2016 at 09:23:35AM -0600, Jan Beulich wrote: >> >> >>> On 08.03.16 at 17:20, <wei.liu2@citrix.com> wrote: >> >> > On Fri, Mar 04, 2016 at 09:48:16AM -0700, Jan Beulich wrote: >> >> >> --- a/tools/libxl/xl_cmdimpl.c >> >> >> +++ b/tools/libxl/xl_cmdimpl.c >> >> >> @@ -6469,6 +6469,84 @@ int main_debug_keys(int argc, char **arg >> >> >> return 0; >> >> >> } >> >> >> >> >> >> +static const struct { >> >> >> + int level; >> >> >> + char string[8]; >> >> >> +} loglvls[] = { >> >> >> + { 0, "none" }, >> >> >> + { 1, "error" }, >> >> >> + { 2, "warning" }, >> >> >> + { 3, "info" }, >> >> >> + { 4, "all" }, >> >> >> + { 4, "debug" }, >> >> > >> >> > The semantics of the numbers should go into libxl_types.idl. Maybe >> >> > something like >> >> > >> >> > # Keep in sync with Xen log level. >> >> > libxl_xen_log_level = Enumeration (...); >> >> > >> >> > Then in here >> >> > >> >> > static const struct { >> >> > int level; >> >> > char string[8]; >> >> > } loglvls[] = { >> >> > { LIBXL_XEN_LOG_LEVEL_NONE, "none" }, >> >> > { ..., "error" }, >> >> > { ..., "warning" }, >> >> > { ..., "info" }, >> >> > { ..., "all" }, >> >> > { ..., "debug" }, >> >> > >> >> > Please also note that after the introduction of this API, Xen log level >> >> > will become part of the stable API and subject to some compatibility >> >> > constraints. Is this what you wanted? >> >> >> >> No, and I actually I'm having trouble following your request: This >> >> lives in xl_cmdimpl.c, which I didn't think is subject to any stability >> >> requirements in the mapping of strings (from the xl command line) >> >> to numbers. It is _specifically_ that I do not want to fix those >> >> mappings. >> > >> > The new API libxl_log_level relies on the semantics of these mappings. >> > To make the new API useful to all clients, the semantics of log levels >> > needs to go into libxl_types.idl (as mentioned a few lines above), hence >> > it becomes part of the stable API. Otherwise you end up with an API >> > accepting arbitrary magic numbers. >> >> Well - what do you suggest? I'm meanwhile willing to give up on this, >> as baking the mapping into some ABI is clearly not what we should do >> or what I want. Moving the translation into libxl also wouldn't help. >> Which would leave the option of doing this in libxc, but then again I'm >> generally opposed to passing around strings instead of numbers, as >> the latter are better to work with at all layers. >> > > I don't have a very clear idea on what to suggest at the moment, sorry. > What kind of changes to xen log level might happen in the future? They could become more fine grained (for example, Linux has a few more than we have now). And the string/number correlation is an implementation detail anyway. Jan
Jan Beulich writes ("Re: [PATCH v2 3/3] xl: new "loglvl" command"):
> They could become more fine grained (for example, Linux has a
> few more than we have now). And the string/number correlation
> is an implementation detail anyway.
Could we solve that problem by multiplying the numbers by 10 ?
Ian.
>>> On 14.03.16 at 18:07, <Ian.Jackson@eu.citrix.com> wrote: > Jan Beulich writes ("Re: [PATCH v2 3/3] xl: new "loglvl" command"): >> They could become more fine grained (for example, Linux has a >> few more than we have now). And the string/number correlation >> is an implementation detail anyway. > > Could we solve that problem by multiplying the numbers by 10 ? That particular one probably yes, but who knows what else adjustments there might be in the future. Plus - which layer would you see do the transformation back then? Hypervisor? libxc? In the end all speaks for passing around strings all the way down to the hypervisor then, however ugly I may consider such... Jan
On Tue, Mar 15, 2016 at 01:37:39AM -0600, Jan Beulich wrote: > >>> On 14.03.16 at 18:07, <Ian.Jackson@eu.citrix.com> wrote: > > Jan Beulich writes ("Re: [PATCH v2 3/3] xl: new "loglvl" command"): > >> They could become more fine grained (for example, Linux has a > >> few more than we have now). And the string/number correlation > >> is an implementation detail anyway. > > > > Could we solve that problem by multiplying the numbers by 10 ? > > That particular one probably yes, but who knows what else > adjustments there might be in the future. Plus - which layer > would you see do the transformation back then? Hypervisor? > libxc? In the end all speaks for passing around strings all the > way down to the hypervisor then, however ugly I may > consider such... The constraint is that we can't delete log levels in libxl once it is exposed to application. If the hypervisor log level changes (especially in case of deletion) we need to map the old one to new one. With that in mind it make more sense to have the transformation layer in libxl. Note that passing a string down won't give us benefit with regard to maintaining backward compatibility -- there still needs to be a compatibility shim somewhere in the event of deletion, so we might just do it in libxl. Does this make sense? BTW I can take over the toolstack part for you if we reach agreement on how to proceed. I think you've had enough stuff on your plate now. It would be easier for me to write toolstack code and save both us some time. Wei. > > Jan >
>>> On 15.03.16 at 14:58, <wei.liu2@citrix.com> wrote: > On Tue, Mar 15, 2016 at 01:37:39AM -0600, Jan Beulich wrote: >> >>> On 14.03.16 at 18:07, <Ian.Jackson@eu.citrix.com> wrote: >> > Jan Beulich writes ("Re: [PATCH v2 3/3] xl: new "loglvl" command"): >> >> They could become more fine grained (for example, Linux has a >> >> few more than we have now). And the string/number correlation >> >> is an implementation detail anyway. >> > >> > Could we solve that problem by multiplying the numbers by 10 ? >> >> That particular one probably yes, but who knows what else >> adjustments there might be in the future. Plus - which layer >> would you see do the transformation back then? Hypervisor? >> libxc? In the end all speaks for passing around strings all the >> way down to the hypervisor then, however ugly I may >> consider such... > > The constraint is that we can't delete log levels in libxl once it is > exposed to application. If the hypervisor log level changes (especially > in case of deletion) we need to map the old one to new one. With that > in mind it make more sense to have the transformation layer in libxl. > > Note that passing a string down won't give us benefit with regard to > maintaining backward compatibility -- there still needs to be a > compatibility shim somewhere in the event of deletion, so we might just > do it in libxl. > > Does this make sense? Yes and no. If all of the sudden the hypervisor didn't have an "error" log level anymore, what would you do? Mapping "error" to "warning" wouldn't be right. Nor would mapping it to anything else. Correct behavior in that case would simply be failure, and it wouldn't seem too relevant to me at what layer that failure would get signaled. > BTW I can take over the toolstack part for you if we reach agreement on > how to proceed. I think you've had enough stuff on your plate now. It > would be easier for me to write toolstack code and save both us some > time. Oh, thanks a lot, I would much appreciate that. Jan
On Tue, Mar 15, 2016 at 08:07:42AM -0600, Jan Beulich wrote: > >>> On 15.03.16 at 14:58, <wei.liu2@citrix.com> wrote: > > On Tue, Mar 15, 2016 at 01:37:39AM -0600, Jan Beulich wrote: > >> >>> On 14.03.16 at 18:07, <Ian.Jackson@eu.citrix.com> wrote: > >> > Jan Beulich writes ("Re: [PATCH v2 3/3] xl: new "loglvl" command"): > >> >> They could become more fine grained (for example, Linux has a > >> >> few more than we have now). And the string/number correlation > >> >> is an implementation detail anyway. > >> > > >> > Could we solve that problem by multiplying the numbers by 10 ? > >> > >> That particular one probably yes, but who knows what else > >> adjustments there might be in the future. Plus - which layer > >> would you see do the transformation back then? Hypervisor? > >> libxc? In the end all speaks for passing around strings all the > >> way down to the hypervisor then, however ugly I may > >> consider such... > > > > The constraint is that we can't delete log levels in libxl once it is > > exposed to application. If the hypervisor log level changes (especially > > in case of deletion) we need to map the old one to new one. With that > > in mind it make more sense to have the transformation layer in libxl. > > > > Note that passing a string down won't give us benefit with regard to > > maintaining backward compatibility -- there still needs to be a > > compatibility shim somewhere in the event of deletion, so we might just > > do it in libxl. > > > > Does this make sense? > > Yes and no. If all of the sudden the hypervisor didn't have an "error" > log level anymore, what would you do? Mapping "error" to "warning" > wouldn't be right. Nor would mapping it to anything else. Correct > behavior in that case would simply be failure, and it wouldn't seem > too relevant to me at what layer that failure would get signaled. > OK, so my thought was that application should continue to work. First we can't break compilation of applications, second we should make it continue to function whenever possible. The first requirement is easy, using either number or string works the same. As for the second, I was thinking about downgrading or upgrading to a different log level -- what would you do to the hypervisor command line option guest_loglvl if one of the log levels is gone? Do you just crash xen or setting log level to some other value? I think libxl can follow this same principle. But then you are of the opinion that it should just return error if one log level is gone -- I think this is probably fine too , we just need to document the semantics the API. Wei. > > BTW I can take over the toolstack part for you if we reach agreement on > > how to proceed. I think you've had enough stuff on your plate now. It > > would be easier for me to write toolstack code and save both us some > > time. > > Oh, thanks a lot, I would much appreciate that. > > Jan >
>>> On 15.03.16 at 15:51, <wei.liu2@citrix.com> wrote: > As for the second, I was thinking about downgrading or upgrading to a > different log level -- what would you do to the hypervisor command line > option guest_loglvl if one of the log levels is gone? Do you just crash > xen or setting log level to some other value? I think libxl can follow > this same principle. Certainly not crash Xen. I guess the most natural thing would be to ignore the bad request, as we do now for log level string we can't make sense of. > But then you are of the opinion that it should just return error if one > log level is gone -- I think this is probably fine too , we just need to > document the semantics the API. I think that's a reasonable route then. Jan
Jan Beulich writes ("Re: [PATCH v2 3/3] xl: new "loglvl" command"):
> Yes and no. If all of the sudden the hypervisor didn't have an "error"
> log level anymore, what would you do? Mapping "error" to "warning"
> wouldn't be right. Nor would mapping it to anything else. Correct
> behavior in that case would simply be failure, and it wouldn't seem
> too relevant to me at what layer that failure would get signaled.
I think you are looking at this the wrong way.
Log levels are primarily attributes of messages. Messages are
categorised by the hypervisor code into one of a set of levels. The
levels form a total order (which I'm going to call `boringness').
Every message has a level, too.
A request to set the log level to a particular value is a request for
the hypervisor to print all messages whose message level is no more as
`boring' than the requested log level.
If the hypervisor changes so as to abolish a level, that does not mean
that it is now nonsensical to request to set the log level to the
now-abolished value. It simply means that the set of messages at that
level becomes the empty set.
Likewise if the hypervisor changes so as to introduce a new level B
(such that A < B < C where A and C are existing levels), this simply
means that old code which doesn't know about B cannot specify
explicitly which of {A}, {A,B}, {A,B,C} it wants. When introducing B
we need to make a decision about whether old code which specified C
(ie show all messages of boringness at least C) should be treated as
having asked for B too. (Obviously old code which specified A will
get B.)
None of this depends on whether the levels are represented as strings
or atoms or numbers or whatever.
(I note that there is some confusion because the ordering is inverted.
That is, rather than messages having severities, and the log level
being a severity threshold; the primary question is log level
verbosity and messages have a boringness threshold. Most other log
level systems assign larger level numbers to more interesting
messages. I am goong to continue to work with the existing sense of
the numerical level parameter because inverting it now will be
confusing.)
I would like to propose the following scheme:
* Multiply, right now, as a one-off ABI change, all the hypervisor
message levels by (let us say) 100, and add (say) 10000. So
error 10100
warning 10200
info 10300
debug 10400
* Declare that the hypervisor ABI is stable in this area. The
hypercall provides the hypervisor with a number (the log level) and
the hypervisor will print all messages whose message level number
is no larger than the specified log level number.
* Change all existing tools and user-facing interfaces[1] which set
the log level to convert string-to-number using a table which, in
its initial form, is identical to the message level enum but with
50 added to each value. This table also has the "none" and "all"
entries:
all 2147483647 [no messages must ever have such a high boringess]
error 10150
warning 10250
info 10350
debug 10450
none 0
[1] This includes both the hypervisor command line and libxl.
The log level request enum becumes a libxl idl enum, too.
We do NOT provide the actual message level numerical values outside
hypervisor code.
* When we remove a level, we remove its enum definition in the
hypervisor code, so that we can be sure that code remains which
generates the removed level. But we retain its name in the
string-to-number table, for the benefit of old users. Eventually
we can make use of the old name produce a warning, and even later,
we can remove the name.
* When we introduce a level, we assign it a new number. We assign
it either +25 or +75, according to whether we want the new level
to count as the lower of the two old levels for naive programs,
or as the higher. Eg:
To introduce "notice" To introduce "notice"
which old "warning" excludes: which old "warning" includes:
[in message level enum:] [in message level enum:]
warning 10200 warning 10200
+ notice 10275 + notice 10225
info 10300 info 10300
[in string-to-level table:] [in string-to-level table:]
info 10350 info 10350
+ notice 10287 + notice 10212
warning 10250 warning 10250
If we do not want to be able to decide, when we introduce a new log
level, which way the "old callers" decision goes, then the
requested level string-to-number table and the hypervisor message
generation level enum can have identical numerical values.
That would be simpler, and would retain a good degree of backward
compatibility.
Ian.
>>> On 15.03.16 at 16:38, <Ian.Jackson@eu.citrix.com> wrote: > Jan Beulich writes ("Re: [PATCH v2 3/3] xl: new "loglvl" command"): >> Yes and no. If all of the sudden the hypervisor didn't have an "error" >> log level anymore, what would you do? Mapping "error" to "warning" >> wouldn't be right. Nor would mapping it to anything else. Correct >> behavior in that case would simply be failure, and it wouldn't seem >> too relevant to me at what layer that failure would get signaled. > > I think you are looking at this the wrong way. Quite possible, and all of what you write makes sense. Yet that wasn't my intention here. I specifically put the string <-> number mapping in xl, so it could be that (and only that, outside the hypervisor itself) which gets changed if the hypervisor log levels ever change. The tool could use version information or some other detection mechanism to provide backwards compatibility (and be independent of the precise hypervisor version it got built in parallel with, if that's desired). And hence I specifically made the interfaces dumb - raw numbers, with no meaning assigned to their values. And then, with what you describe I assume the current hypervisor side implementation wouldn't be suitable anymore anyway, as the translation between the interface exposed log levels and the internally used ones would need to happen in the sysctl handler. To me, all of this looks increasingly like over-engineering for a very simple debugging aid (which is all the new command was meant for). If you and Wei can settle on some alternative implementation, I'm fine to accept that, but I don't think I'm going to spend much more time on fiddling with any of the 3 patches. It's going to be sad though if even the serial console based log level adjustment won't make it into 4.7, despite it having got posted months ago (with this v2 just extending on it). Jan > Log levels are primarily attributes of messages. Messages are > categorised by the hypervisor code into one of a set of levels. The > levels form a total order (which I'm going to call `boringness'). > Every message has a level, too. > > A request to set the log level to a particular value is a request for > the hypervisor to print all messages whose message level is no more as > `boring' than the requested log level. > > If the hypervisor changes so as to abolish a level, that does not mean > that it is now nonsensical to request to set the log level to the > now-abolished value. It simply means that the set of messages at that > level becomes the empty set. > > Likewise if the hypervisor changes so as to introduce a new level B > (such that A < B < C where A and C are existing levels), this simply > means that old code which doesn't know about B cannot specify > explicitly which of {A}, {A,B}, {A,B,C} it wants. When introducing B > we need to make a decision about whether old code which specified C > (ie show all messages of boringness at least C) should be treated as > having asked for B too. (Obviously old code which specified A will > get B.) > > None of this depends on whether the levels are represented as strings > or atoms or numbers or whatever. > > (I note that there is some confusion because the ordering is inverted. > That is, rather than messages having severities, and the log level > being a severity threshold; the primary question is log level > verbosity and messages have a boringness threshold. Most other log > level systems assign larger level numbers to more interesting > messages. I am goong to continue to work with the existing sense of > the numerical level parameter because inverting it now will be > confusing.) > > > I would like to propose the following scheme: > > * Multiply, right now, as a one-off ABI change, all the hypervisor > message levels by (let us say) 100, and add (say) 10000. So > error 10100 > warning 10200 > info 10300 > debug 10400 > > * Declare that the hypervisor ABI is stable in this area. The > hypercall provides the hypervisor with a number (the log level) and > the hypervisor will print all messages whose message level number > is no larger than the specified log level number. > > * Change all existing tools and user-facing interfaces[1] which set > the log level to convert string-to-number using a table which, in > its initial form, is identical to the message level enum but with > 50 added to each value. This table also has the "none" and "all" > entries: > all 2147483647 [no messages must ever have such a high > boringess] > error 10150 > warning 10250 > info 10350 > debug 10450 > none 0 > [1] This includes both the hypervisor command line and libxl. > The log level request enum becumes a libxl idl enum, too. > > We do NOT provide the actual message level numerical values outside > hypervisor code. > > * When we remove a level, we remove its enum definition in the > hypervisor code, so that we can be sure that code remains which > generates the removed level. But we retain its name in the > string-to-number table, for the benefit of old users. Eventually > we can make use of the old name produce a warning, and even later, > we can remove the name. > > * When we introduce a level, we assign it a new number. We assign > it either +25 or +75, according to whether we want the new level > to count as the lower of the two old levels for naive programs, > or as the higher. Eg: > > To introduce "notice" To introduce "notice" > which old "warning" excludes: which old "warning" includes: > > [in message level enum:] [in message level enum:] > warning 10200 warning 10200 > + notice 10275 + notice 10225 > info 10300 info 10300 > [in string-to-level table:] [in string-to-level table:] > info 10350 info 10350 > + notice 10287 + notice 10212 > warning 10250 warning 10250 > > If we do not want to be able to decide, when we introduce a new log > level, which way the "old callers" decision goes, then the > requested level string-to-number table and the hypervisor message > generation level enum can have identical numerical values. > > That would be simpler, and would retain a good degree of backward > compatibility. > > Ian.
On Wed, Mar 16, 2016 at 05:22:27AM -0600, Jan Beulich wrote: > >>> On 15.03.16 at 16:38, <Ian.Jackson@eu.citrix.com> wrote: > > Jan Beulich writes ("Re: [PATCH v2 3/3] xl: new "loglvl" command"): > >> Yes and no. If all of the sudden the hypervisor didn't have an "error" > >> log level anymore, what would you do? Mapping "error" to "warning" > >> wouldn't be right. Nor would mapping it to anything else. Correct > >> behavior in that case would simply be failure, and it wouldn't seem > >> too relevant to me at what layer that failure would get signaled. > > > > I think you are looking at this the wrong way. > > Quite possible, and all of what you write makes sense. Yet that > wasn't my intention here. I specifically put the string <-> number > mapping in xl, so it could be that (and only that, outside the > hypervisor itself) which gets changed if the hypervisor log levels > ever change. The tool could use version information or some > other detection mechanism to provide backwards compatibility > (and be independent of the precise hypervisor version it got > built in parallel with, if that's desired). And hence I specifically > made the interfaces dumb - raw numbers, with no meaning > assigned to their values. > > And then, with what you describe I assume the current hypervisor > side implementation wouldn't be suitable anymore anyway, as the > translation between the interface exposed log levels and the > internally used ones would need to happen in the sysctl handler. > > To me, all of this looks increasingly like over-engineering for a > very simple debugging aid (which is all the new command was > meant for). If you and Wei can settle on some alternative > implementation, I'm fine to accept that, but I don't think I'm > going to spend much more time on fiddling with any of the 3 > patches. It's going to be sad though if even the serial console > based log level adjustment won't make it into 4.7, despite it > having got posted months ago (with this v2 just extending on > it). > If this is just a debugging aid and not intending to be consumed by high level toolstack, maybe we can make a dedicated helper program? We already have a bunch of those. Should the need really arises we can then consider making it proper stable API / ABI. Wei.
>>> On 28.04.16 at 17:33, <wei.liu2@citrix.com> wrote: > On Wed, Mar 16, 2016 at 05:22:27AM -0600, Jan Beulich wrote: >> >>> On 15.03.16 at 16:38, <Ian.Jackson@eu.citrix.com> wrote: >> > Jan Beulich writes ("Re: [PATCH v2 3/3] xl: new "loglvl" command"): >> >> Yes and no. If all of the sudden the hypervisor didn't have an "error" >> >> log level anymore, what would you do? Mapping "error" to "warning" >> >> wouldn't be right. Nor would mapping it to anything else. Correct >> >> behavior in that case would simply be failure, and it wouldn't seem >> >> too relevant to me at what layer that failure would get signaled. >> > >> > I think you are looking at this the wrong way. >> >> Quite possible, and all of what you write makes sense. Yet that >> wasn't my intention here. I specifically put the string <-> number >> mapping in xl, so it could be that (and only that, outside the >> hypervisor itself) which gets changed if the hypervisor log levels >> ever change. The tool could use version information or some >> other detection mechanism to provide backwards compatibility >> (and be independent of the precise hypervisor version it got >> built in parallel with, if that's desired). And hence I specifically >> made the interfaces dumb - raw numbers, with no meaning >> assigned to their values. >> >> And then, with what you describe I assume the current hypervisor >> side implementation wouldn't be suitable anymore anyway, as the >> translation between the interface exposed log levels and the >> internally used ones would need to happen in the sysctl handler. >> >> To me, all of this looks increasingly like over-engineering for a >> very simple debugging aid (which is all the new command was >> meant for). If you and Wei can settle on some alternative >> implementation, I'm fine to accept that, but I don't think I'm >> going to spend much more time on fiddling with any of the 3 >> patches. It's going to be sad though if even the serial console >> based log level adjustment won't make it into 4.7, despite it >> having got posted months ago (with this v2 just extending on >> it). > > If this is just a debugging aid and not intending to be consumed by high > level toolstack, maybe we can make a dedicated helper program? We > already have a bunch of those. Should the need really arises we can > then consider making it proper stable API / ABI. That's an option, albeit a slightly awkward one. This new thing really fits well with the debug-key and dmesg sub-commands, which both are there just for debugging, too. Jan
On Fri, Apr 29, 2016 at 01:20:57AM -0600, Jan Beulich wrote: > >>> On 28.04.16 at 17:33, <wei.liu2@citrix.com> wrote: > > On Wed, Mar 16, 2016 at 05:22:27AM -0600, Jan Beulich wrote: > >> >>> On 15.03.16 at 16:38, <Ian.Jackson@eu.citrix.com> wrote: > >> > Jan Beulich writes ("Re: [PATCH v2 3/3] xl: new "loglvl" command"): > >> >> Yes and no. If all of the sudden the hypervisor didn't have an "error" > >> >> log level anymore, what would you do? Mapping "error" to "warning" > >> >> wouldn't be right. Nor would mapping it to anything else. Correct > >> >> behavior in that case would simply be failure, and it wouldn't seem > >> >> too relevant to me at what layer that failure would get signaled. > >> > > >> > I think you are looking at this the wrong way. > >> > >> Quite possible, and all of what you write makes sense. Yet that > >> wasn't my intention here. I specifically put the string <-> number > >> mapping in xl, so it could be that (and only that, outside the > >> hypervisor itself) which gets changed if the hypervisor log levels > >> ever change. The tool could use version information or some > >> other detection mechanism to provide backwards compatibility > >> (and be independent of the precise hypervisor version it got > >> built in parallel with, if that's desired). And hence I specifically > >> made the interfaces dumb - raw numbers, with no meaning > >> assigned to their values. > >> > >> And then, with what you describe I assume the current hypervisor > >> side implementation wouldn't be suitable anymore anyway, as the > >> translation between the interface exposed log levels and the > >> internally used ones would need to happen in the sysctl handler. > >> > >> To me, all of this looks increasingly like over-engineering for a > >> very simple debugging aid (which is all the new command was > >> meant for). If you and Wei can settle on some alternative > >> implementation, I'm fine to accept that, but I don't think I'm > >> going to spend much more time on fiddling with any of the 3 > >> patches. It's going to be sad though if even the serial console > >> based log level adjustment won't make it into 4.7, despite it > >> having got posted months ago (with this v2 just extending on > >> it). > > > > If this is just a debugging aid and not intending to be consumed by high > > level toolstack, maybe we can make a dedicated helper program? We > > already have a bunch of those. Should the need really arises we can > > then consider making it proper stable API / ABI. > > That's an option, albeit a slightly awkward one. This new thing > really fits well with the debug-key and dmesg sub-commands, > which both are there just for debugging, too. > This is a good argument. I'm fine with treating it like debug-key and dmesg. Ian? Wei. > Jan >
--- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx return 0; } +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest, + int *lower_thresh, int *upper_thresh) +{ + int ret; + GC_INIT(ctx); + if (set) { + ret = xc_set_log_level(ctx->xch, guest, *lower_thresh, *upper_thresh); + } else { + ret = xc_get_log_level(ctx->xch, guest, lower_thresh, upper_thresh); + } + if ( ret < 0 ) { + LOGE(ERROR, "%s %slog level", + set ? "setting" : "getting", guest ? "guest " : ""); + GC_FREE; + return ERROR_FAIL; + } + GC_FREE; + return 0; +} + libxl_xen_console_reader * libxl_xen_console_read_start(libxl_ctx *ctx, int clear) { --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -1765,6 +1765,8 @@ int libxl_send_trigger(libxl_ctx *ctx, u libxl_trigger trigger, uint32_t vcpuid); int libxl_send_sysrq(libxl_ctx *ctx, uint32_t domid, char sysrq); int libxl_send_debug_keys(libxl_ctx *ctx, char *keys); +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest, + int *lower_thresh, int *upper_thresh); typedef struct libxl__xen_console_reader libxl_xen_console_reader; --- a/tools/libxl/xl.h +++ b/tools/libxl/xl.h @@ -81,6 +81,7 @@ int main_trigger(int argc, char **argv); int main_sysrq(int argc, char **argv); int main_debug_keys(int argc, char **argv); int main_dmesg(int argc, char **argv); +int main_loglvl(int argc, char **argv); int main_top(int argc, char **argv); int main_networkattach(int argc, char **argv); int main_networklist(int argc, char **argv); @@ -209,6 +210,8 @@ extern void printf_info_sexp(int domid, #define XL_GLOBAL_CONFIG XEN_CONFIG_DIR "/xl.conf" #define XL_LOCK_FILE XEN_LOCK_DIR "/xl" +#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0])) + #endif /* XL_H */ /* --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -6469,6 +6469,84 @@ int main_debug_keys(int argc, char **arg return 0; } +static const struct { + int level; + char string[8]; +} loglvls[] = { + { 0, "none" }, + { 1, "error" }, + { 2, "warning" }, + { 3, "info" }, + { 4, "all" }, + { 4, "debug" }, +}; + +static int parse_loglvl(char **parg) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(loglvls); ++i) { + size_t l = strlen(loglvls[i].string); + + if (!strncmp(*parg, loglvls[i].string, l)) { + *parg += l; + return loglvls[i].level; + } + } + + return -1; +} + +static const char *format_loglvl(int loglvl) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(loglvls); ++i) { + if (loglvl == loglvls[i].level) + return loglvls[i].string; + } + + return "<unknown>"; +} + +int main_loglvl(int argc, char **argv) +{ + static const struct option opts[] = { + {"guest", 0, 0, 'g'}, + {"set", 0, 0, 's'}, + COMMON_LONG_OPTS + }; + int opt, lower_thresh = -1, upper_thresh = -1; + bool guest = false, set = false; + + SWITCH_FOREACH_OPT(opt, "gs:", opts, "loglvl", 0) { + case 'g': + guest = true; + break; + case 's': + if (*optarg != '/') + lower_thresh = parse_loglvl(&optarg); + if (*optarg == '/') { + ++optarg; + upper_thresh = parse_loglvl(&optarg); + } + set = true; + break; + } + + if (libxl_log_level(ctx, set, guest, &lower_thresh, &upper_thresh)) { + fprintf(stderr, "cannot %s %s log level\n", + set ? "set" : "get", guest ? "guest" : "host"); + return 1; + } + + if (!set) + printf("%s log levels: %s/%s\n", guest ? "guest" : "host", + format_loglvl(lower_thresh), format_loglvl(upper_thresh)); + + return 0; +} + int main_dmesg(int argc, char **argv) { unsigned int clear = 0; --- a/tools/libxl/xl_cmdtable.c +++ b/tools/libxl/xl_cmdtable.c @@ -309,6 +309,13 @@ struct cmd_spec cmd_table[] = { "[-c]", " -c Clear dmesg buffer as well as printing it", }, + { "loglvl", + &main_loglvl, 0, 1, + "Manage Xen log levels", + "[-g] [-s=[LOWER][/UPPER]]", + "-g, --guest act on guest log level\n" + "-s [LOWER][/UPPER], --set=[LOWER][/UPPER] set new log level\n" + }, { "top", &main_top, 0, 0, "Monitor a host and the domains in real time",