Re: [dpdk-dev] [PATCH v3 5/8] examples/power: add json string handling

["Hunt, David" <david.hunt@intel.com>]

Hi Anatoly,


On 25/9/2018 12:27 PM, Burakov, Anatoly wrote:
> On 14-Sep-18 2:54 PM, David Hunt wrote:
>> Add JSON string handling to vm_power_manager for JSON strings received
>> through the fifo. The format of the JSON strings are detailed in the
>> next patch, the vm_power_manager user guide documentation updates.
>>
>> This patch introduces a new dependency on Jansson, a C library for
>> encoding, decoding and manipulating JSON data. To compile the sample app
>> you now need to have installed libjansson4 and libjansson-dev (these may
>> be named slightly differently depending on your Operating System)
>>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>> ---
>
> <snip>
>
>>   void channel_monitor_exit(void)
>>   {
>>       run_loop = 0;
>> @@ -124,18 +259,11 @@ get_pcpu_to_control(struct policy *pol)
>>         ci = get_core_info();
>>   -    RTE_LOG(INFO, CHANNEL_MONITOR,
>> -            "Looking for pcpu for %s\n", pol->pkt.vm_name);
>> -
>>       /*
>>        * So now that we're handling virtual and physical cores, we 
>> need to
>>        * differenciate between them when adding them to the branch 
>> monitor.
>>        * Virtual cores need to be converted to physical cores.
>>        */
>> -
>> -
>> -
>> -
>
> Now you're removing those newlines you added in previous commit :)
>

Fixed in previous patch in the next version.


>>       if (pol->pkt.core_type == CORE_TYPE_VIRTUAL) {
>>           /*
>>            * If the cores in the policy are virtual, we need to map them
>> @@ -295,8 +423,6 @@ apply_traffic_profile(struct policy *pol)
>>         diff = get_pkt_diff(pol);
>>   -    RTE_LOG(INFO, CHANNEL_MONITOR, "Applying traffic profile\n");
>> -
>
> Here and in a few other places: these log message removals look to be 
> unrelated to this commit. Also, in my experience, more logging is 
> better than less logging, especially when something goes wrong - maybe 
> instead of removing them, just switch the level to debug?
>

Changed to Debug instead. Was causing quite a verbose output.

>>       if (diff >= (pol->pkt.traffic_policy.max_max_packet_thresh)) {
>>           for (count = 0; count < pol->pkt.num_vcpu; count++) {
>>               if (pol->core_share[count].status != 1)
>> @@ -340,9 +466,6 @@ apply_time_profile(struct policy *pol)
>>                   if (pol->core_share[count].status != 1) {
>>                       power_manager_scale_core_max(
>>                           pol->core_share[count].pcpu);
>> -                RTE_LOG(INFO, CHANNEL_MONITOR,
>
> <snip>
>
>> +        int idx = 0;
>> +        int indent = 0;
>> +        do {
>> +            n_bytes = read(chan_info->fd, &json_data[idx], 1);
>> +            if (n_bytes == 0)
>> +                break;
>> +            if (json_data[idx] == '{')
>> +                indent++;
>> +            if (json_data[idx] == '}')
>> +                indent--;
>
> What happens if someone sends a string with a "{" or "}" inside?
>

If we get to the end of the buffer without a "}", it calls the library 
to convert, will fail, and move on.  No damage done (I hope).
Also, a short un-terminated (by "}") string will also exit when no 
characters read.
So any invalid JSON string that's send to Jansson will fail to parse, 
and the application will be ready for the next (hopefully valid) JSON 
string.

>> +            if ((indent > 0) || (idx >> 0))
>
> idx > 0?
>

Yes, will fix.

>> +                idx++;
>> +            if (indent == 0)
>> +                json_data[idx] = 0;
>> +            if (idx >= MAX_JSON_STRING_LEN)
>> +                break;
>
> This looks like a potential overflow to me, because you increment idx, 
> check if it's >= max, but still write into that location if indent == 
> 0 on previous line.
>

Yes, will exit at MAX_JSON_STRING_LEN-1

>> +        } while (indent > 0);
>> +
>> +        if (indent > 0)
>> +            /*
>> +             * We've broken out of the read loop without getting
>> +             * a closing brace, so throw away the datai
>
> I think "data" is plural already, no need to invent a new word :)
>
>> +             */
>> +            json_data[idx] = 0;
>
> idx could potentially be overflown already due to code above?
>

Typo fixed in next version.

>> +
>> +        if (strlen(json_data) == 0)
>> +            continue;
>> +
>> +        printf("got [%s]\n", json_data);
>> +
>
> <snip>
>
>>   void
>>   run_channel_monitor(void)
>>   {
>> @@ -570,11 +785,16 @@ run_channel_monitor(void)
>>           if (!run_loop)
>>               break;
>>           for (i = 0; i < n_events; i++) {
>> +            if (!global_events_list[i].data.ptr) {
>> +                fflush(stdout);
>> +                continue;
>> +            }
>
> This change looks unrelated to this commit.

There was a few printfs in there, this change will be removed altogether 
in next version.

>
>>               struct channel_info *chan_info = (struct channel_info *)
>>                       global_events_list[i].data.ptr;
>>               if ((global_events_list[i].events & EPOLLERR) ||
>

Thanks,
Dave.