xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [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

* [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

* [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 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 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 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 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 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-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 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

* 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 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

* 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 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-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).