* [PATCH v2 0/3] allow runtime log level threshold adjustments @ 2016-03-04 16:38 Jan Beulich 2016-03-04 16:46 ` [PATCH v2 1/3] console: allow " Jan Beulich ` (2 more replies) 0 siblings, 3 replies; 37+ messages in thread From: Jan Beulich @ 2016-03-04 16:38 UTC (permalink / raw) To: xen-devel Only patch 1 has been sent before. 1: console: allow log level threshold adjustments 2: libxc: wrapper for log level sysctl 3: xl: new "loglvl" command Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Add sysctl, libxc wrapper, and xl command. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 1/3] console: allow log level threshold adjustments 2016-03-04 16:38 [PATCH v2 0/3] allow runtime log level threshold adjustments Jan Beulich @ 2016-03-04 16:46 ` Jan Beulich 2016-03-04 20:55 ` Konrad Rzeszutek Wilk 2016-03-04 16:47 ` [PATCH v2 2/3] libxc: wrapper for log level sysctl Jan Beulich 2016-03-04 16:48 ` [PATCH v2 3/3] xl: new "loglvl" command Jan Beulich 2 siblings, 1 reply; 37+ messages in thread From: Jan Beulich @ 2016-03-04 16:46 UTC (permalink / raw) To: xen-devel; +Cc: Keir Fraser, Ian Jackson, Tim Deegan [-- Attachment #1: Type: text/plain, Size: 7319 bytes --] ... from serial console and via sysctl so that one doesn't always need to reboot to see more / less messages. Note that upper thresholds driven from the serial console are sticky, i.e. while they get adjusted upwards when the lower threshold would otherwise end up above the upper one, they don't get adjusted when reducing the lower one. Full flexibility is available only via the sysctl interface. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Add sysctl. --- a/xen/common/sysctl.c +++ b/xen/common/sysctl.c @@ -460,6 +460,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe ret = tmem_control(&op->u.tmem_op); break; + case XEN_SYSCTL_loglvl_op: + ret = console_loglvl_op(&op->u.loglvl); + break; + default: ret = arch_do_sysctl(op, u_sysctl); copyback = 0; --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -168,7 +168,7 @@ static void __init parse_guest_loglvl(ch _parse_loglvl(s, &xenlog_guest_lower_thresh, &xenlog_guest_upper_thresh); } -static char * __init loglvl_str(int lvl) +static char *loglvl_str(int lvl) { switch ( lvl ) { @@ -181,6 +181,119 @@ static char * __init loglvl_str(int lvl) return "???"; } +static int *__read_mostly upper_thresh_adj = &xenlog_upper_thresh; +static int *__read_mostly lower_thresh_adj = &xenlog_lower_thresh; +static const char *__read_mostly thresh_adj = "standard"; + +static void do_toggle_guest(unsigned char key, struct cpu_user_regs *regs) +{ + if ( upper_thresh_adj == &xenlog_upper_thresh ) + { + upper_thresh_adj = &xenlog_guest_upper_thresh; + lower_thresh_adj = &xenlog_guest_lower_thresh; + thresh_adj = "guest"; + } + else + { + upper_thresh_adj = &xenlog_upper_thresh; + lower_thresh_adj = &xenlog_lower_thresh; + thresh_adj = "standard"; + } + printk("'%c' pressed -> %s log level adjustments enabled\n", + key, thresh_adj); +} + +static void do_adj_thresh(unsigned char key) +{ + if ( *upper_thresh_adj < *lower_thresh_adj ) + *upper_thresh_adj = *lower_thresh_adj; + printk("'%c' pressed -> %s log level: %s (rate limited %s)\n", + key, thresh_adj, loglvl_str(*lower_thresh_adj), + loglvl_str(*upper_thresh_adj)); +} + +static void do_inc_thresh(unsigned char key, struct cpu_user_regs *regs) +{ + ++*lower_thresh_adj; + do_adj_thresh(key); +} + +static void do_dec_thresh(unsigned char key, struct cpu_user_regs *regs) +{ + if ( *lower_thresh_adj ) + --*lower_thresh_adj; + do_adj_thresh(key); +} + +static void __putstr(const char *); +static void printk_start_of_line(const char *); + +static void do_loglvl_op(const struct xen_sysctl_loglvl_thresh *op, + int *lower, int *upper, const char *which) +{ + if ( op->lower_thresh < 0 && op->upper_thresh < 0 ) + return; + + if ( op->lower_thresh >= 0 ) + *lower = op->lower_thresh; + + if ( op->upper_thresh >= 0 ) + *upper = op->upper_thresh; + + if ( *upper < *lower ) + { + if ( op->upper_thresh < 0 ) + *upper = *lower; + else + *lower = *upper; + } + + if ( printk_ratelimit() ) + { + spin_lock_irq(&console_lock); + printk_start_of_line("(XEN) "); + __putstr(which); + __putstr(" log level: "); + __putstr(loglvl_str(*lower)); + __putstr(" (rate limited "); + __putstr(loglvl_str(*upper)); + __putstr(")\n"); + spin_unlock_irq(&console_lock); + } +} + +int console_loglvl_op(struct xen_sysctl_loglvl_op *op) +{ + switch ( op->cmd ) + { + default: + return -EOPNOTSUPP; + + case XEN_SYSCTL_LOGLVL_set: + if ( (op->host.lower_thresh >= 0 && op->host.upper_thresh >= 0 && + op->host.lower_thresh > op->host.upper_thresh) || + (op->guest.lower_thresh >= 0 && op->guest.upper_thresh >= 0 && + op->guest.lower_thresh > op->guest.upper_thresh) ) + return -EINVAL; + + do_loglvl_op(&op->host, &xenlog_lower_thresh, + &xenlog_upper_thresh, "standard"); + do_loglvl_op(&op->guest, &xenlog_guest_lower_thresh, + &xenlog_guest_upper_thresh, "guest"); + + /* fall through */ + case XEN_SYSCTL_LOGLVL_get: + op->host.lower_thresh = xenlog_lower_thresh; + op->host.upper_thresh = xenlog_upper_thresh; + + op->guest.lower_thresh = xenlog_guest_lower_thresh; + op->guest.upper_thresh = xenlog_guest_upper_thresh; + + break; + } + + return 0; +} /* * ******************************************************** * *************** ACCESS TO CONSOLE RING ***************** @@ -833,6 +946,12 @@ void __init console_endboot(void) register_keyhandler('w', dump_console_ring_key, "synchronously dump console ring buffer (dmesg)", 0); + register_irq_keyhandler('+', &do_inc_thresh, + "increase log level threshold", 0); + register_irq_keyhandler('-', &do_dec_thresh, + "decrease log level threshold", 0); + register_irq_keyhandler('G', &do_toggle_guest, + "toggle host/guest log level adjustment", 0); /* Serial input is directed to DOM0 by default. */ switch_serial_input(); --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -766,6 +766,17 @@ struct xen_sysctl_tmem_op { typedef struct xen_sysctl_tmem_op xen_sysctl_tmem_op_t; DEFINE_XEN_GUEST_HANDLE(xen_sysctl_tmem_op_t); +/* XEN_SYSCTL_loglvl_op */ +#define XEN_SYSCTL_LOGLVL_get 0 +#define XEN_SYSCTL_LOGLVL_set 1 +struct xen_sysctl_loglvl_op { + uint32_t cmd; /* XEN_SYSCTL_LOGLVL_* */ + struct xen_sysctl_loglvl_thresh { + /* Negative values mean "no adjustment". */ + int32_t lower_thresh, upper_thresh; + } host, guest; +}; + struct xen_sysctl { uint32_t cmd; #define XEN_SYSCTL_readconsole 1 @@ -791,6 +802,7 @@ struct xen_sysctl { #define XEN_SYSCTL_pcitopoinfo 22 #define XEN_SYSCTL_psr_cat_op 23 #define XEN_SYSCTL_tmem_op 24 +#define XEN_SYSCTL_loglvl_op 25 uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */ union { struct xen_sysctl_readconsole readconsole; @@ -816,6 +828,7 @@ struct xen_sysctl { struct xen_sysctl_psr_cmt_op psr_cmt_op; struct xen_sysctl_psr_cat_op psr_cat_op; struct xen_sysctl_tmem_op tmem_op; + struct xen_sysctl_loglvl_op loglvl; uint8_t pad[128]; } u; }; --- a/xen/include/xen/console.h +++ b/xen/include/xen/console.h @@ -12,6 +12,8 @@ struct xen_sysctl_readconsole; long read_console_ring(struct xen_sysctl_readconsole *op); +struct xen_sysctl_loglvl_op; +int console_loglvl_op(struct xen_sysctl_loglvl_op *); void console_init_preirq(void); void console_init_ring(void); [-- Attachment #2: loglvl-adjust.patch --] [-- Type: text/plain, Size: 7365 bytes --] console: allow log level threshold adjustments ... from serial console and via sysctl so that one doesn't always need to reboot to see more / less messages. Note that upper thresholds driven from the serial console are sticky, i.e. while they get adjusted upwards when the lower threshold would otherwise end up above the upper one, they don't get adjusted when reducing the lower one. Full flexibility is available only via the sysctl interface. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Add sysctl. --- a/xen/common/sysctl.c +++ b/xen/common/sysctl.c @@ -460,6 +460,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe ret = tmem_control(&op->u.tmem_op); break; + case XEN_SYSCTL_loglvl_op: + ret = console_loglvl_op(&op->u.loglvl); + break; + default: ret = arch_do_sysctl(op, u_sysctl); copyback = 0; --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -168,7 +168,7 @@ static void __init parse_guest_loglvl(ch _parse_loglvl(s, &xenlog_guest_lower_thresh, &xenlog_guest_upper_thresh); } -static char * __init loglvl_str(int lvl) +static char *loglvl_str(int lvl) { switch ( lvl ) { @@ -181,6 +181,119 @@ static char * __init loglvl_str(int lvl) return "???"; } +static int *__read_mostly upper_thresh_adj = &xenlog_upper_thresh; +static int *__read_mostly lower_thresh_adj = &xenlog_lower_thresh; +static const char *__read_mostly thresh_adj = "standard"; + +static void do_toggle_guest(unsigned char key, struct cpu_user_regs *regs) +{ + if ( upper_thresh_adj == &xenlog_upper_thresh ) + { + upper_thresh_adj = &xenlog_guest_upper_thresh; + lower_thresh_adj = &xenlog_guest_lower_thresh; + thresh_adj = "guest"; + } + else + { + upper_thresh_adj = &xenlog_upper_thresh; + lower_thresh_adj = &xenlog_lower_thresh; + thresh_adj = "standard"; + } + printk("'%c' pressed -> %s log level adjustments enabled\n", + key, thresh_adj); +} + +static void do_adj_thresh(unsigned char key) +{ + if ( *upper_thresh_adj < *lower_thresh_adj ) + *upper_thresh_adj = *lower_thresh_adj; + printk("'%c' pressed -> %s log level: %s (rate limited %s)\n", + key, thresh_adj, loglvl_str(*lower_thresh_adj), + loglvl_str(*upper_thresh_adj)); +} + +static void do_inc_thresh(unsigned char key, struct cpu_user_regs *regs) +{ + ++*lower_thresh_adj; + do_adj_thresh(key); +} + +static void do_dec_thresh(unsigned char key, struct cpu_user_regs *regs) +{ + if ( *lower_thresh_adj ) + --*lower_thresh_adj; + do_adj_thresh(key); +} + +static void __putstr(const char *); +static void printk_start_of_line(const char *); + +static void do_loglvl_op(const struct xen_sysctl_loglvl_thresh *op, + int *lower, int *upper, const char *which) +{ + if ( op->lower_thresh < 0 && op->upper_thresh < 0 ) + return; + + if ( op->lower_thresh >= 0 ) + *lower = op->lower_thresh; + + if ( op->upper_thresh >= 0 ) + *upper = op->upper_thresh; + + if ( *upper < *lower ) + { + if ( op->upper_thresh < 0 ) + *upper = *lower; + else + *lower = *upper; + } + + if ( printk_ratelimit() ) + { + spin_lock_irq(&console_lock); + printk_start_of_line("(XEN) "); + __putstr(which); + __putstr(" log level: "); + __putstr(loglvl_str(*lower)); + __putstr(" (rate limited "); + __putstr(loglvl_str(*upper)); + __putstr(")\n"); + spin_unlock_irq(&console_lock); + } +} + +int console_loglvl_op(struct xen_sysctl_loglvl_op *op) +{ + switch ( op->cmd ) + { + default: + return -EOPNOTSUPP; + + case XEN_SYSCTL_LOGLVL_set: + if ( (op->host.lower_thresh >= 0 && op->host.upper_thresh >= 0 && + op->host.lower_thresh > op->host.upper_thresh) || + (op->guest.lower_thresh >= 0 && op->guest.upper_thresh >= 0 && + op->guest.lower_thresh > op->guest.upper_thresh) ) + return -EINVAL; + + do_loglvl_op(&op->host, &xenlog_lower_thresh, + &xenlog_upper_thresh, "standard"); + do_loglvl_op(&op->guest, &xenlog_guest_lower_thresh, + &xenlog_guest_upper_thresh, "guest"); + + /* fall through */ + case XEN_SYSCTL_LOGLVL_get: + op->host.lower_thresh = xenlog_lower_thresh; + op->host.upper_thresh = xenlog_upper_thresh; + + op->guest.lower_thresh = xenlog_guest_lower_thresh; + op->guest.upper_thresh = xenlog_guest_upper_thresh; + + break; + } + + return 0; +} /* * ******************************************************** * *************** ACCESS TO CONSOLE RING ***************** @@ -833,6 +946,12 @@ void __init console_endboot(void) register_keyhandler('w', dump_console_ring_key, "synchronously dump console ring buffer (dmesg)", 0); + register_irq_keyhandler('+', &do_inc_thresh, + "increase log level threshold", 0); + register_irq_keyhandler('-', &do_dec_thresh, + "decrease log level threshold", 0); + register_irq_keyhandler('G', &do_toggle_guest, + "toggle host/guest log level adjustment", 0); /* Serial input is directed to DOM0 by default. */ switch_serial_input(); --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -766,6 +766,17 @@ struct xen_sysctl_tmem_op { typedef struct xen_sysctl_tmem_op xen_sysctl_tmem_op_t; DEFINE_XEN_GUEST_HANDLE(xen_sysctl_tmem_op_t); +/* XEN_SYSCTL_loglvl_op */ +#define XEN_SYSCTL_LOGLVL_get 0 +#define XEN_SYSCTL_LOGLVL_set 1 +struct xen_sysctl_loglvl_op { + uint32_t cmd; /* XEN_SYSCTL_LOGLVL_* */ + struct xen_sysctl_loglvl_thresh { + /* Negative values mean "no adjustment". */ + int32_t lower_thresh, upper_thresh; + } host, guest; +}; + struct xen_sysctl { uint32_t cmd; #define XEN_SYSCTL_readconsole 1 @@ -791,6 +802,7 @@ struct xen_sysctl { #define XEN_SYSCTL_pcitopoinfo 22 #define XEN_SYSCTL_psr_cat_op 23 #define XEN_SYSCTL_tmem_op 24 +#define XEN_SYSCTL_loglvl_op 25 uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */ union { struct xen_sysctl_readconsole readconsole; @@ -816,6 +828,7 @@ struct xen_sysctl { struct xen_sysctl_psr_cmt_op psr_cmt_op; struct xen_sysctl_psr_cat_op psr_cat_op; struct xen_sysctl_tmem_op tmem_op; + struct xen_sysctl_loglvl_op loglvl; uint8_t pad[128]; } u; }; --- a/xen/include/xen/console.h +++ b/xen/include/xen/console.h @@ -12,6 +12,8 @@ struct xen_sysctl_readconsole; long read_console_ring(struct xen_sysctl_readconsole *op); +struct xen_sysctl_loglvl_op; +int console_loglvl_op(struct xen_sysctl_loglvl_op *); void console_init_preirq(void); void console_init_ring(void); [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/3] console: allow log level threshold adjustments 2016-03-04 16:46 ` [PATCH v2 1/3] console: allow " Jan Beulich @ 2016-03-04 20:55 ` Konrad Rzeszutek Wilk 2016-03-07 10:44 ` Jan Beulich 0 siblings, 1 reply; 37+ messages in thread From: Konrad Rzeszutek Wilk @ 2016-03-04 20:55 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Ian Jackson, Tim Deegan > +static void do_inc_thresh(unsigned char key, struct cpu_user_regs *regs) > +{ > + ++*lower_thresh_adj; > + do_adj_thresh(key); > +} > + > +static void do_dec_thresh(unsigned char key, struct cpu_user_regs *regs) > +{ > + if ( *lower_thresh_adj ) > + --*lower_thresh_adj; > + do_adj_thresh(key); > +} > + > +static void __putstr(const char *); > +static void printk_start_of_line(const char *); > + > +static void do_loglvl_op(const struct xen_sysctl_loglvl_thresh *op, > + int *lower, int *upper, const char *which) > +{ > + if ( op->lower_thresh < 0 && op->upper_thresh < 0 ) > + return; > + > + if ( op->lower_thresh >= 0 ) > + *lower = op->lower_thresh; > + > + if ( op->upper_thresh >= 0 ) > + *upper = op->upper_thresh; > + ..snip.. > + case XEN_SYSCTL_LOGLVL_set: > + if ( (op->host.lower_thresh >= 0 && op->host.upper_thresh >= 0 && > + op->host.lower_thresh > op->host.upper_thresh) || > + (op->guest.lower_thresh >= 0 && op->guest.upper_thresh >= 0 && > + op->guest.lower_thresh > op->guest.upper_thresh) ) > + return -EINVAL; > + > + do_loglvl_op(&op->host, &xenlog_lower_thresh, > + &xenlog_upper_thresh, "standard"); The keyboard and the sysctl both allow the user to go beyound the XENLOG_ values we have. That is you could set the lower and upper threshold to be at 9 (or more) say. It will have the same effect as XENLOG_DEBUG (which is 4) as printk_prefix_check seems to have a simple < check. But perhaps to be correct only accept only proper values? Not allow the system admin to set the level to say 31415? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/3] console: allow log level threshold adjustments 2016-03-04 20:55 ` Konrad Rzeszutek Wilk @ 2016-03-07 10:44 ` Jan Beulich 2016-03-07 14:41 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 37+ messages in thread From: Jan Beulich @ 2016-03-07 10:44 UTC (permalink / raw) To: Konrad Rzeszutek Wilk; +Cc: xen-devel, Keir Fraser, Ian Jackson, Tim Deegan >>> On 04.03.16 at 21:55, <konrad.wilk@oracle.com> wrote: >> + case XEN_SYSCTL_LOGLVL_set: >> + if ( (op->host.lower_thresh >= 0 && op->host.upper_thresh >= 0 && >> + op->host.lower_thresh > op->host.upper_thresh) || >> + (op->guest.lower_thresh >= 0 && op->guest.upper_thresh >= 0 && >> + op->guest.lower_thresh > op->guest.upper_thresh) ) >> + return -EINVAL; >> + >> + do_loglvl_op(&op->host, &xenlog_lower_thresh, >> + &xenlog_upper_thresh, "standard"); > > > The keyboard and the sysctl both allow the user to go beyound the XENLOG_ > values we have. That is you could set the lower and upper threshold to be > at 9 (or more) say. It will have the same effect as XENLOG_DEBUG (which is > 4) > as printk_prefix_check seems to have a simple < check. > > But perhaps to be correct only accept only proper values? Not allow > the system admin to set the level to say 31415? Since there's no bad side effect from doing so I opted for not adding respective extra checks, keeping the code easier to read. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/3] console: allow log level threshold adjustments 2016-03-07 10:44 ` Jan Beulich @ 2016-03-07 14:41 ` Konrad Rzeszutek Wilk 2016-03-07 15:19 ` Jan Beulich 0 siblings, 1 reply; 37+ messages in thread From: Konrad Rzeszutek Wilk @ 2016-03-07 14:41 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Tim Deegan, Keir Fraser, Ian Jackson On Mon, Mar 7, 2016 at 5:44 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 04.03.16 at 21:55, <konrad.wilk@oracle.com> wrote: >>> + case XEN_SYSCTL_LOGLVL_set: >>> + if ( (op->host.lower_thresh >= 0 && op->host.upper_thresh >= 0 && >>> + op->host.lower_thresh > op->host.upper_thresh) || >>> + (op->guest.lower_thresh >= 0 && op->guest.upper_thresh >= 0 && >>> + op->guest.lower_thresh > op->guest.upper_thresh) ) >>> + return -EINVAL; >>> + >>> + do_loglvl_op(&op->host, &xenlog_lower_thresh, >>> + &xenlog_upper_thresh, "standard"); >> >> >> The keyboard and the sysctl both allow the user to go beyound the XENLOG_ >> values we have. That is you could set the lower and upper threshold to be >> at 9 (or more) say. It will have the same effect as XENLOG_DEBUG (which is >> 4) >> as printk_prefix_check seems to have a simple < check. >> >> But perhaps to be correct only accept only proper values? Not allow >> the system admin to set the level to say 31415? > > Since there's no bad side effect from doing so I opted for not > adding respective extra checks, keeping the code easier to read. > Fair enough. Could you perhaps just add that in the commit description? Also I noticed that this patch is missing an XSM check in flask_sysctl - could that be added please? > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 1/3] console: allow log level threshold adjustments 2016-03-07 14:41 ` Konrad Rzeszutek Wilk @ 2016-03-07 15:19 ` Jan Beulich 0 siblings, 0 replies; 37+ messages in thread From: Jan Beulich @ 2016-03-07 15:19 UTC (permalink / raw) To: Konrad Rzeszutek Wilk; +Cc: xen-devel, Tim Deegan, Keir Fraser, Ian Jackson >>> On 07.03.16 at 15:41, <konrad@kernel.org> wrote: > On Mon, Mar 7, 2016 at 5:44 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 04.03.16 at 21:55, <konrad.wilk@oracle.com> wrote: >>>> + case XEN_SYSCTL_LOGLVL_set: >>>> + if ( (op->host.lower_thresh >= 0 && op->host.upper_thresh >= 0 && >>>> + op->host.lower_thresh > op->host.upper_thresh) || >>>> + (op->guest.lower_thresh >= 0 && op->guest.upper_thresh >= 0 && >>>> + op->guest.lower_thresh > op->guest.upper_thresh) ) >>>> + return -EINVAL; >>>> + >>>> + do_loglvl_op(&op->host, &xenlog_lower_thresh, >>>> + &xenlog_upper_thresh, "standard"); >>> >>> >>> The keyboard and the sysctl both allow the user to go beyound the XENLOG_ >>> values we have. That is you could set the lower and upper threshold to be >>> at 9 (or more) say. It will have the same effect as XENLOG_DEBUG (which is >>> 4) >>> as printk_prefix_check seems to have a simple < check. >>> >>> But perhaps to be correct only accept only proper values? Not allow >>> the system admin to set the level to say 31415? >> >> Since there's no bad side effect from doing so I opted for not >> adding respective extra checks, keeping the code easier to read. >> > > Fair enough. Could you perhaps just add that in the commit description? Sure. > Also I noticed that this patch is missing an XSM check in flask_sysctl > - could that be added please? Of course; it's pretty ugly that one doesn't notice the lack thereof via a build failure. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 2/3] libxc: wrapper for log level sysctl 2016-03-04 16:38 [PATCH v2 0/3] allow runtime log level threshold adjustments Jan Beulich 2016-03-04 16:46 ` [PATCH v2 1/3] console: allow " Jan Beulich @ 2016-03-04 16:47 ` Jan Beulich 2016-03-05 16:00 ` Dario Faggioli 2016-03-08 16:20 ` Wei Liu 2016-03-04 16:48 ` [PATCH v2 3/3] xl: new "loglvl" command Jan Beulich 2 siblings, 2 replies; 37+ messages in thread From: Jan Beulich @ 2016-03-04 16:47 UTC (permalink / raw) To: xen-devel; +Cc: Ian Jackson, Wei Liu, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 2275 bytes --] Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -1120,6 +1120,11 @@ int xc_readconsolering(xc_interface *xch int xc_send_debug_keys(xc_interface *xch, char *keys); +int xc_get_log_level(xc_interface *xch, bool guest, + int *lower_thresh, int *upper_thresh); +int xc_set_log_level(xc_interface *xch, bool guest, + int lower_thresh, int upper_thresh); + typedef xen_sysctl_physinfo_t xc_physinfo_t; typedef xen_sysctl_cputopo_t xc_cputopo_t; typedef xen_sysctl_numainfo_t xc_numainfo_t; --- a/tools/libxc/xc_misc.c +++ b/tools/libxc/xc_misc.c @@ -184,6 +184,55 @@ int xc_send_debug_keys(xc_interface *xch return ret; } +int xc_get_log_level(xc_interface *xch, bool guest, + int *lower_thresh, int *upper_thresh) +{ + int ret; + DECLARE_SYSCTL; + + sysctl.cmd = XEN_SYSCTL_loglvl_op; + sysctl.u.loglvl.cmd = XEN_SYSCTL_LOGLVL_get; + ret = do_sysctl(xch, &sysctl); + + if ( guest ) + { + *lower_thresh = sysctl.u.loglvl.guest.lower_thresh; + *upper_thresh = sysctl.u.loglvl.guest.upper_thresh; + } + else + { + *lower_thresh = sysctl.u.loglvl.host.lower_thresh; + *upper_thresh = sysctl.u.loglvl.host.upper_thresh; + } + + return ret; +} + +int xc_set_log_level(xc_interface *xch, bool guest, + int lower_thresh, int upper_thresh) +{ + DECLARE_SYSCTL; + + sysctl.cmd = XEN_SYSCTL_loglvl_op; + sysctl.u.loglvl.cmd = XEN_SYSCTL_LOGLVL_set; + if ( guest ) + { + sysctl.u.loglvl.host.lower_thresh = -1; + sysctl.u.loglvl.host.upper_thresh = -1; + sysctl.u.loglvl.guest.lower_thresh = lower_thresh; + sysctl.u.loglvl.guest.upper_thresh = upper_thresh; + } + else + { + sysctl.u.loglvl.host.lower_thresh = lower_thresh; + sysctl.u.loglvl.host.upper_thresh = upper_thresh; + sysctl.u.loglvl.guest.lower_thresh = -1; + sysctl.u.loglvl.guest.upper_thresh = -1; + } + + return do_sysctl(xch, &sysctl); +} + int xc_physinfo(xc_interface *xch, xc_physinfo_t *put_info) { [-- Attachment #2: loglvl-adjust-libxc.patch --] [-- Type: text/plain, Size: 2308 bytes --] libxc: wrapper for log level sysctl Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -1120,6 +1120,11 @@ int xc_readconsolering(xc_interface *xch int xc_send_debug_keys(xc_interface *xch, char *keys); +int xc_get_log_level(xc_interface *xch, bool guest, + int *lower_thresh, int *upper_thresh); +int xc_set_log_level(xc_interface *xch, bool guest, + int lower_thresh, int upper_thresh); + typedef xen_sysctl_physinfo_t xc_physinfo_t; typedef xen_sysctl_cputopo_t xc_cputopo_t; typedef xen_sysctl_numainfo_t xc_numainfo_t; --- a/tools/libxc/xc_misc.c +++ b/tools/libxc/xc_misc.c @@ -184,6 +184,55 @@ int xc_send_debug_keys(xc_interface *xch return ret; } +int xc_get_log_level(xc_interface *xch, bool guest, + int *lower_thresh, int *upper_thresh) +{ + int ret; + DECLARE_SYSCTL; + + sysctl.cmd = XEN_SYSCTL_loglvl_op; + sysctl.u.loglvl.cmd = XEN_SYSCTL_LOGLVL_get; + ret = do_sysctl(xch, &sysctl); + + if ( guest ) + { + *lower_thresh = sysctl.u.loglvl.guest.lower_thresh; + *upper_thresh = sysctl.u.loglvl.guest.upper_thresh; + } + else + { + *lower_thresh = sysctl.u.loglvl.host.lower_thresh; + *upper_thresh = sysctl.u.loglvl.host.upper_thresh; + } + + return ret; +} + +int xc_set_log_level(xc_interface *xch, bool guest, + int lower_thresh, int upper_thresh) +{ + DECLARE_SYSCTL; + + sysctl.cmd = XEN_SYSCTL_loglvl_op; + sysctl.u.loglvl.cmd = XEN_SYSCTL_LOGLVL_set; + if ( guest ) + { + sysctl.u.loglvl.host.lower_thresh = -1; + sysctl.u.loglvl.host.upper_thresh = -1; + sysctl.u.loglvl.guest.lower_thresh = lower_thresh; + sysctl.u.loglvl.guest.upper_thresh = upper_thresh; + } + else + { + sysctl.u.loglvl.host.lower_thresh = lower_thresh; + sysctl.u.loglvl.host.upper_thresh = upper_thresh; + sysctl.u.loglvl.guest.lower_thresh = -1; + sysctl.u.loglvl.guest.upper_thresh = -1; + } + + return do_sysctl(xch, &sysctl); +} + int xc_physinfo(xc_interface *xch, xc_physinfo_t *put_info) { [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 2/3] libxc: wrapper for log level sysctl 2016-03-04 16:47 ` [PATCH v2 2/3] libxc: wrapper for log level sysctl Jan Beulich @ 2016-03-05 16:00 ` Dario Faggioli 2016-03-08 16:20 ` Wei Liu 1 sibling, 0 replies; 37+ messages in thread From: Dario Faggioli @ 2016-03-05 16:00 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Wei Liu, Ian Jackson, Stefano Stabellini [-- Attachment #1.1: Type: text/plain, Size: 447 bytes --] On Fri, 2016-03-04 at 09:47 -0700, Jan Beulich wrote: > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com> 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) [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 181 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 2/3] libxc: wrapper for log level sysctl 2016-03-04 16:47 ` [PATCH v2 2/3] libxc: wrapper for log level sysctl Jan Beulich 2016-03-05 16:00 ` Dario Faggioli @ 2016-03-08 16:20 ` Wei Liu 1 sibling, 0 replies; 37+ messages in thread From: Wei Liu @ 2016-03-08 16:20 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Wei Liu, Ian Jackson, Stefano Stabellini On Fri, Mar 04, 2016 at 09:47:32AM -0700, Jan Beulich wrote: > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Acked-by: Wei Liu <wei.liu2@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 3/3] xl: new "loglvl" command 2016-03-04 16:38 [PATCH v2 0/3] allow runtime log level threshold adjustments Jan Beulich 2016-03-04 16:46 ` [PATCH v2 1/3] console: allow " Jan Beulich 2016-03-04 16:47 ` [PATCH v2 2/3] libxc: wrapper for log level sysctl Jan Beulich @ 2016-03-04 16:48 ` Jan Beulich 2016-03-04 18:45 ` Dario Faggioli ` (3 more replies) 2 siblings, 4 replies; 37+ messages in thread From: Jan Beulich @ 2016-03-04 16:48 UTC (permalink / raw) To: xen-devel; +Cc: Ian Jackson, Wei Liu, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 4867 bytes --] 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" }, +}; + +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", [-- Attachment #2: loglvl-adjust-xl.patch --] [-- Type: text/plain, Size: 4891 bytes --] xl: new "loglvl" command 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" }, +}; + +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", [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-04 16:48 ` [PATCH v2 3/3] xl: new "loglvl" command Jan Beulich @ 2016-03-04 18:45 ` Dario Faggioli 2016-03-07 11:46 ` Jan Beulich 2016-03-05 15:36 ` Dario Faggioli ` (2 subsequent siblings) 3 siblings, 1 reply; 37+ messages in thread From: Dario Faggioli @ 2016-03-04 18:45 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Wei Liu, Ian Jackson, Stefano Stabellini [-- Attachment #1.1: Type: text/plain, Size: 3302 bytes --] 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 -- <<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) [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 181 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-04 18:45 ` Dario Faggioli @ 2016-03-07 11:46 ` Jan Beulich 2016-03-07 18:07 ` Dario Faggioli 0 siblings, 1 reply; 37+ messages in thread From: Jan Beulich @ 2016-03-07 11:46 UTC (permalink / raw) To: Dario Faggioli; +Cc: Ian Jackson, Wei Liu, xen-devel, Stefano Stabellini >>> 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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-07 11:46 ` Jan Beulich @ 2016-03-07 18:07 ` Dario Faggioli 2016-03-08 8:08 ` Jan Beulich 0 siblings, 1 reply; 37+ messages in thread From: Dario Faggioli @ 2016-03-07 18:07 UTC (permalink / raw) To: Jan Beulich; +Cc: Ian Jackson, Wei Liu, xen-devel, Stefano Stabellini [-- Attachment #1.1: Type: text/plain, Size: 1898 bytes --] 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 -- <<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) [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 181 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-07 18:07 ` Dario Faggioli @ 2016-03-08 8:08 ` Jan Beulich 2016-03-08 14:05 ` George Dunlap 0 siblings, 1 reply; 37+ messages in thread From: Jan Beulich @ 2016-03-08 8:08 UTC (permalink / raw) To: Dario Faggioli; +Cc: Ian Jackson, Wei Liu, xen-devel, Stefano Stabellini >>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-08 8:08 ` Jan Beulich @ 2016-03-08 14:05 ` George Dunlap 2016-03-08 16:09 ` Wei Liu 2016-03-08 18:05 ` Dario Faggioli 0 siblings, 2 replies; 37+ messages in thread From: George Dunlap @ 2016-03-08 14:05 UTC (permalink / raw) To: Jan Beulich Cc: Wei Liu, Dario Faggioli, Stefano Stabellini, Ian Jackson, xen-devel 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-08 14:05 ` George Dunlap @ 2016-03-08 16:09 ` Wei Liu 2016-03-08 18:05 ` Dario Faggioli 1 sibling, 0 replies; 37+ messages in thread From: Wei Liu @ 2016-03-08 16:09 UTC (permalink / raw) To: George Dunlap Cc: Wei Liu, Stefano Stabellini, Dario Faggioli, Ian Jackson, Jan Beulich, xen-devel 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-08 14:05 ` George Dunlap 2016-03-08 16:09 ` Wei Liu @ 2016-03-08 18:05 ` Dario Faggioli 1 sibling, 0 replies; 37+ messages in thread From: Dario Faggioli @ 2016-03-08 18:05 UTC (permalink / raw) To: George Dunlap, Jan Beulich Cc: Wei Liu, Stefano Stabellini, Ian Jackson, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 2194 bytes --] 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 -- <<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) [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 181 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-04 16:48 ` [PATCH v2 3/3] xl: new "loglvl" command Jan Beulich 2016-03-04 18:45 ` Dario Faggioli @ 2016-03-05 15:36 ` Dario Faggioli 2016-03-07 13:20 ` Fabio Fantoni 2016-03-08 16:20 ` Wei Liu 3 siblings, 0 replies; 37+ messages in thread From: Dario Faggioli @ 2016-03-05 15:36 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Wei Liu, Ian Jackson, Stefano Stabellini [-- Attachment #1.1: Type: text/plain, Size: 1243 bytes --] 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 -- <<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) [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 181 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-04 16:48 ` [PATCH v2 3/3] xl: new "loglvl" command Jan Beulich 2016-03-04 18:45 ` Dario Faggioli 2016-03-05 15:36 ` Dario Faggioli @ 2016-03-07 13:20 ` Fabio Fantoni 2016-03-07 13:26 ` Jan Beulich 2016-03-08 16:20 ` Wei Liu 3 siblings, 1 reply; 37+ messages in thread From: Fabio Fantoni @ 2016-03-07 13:20 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Wei Liu, Ian Jackson, Stefano Stabellini 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", > > > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-07 13:20 ` Fabio Fantoni @ 2016-03-07 13:26 ` Jan Beulich 0 siblings, 0 replies; 37+ messages in thread From: Jan Beulich @ 2016-03-07 13:26 UTC (permalink / raw) To: Fabio Fantoni; +Cc: Ian Jackson, Wei Liu, xen-devel, Stefano Stabellini >>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-04 16:48 ` [PATCH v2 3/3] xl: new "loglvl" command Jan Beulich ` (2 preceding siblings ...) 2016-03-07 13:20 ` Fabio Fantoni @ 2016-03-08 16:20 ` Wei Liu 2016-03-14 15:23 ` Jan Beulich 3 siblings, 1 reply; 37+ messages in thread From: Wei Liu @ 2016-03-08 16:20 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Wei Liu, Ian Jackson, Stefano Stabellini 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-08 16:20 ` Wei Liu @ 2016-03-14 15:23 ` Jan Beulich 2016-03-14 15:36 ` Wei Liu 0 siblings, 1 reply; 37+ messages in thread From: Jan Beulich @ 2016-03-14 15:23 UTC (permalink / raw) To: Wei Liu; +Cc: xen-devel, Ian Jackson, Stefano Stabellini >>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-14 15:23 ` Jan Beulich @ 2016-03-14 15:36 ` Wei Liu 2016-03-14 15:49 ` Jan Beulich 0 siblings, 1 reply; 37+ messages in thread From: Wei Liu @ 2016-03-14 15:36 UTC (permalink / raw) To: Jan Beulich; +Cc: Ian Jackson, Wei Liu, xen-devel, Stefano Stabellini 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 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-14 15:36 ` Wei Liu @ 2016-03-14 15:49 ` Jan Beulich 2016-03-14 16:01 ` Wei Liu 0 siblings, 1 reply; 37+ messages in thread From: Jan Beulich @ 2016-03-14 15:49 UTC (permalink / raw) To: Wei Liu; +Cc: xen-devel, Ian Jackson, Stefano Stabellini >>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-14 15:49 ` Jan Beulich @ 2016-03-14 16:01 ` Wei Liu 2016-03-14 17:00 ` Jan Beulich 0 siblings, 1 reply; 37+ messages in thread From: Wei Liu @ 2016-03-14 16:01 UTC (permalink / raw) To: Jan Beulich; +Cc: Ian Jackson, Wei Liu, xen-devel, Stefano Stabellini 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 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-14 16:01 ` Wei Liu @ 2016-03-14 17:00 ` Jan Beulich 2016-03-14 17:07 ` Ian Jackson 0 siblings, 1 reply; 37+ messages in thread From: Jan Beulich @ 2016-03-14 17:00 UTC (permalink / raw) To: Wei Liu; +Cc: xen-devel, Ian Jackson, Stefano Stabellini >>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-14 17:00 ` Jan Beulich @ 2016-03-14 17:07 ` Ian Jackson 2016-03-15 7:37 ` Jan Beulich 0 siblings, 1 reply; 37+ messages in thread From: Ian Jackson @ 2016-03-14 17:07 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Wei Liu, Stefano Stabellini 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-14 17:07 ` Ian Jackson @ 2016-03-15 7:37 ` Jan Beulich 2016-03-15 13:58 ` Wei Liu 0 siblings, 1 reply; 37+ messages in thread From: Jan Beulich @ 2016-03-15 7:37 UTC (permalink / raw) To: Ian Jackson; +Cc: xen-devel, Wei Liu, Stefano Stabellini >>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-15 7:37 ` Jan Beulich @ 2016-03-15 13:58 ` Wei Liu 2016-03-15 14:07 ` Jan Beulich 0 siblings, 1 reply; 37+ messages in thread From: Wei Liu @ 2016-03-15 13:58 UTC (permalink / raw) To: Jan Beulich; +Cc: Wei Liu, Ian Jackson, xen-devel, Stefano Stabellini 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 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-15 13:58 ` Wei Liu @ 2016-03-15 14:07 ` Jan Beulich 2016-03-15 14:51 ` Wei Liu 2016-03-15 15:38 ` Ian Jackson 0 siblings, 2 replies; 37+ messages in thread From: Jan Beulich @ 2016-03-15 14:07 UTC (permalink / raw) To: Wei Liu; +Cc: xen-devel, Ian Jackson, Stefano Stabellini >>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-15 14:07 ` Jan Beulich @ 2016-03-15 14:51 ` Wei Liu 2016-03-15 15:03 ` Jan Beulich 2016-03-15 15:38 ` Ian Jackson 1 sibling, 1 reply; 37+ messages in thread From: Wei Liu @ 2016-03-15 14:51 UTC (permalink / raw) To: Jan Beulich; +Cc: Ian Jackson, Wei Liu, xen-devel, Stefano Stabellini 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 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-15 14:51 ` Wei Liu @ 2016-03-15 15:03 ` Jan Beulich 0 siblings, 0 replies; 37+ messages in thread From: Jan Beulich @ 2016-03-15 15:03 UTC (permalink / raw) To: Wei Liu; +Cc: xen-devel, Ian Jackson, Stefano Stabellini >>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-15 14:07 ` Jan Beulich 2016-03-15 14:51 ` Wei Liu @ 2016-03-15 15:38 ` Ian Jackson 2016-03-16 11:22 ` Jan Beulich 1 sibling, 1 reply; 37+ messages in thread From: Ian Jackson @ 2016-03-15 15:38 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Wei Liu, Stefano Stabellini 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-15 15:38 ` Ian Jackson @ 2016-03-16 11:22 ` Jan Beulich 2016-04-28 15:33 ` Wei Liu 0 siblings, 1 reply; 37+ messages in thread From: Jan Beulich @ 2016-03-16 11:22 UTC (permalink / raw) To: Ian Jackson; +Cc: xen-devel, Wei Liu, Stefano Stabellini >>> 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-03-16 11:22 ` Jan Beulich @ 2016-04-28 15:33 ` Wei Liu 2016-04-29 7:20 ` Jan Beulich 0 siblings, 1 reply; 37+ messages in thread From: Wei Liu @ 2016-04-28 15:33 UTC (permalink / raw) To: Jan Beulich; +Cc: Wei Liu, Ian Jackson, xen-devel, Stefano Stabellini 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-04-28 15:33 ` Wei Liu @ 2016-04-29 7:20 ` Jan Beulich 2016-05-02 11:14 ` Wei Liu 0 siblings, 1 reply; 37+ messages in thread From: Jan Beulich @ 2016-04-29 7:20 UTC (permalink / raw) To: Wei Liu; +Cc: xen-devel, Ian Jackson, Stefano Stabellini >>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/3] xl: new "loglvl" command 2016-04-29 7:20 ` Jan Beulich @ 2016-05-02 11:14 ` Wei Liu 0 siblings, 0 replies; 37+ messages in thread From: Wei Liu @ 2016-05-02 11:14 UTC (permalink / raw) To: Jan Beulich; +Cc: Ian Jackson, Wei Liu, xen-devel, Stefano Stabellini 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 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2016-05-02 11:14 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-04 16:38 [PATCH v2 0/3] allow runtime log level threshold adjustments Jan Beulich 2016-03-04 16:46 ` [PATCH v2 1/3] console: allow " Jan Beulich 2016-03-04 20:55 ` Konrad Rzeszutek Wilk 2016-03-07 10:44 ` Jan Beulich 2016-03-07 14:41 ` Konrad Rzeszutek Wilk 2016-03-07 15:19 ` Jan Beulich 2016-03-04 16:47 ` [PATCH v2 2/3] libxc: wrapper for log level sysctl Jan Beulich 2016-03-05 16:00 ` Dario Faggioli 2016-03-08 16:20 ` Wei Liu 2016-03-04 16:48 ` [PATCH v2 3/3] xl: new "loglvl" command Jan Beulich 2016-03-04 18:45 ` Dario Faggioli 2016-03-07 11:46 ` Jan Beulich 2016-03-07 18:07 ` Dario Faggioli 2016-03-08 8:08 ` Jan Beulich 2016-03-08 14:05 ` George Dunlap 2016-03-08 16:09 ` Wei Liu 2016-03-08 18:05 ` Dario Faggioli 2016-03-05 15:36 ` Dario Faggioli 2016-03-07 13:20 ` Fabio Fantoni 2016-03-07 13:26 ` Jan Beulich 2016-03-08 16:20 ` Wei Liu 2016-03-14 15:23 ` Jan Beulich 2016-03-14 15:36 ` Wei Liu 2016-03-14 15:49 ` Jan Beulich 2016-03-14 16:01 ` Wei Liu 2016-03-14 17:00 ` Jan Beulich 2016-03-14 17:07 ` Ian Jackson 2016-03-15 7:37 ` Jan Beulich 2016-03-15 13:58 ` Wei Liu 2016-03-15 14:07 ` Jan Beulich 2016-03-15 14:51 ` Wei Liu 2016-03-15 15:03 ` Jan Beulich 2016-03-15 15:38 ` Ian Jackson 2016-03-16 11:22 ` Jan Beulich 2016-04-28 15:33 ` Wei Liu 2016-04-29 7:20 ` Jan Beulich 2016-05-02 11:14 ` Wei Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).