[ovs-dev] [PATCH] dpdk: Add commands to configure log levels.

[David Marchand]

On Thu, Jul 2, 2020 at 12:05 AM Ilya Maximets <i.maximets at ovn.org> wrote:
> On 6/26/20 2:27 PM, David Marchand wrote:
> > Enabling debug logs in dpdk can be a challenge to be sure of what is
> > actually enabled, add commands to list and change those log levels.
>
> Could you, please, provide some usage examples?
> Maybe add some documentation about these commands.  And a NEWS update
> since this is a user visible change.

In the documentation, example outputs could get outdated since we
directly pass dpdk output (/me still wants to shoot the dpdk global
log level that is just useless).
But I think it is still worth adding.

I would see either in Documentation/howto/dpdk.rst or
Documentation/intro/install/dpdk.rst.
The latter seems the best fit: we have setup instructions, configuring
logs is close.
WDYT?

[snip]

> > @@ -261,6 +262,102 @@ static cookie_io_functions_t dpdk_log_func = {
> >      .write = dpdk_log_write,
> >  };
> >
> > +static void
> > +dpdk_unixctl_log_list(struct unixctl_conn *conn, int argc OVS_UNUSED,
> > +                      const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> > +{
> > +    char *response = NULL;
> > +    FILE *stream;
> > +    size_t size;
> > +
> > +    stream = open_memstream(&response, &size);
> > +    if (!stream) {
> > +        response = xasprintf("Unable to open memstream: %s.",
> > +                             ovs_strerror(errno));
> > +        unixctl_command_reply_error(conn, response);
> > +        goto out;
> > +    }
> > +
> > +    rte_log_dump(stream);
>
> This function is fine, however, I see that you're adding almost same function
> do dump lcores in a later patch.  Maybe we could rename this one to
> 'dpdk_unixctl_memstream' and pass 'rte_log_dump' function pointer via 'aux'
> argument.  This will allow us to easily add this kind of unixctl calls.

dpdk "dump" apis tend to have the same prototype, so yes it will save some code.

[snip]

> > +
> > +            unixctl_command_reply_error(conn, msg);
> > +            free(s);
> > +            free(msg);
> > +            return;
> > +        }
> > +
> > +        if (rte_log_set_level_pattern(pattern, level) < 0) {
> > +            char *msg = xasprintf("cannot set log level for %s", argv[i]);
> > +
> > +            unixctl_command_reply_error(conn, msg);
> > +            free(s);
> > +            free(msg);
> > +            return;
>
> This 4 lines repeated twice.  Maybe combine them somehow?
> Something like this:
>
>     char *err_msg = NULL;
>     ...
>     if (level == -1) {
>         err_msg = xasprintf("invalid log level: \'%s\'", level_string);

No need for escaping?


Thanks for the review.

-- 
David Marchand