On 7/2/20 10:25 AM, David Marchand wrote:
> On Thu, Jul 2, 2020 at 12:05 AM Ilya Maximets <[email protected]> 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?
install/dpdk.rst might be more appropriate.
However, maybe it's enough to just create lib/dpdk-unixctl.man similar
to lib/netdev-dpdk-unixctl.man ? We need it anyway to keep man pages
updated.
>
> [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?
Yes. You're right. I'm not sure why we (me mostly) doing that sometimes
in code... Some weird habits from shell scripting? I don't know. :)
>
>
> Thanks for the review.
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
