From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 63B541B161 for ; Tue, 25 Sep 2018 16:02:42 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Sep 2018 07:02:41 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,302,1534834800"; d="scan'208";a="86401885" Received: from dhunt5-mobl2.ger.corp.intel.com (HELO [10.237.221.37]) ([10.237.221.37]) by orsmga003.jf.intel.com with ESMTP; 25 Sep 2018 07:00:22 -0700 To: "Burakov, Anatoly" , dev@dpdk.org Cc: john.mcnamara@intel.com, stephen@networkplumber.org, lei.a.yao@intel.com, bruce.richardson@intel.com References: <20180912144930.50578-1-david.hunt@intel.com> <20180914135406.52190-1-david.hunt@intel.com> <20180914135406.52190-6-david.hunt@intel.com> <47b4dbc6-48ca-f41c-86cb-e6f5b168ebf9@intel.com> From: "Hunt, David" Message-ID: <046d9290-55f9-8433-cbbd-b975f353fa47@intel.com> Date: Tue, 25 Sep 2018 15:00:22 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <47b4dbc6-48ca-f41c-86cb-e6f5b168ebf9@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH v3 5/8] examples/power: add json string handling X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Sep 2018 14:02:43 -0000 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 >> --- > > > >>   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, > > > >> +        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); >> + > > > >>   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.