Re: [Xen-devel] [PATCH v2 2/5] xen/console: allow log level threshold adjustments

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.07.16 at 17:13, <wei.liu2@xxxxxxxxxx> wrote:

In case this (or a variant thereof) is to be used despite the earlier
voiced concerns, a couple of mechanical comments:

> +static const char *loglvl_to_str(int lvl)
> +{
> +    unsigned int i;
> +
> +    if ( lvl < LOG_LEVEL_MIN || lvl > LOG_LEVEL_MAX )
> +        return "???";
> +
> +    /* Multiple log levels can use the same number. Return the most
> +     * comprehensive log level string.
> +     */

Comment style.

> +    for ( i = ARRAY_SIZE(log_levels) - 1; i >= 0; i-- )

Possibly infinite loop - i is unsigned and hence always non-negative.

> +int console_loglvl_op(struct xen_sysctl_loglvl_op *op)
> +{
> +    int ret;
> +
> +    switch ( op->cmd )
> +    {
> +    default:
> +        ret = -EOPNOTSUPP;
> +        break;
> +
> +    case XEN_SYSCTL_LOGLVL_set:
> +    {
> +        char *buf;
> +        unsigned int buf_size = 0;

Pointless initializer.

> +        int host_lower_thresh, host_upper_thresh;
> +        int guest_lower_thresh, guest_upper_thresh;
> +
> +        buf_size = op->host.lower_thresh_len;
> +        if ( buf_size < op->host.upper_thresh_len )
> +            buf_size = op->host.upper_thresh_len;
> +        if ( buf_size < op->guest.lower_thresh_len )
> +            buf_size = op->guest.lower_thresh_len;
> +        if ( buf_size < op->guest.upper_thresh_len )
> +            buf_size = op->guest.upper_thresh_len;
> +
> +        buf = xmalloc_array(char, buf_size);
> +        if ( !buf )
> +        {
> +            ret = -ENOMEM;
> +            goto set_done;
> +        }
> +
> +#define parse(hg, lu)                                                   \
> +        hg##_##lu##_thresh = -1;                                        \
> +        if ( op->hg.lu ##_thresh_len )                                  \
> +        {                                                               \
> +            if ( copy_from_guest(buf, op->hg.lu ##_thresh,              \
> +                                 op->hg.lu ##_thresh_len) )             \
> +            {                                                           \
> +                ret = -EFAULT;                                          \
> +                goto set_done;                                          \
> +            }                                                           \
> +                                                                        \
> +            buf[buf_size-1] = 0;                                        \
> +                                                                        \
> +            ret = parse_log_level(buf);                                 \
> +            if ( ret == -1 )                                            \
> +            {                                                           \
> +                ret = -EINVAL;                                          \
> +                goto set_done;                                          \
> +            }                                                           \
> +                                                                        \
> +            hg##_##lu##_thresh = ret;                                   \
> +        }
> +
> +        parse(host,  lower);
> +        parse(host,  upper);
> +        parse(guest, lower);
> +        parse(guest, upper);
> +
> +#undef parse
> +
> +        if ( (host_lower_thresh >= 0 && host_upper_thresh >= 0 &&
> +              host_lower_thresh > host_upper_thresh) ||
> +             (guest_lower_thresh >= 0 && guest_upper_thresh >= 0 &&
> +              guest_lower_thresh > guest_upper_thresh) )
> +        {
> +            ret = -EINVAL;
> +            goto set_done;
> +        }
> +
> +        do_loglvl_op(host_lower_thresh, host_upper_thresh,
> +                     &xenlog_lower_thresh, &xenlog_upper_thresh,
> +                     "standard");
> +
> +        do_loglvl_op(guest_lower_thresh, guest_upper_thresh,
> +                     &xenlog_guest_lower_thresh, &xenlog_guest_upper_thresh,
> +                     "guest");
> +
> +        ret = 0;
> +    set_done:
> +        xfree(buf);
> +        break;
> +    }
> +    case XEN_SYSCTL_LOGLVL_get:

Blank line ahead of the case label please.

> +    {
> +        unsigned int host_lower_thresh_len =
> +            strlen(loglvl_to_str(xenlog_lower_thresh)) + 1;
> +        unsigned int host_upper_thresh_len =
> +            strlen(loglvl_to_str(xenlog_upper_thresh)) + 1;
> +        unsigned int guest_lower_thresh_len =
> +            strlen(loglvl_to_str(xenlog_guest_lower_thresh)) + 1;
> +        unsigned int guest_upper_thresh_len =
> +            strlen(loglvl_to_str(xenlog_guest_upper_thresh)) + 1;
> +        char scratch[LOG_LEVEL_STRLEN_MAX+1];
> +
> +        if ( (op->host.lower_thresh_len &&
> +              op->host.lower_thresh_len < host_lower_thresh_len) ||
> +             (op->host.upper_thresh_len &&
> +              op->host.upper_thresh_len < host_upper_thresh_len) ||
> +             (op->guest.lower_thresh_len &&
> +              op->guest.lower_thresh_len < guest_lower_thresh_len) ||
> +             (op->guest.upper_thresh_len
> +              && op->guest.upper_thresh_len < guest_upper_thresh_len)
> +            )
> +        {
> +            ret = -ENOBUFS;
> +            goto get_done;
> +        }
> +
> +        ret = -EFAULT;
> +
> +        if ( op->host.lower_thresh_len )
> +        {
> +            snprintf(scratch, sizeof(scratch), "%s",
> +                     loglvl_to_str(xenlog_lower_thresh));
> +            if ( copy_to_guest(op->host.lower_thresh, scratch,
> +                               strlen(scratch)+1) )
> +                goto get_done;

Why the indirection through snprintf()?

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1031,6 +1031,45 @@ struct xen_sysctl_livepatch_op {
>  typedef struct xen_sysctl_livepatch_op xen_sysctl_livepatch_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_livepatch_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 {

Explicit padding please ahead of this struct.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel