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

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

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 :)

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

>   	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 ((indent > 0) || (idx >> 0))

idx > 0?

> +				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.

> +		} 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?

> +
> +		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.

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

-- 
Thanks,
Anatoly