From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f44.google.com (mail-wm0-f44.google.com [74.125.82.44]) by dpdk.org (Postfix) with ESMTP id 05B9528FD for ; Fri, 1 Jul 2016 09:19:13 +0200 (CEST) Received: by mail-wm0-f44.google.com with SMTP id r190so8314658wmr.0 for ; Fri, 01 Jul 2016 00:19:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-transfer-encoding; bh=PMeIMyC5ppRxo+SvVo6YZ3zrGrkCzog5CgrmnEwYdvs=; b=W0tMUX2fzRQ8stTfTVv1VBdzYTsmzjxOqGAqNSQjYCila3UQDhxMTAikIQ/c/Wop9w ssxARjf3oy6YMUvtp/+/U//I6gEo2JEFGcp7M+UEgcVDkpQc+LJezbcWQxBi3/pIwbqy RPzMHicpw/K8SGKYBf02aKZUln1A5ILsw4yQy/T+1atOuL/g6PFe6F97PGc3F9LQ/BsQ Xl7IroJrNo9Cj9Wv6eJGJ+IDDVFBuehb+odF37TTXA6MhqXRKdiSIb8anXVE2j4z/ygV hBp0wWwfuFT6FvbrbExP5ooVKQGZHr13hq9iZtWDcjFG4UzSbwePgcFLdDJHTxxj9sZc VH1A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=PMeIMyC5ppRxo+SvVo6YZ3zrGrkCzog5CgrmnEwYdvs=; b=MZaMWe6CmQral8Mij0Hn0DAF6timqk5/5ezeXGmEZoZ9KstPisnVkpq28+avvdBhv7 +uSTlP+yuQDKGup1/mT3fK2dFy1KxLwuEN6gdmdrAtye9ai0dz/TBGpSNnXgJ+/dTO9/ aFdUaqgX0oWo5sQV0Q7KB2d7MNu9o/zbGkphS5JERiNNANtPtAUVfUbwsCzY+sv8+rdv XpOCd8MCMfeTozUAZ3SHbVYv4yNB84b3UMWPK0zw+TxEjwHXHDOK3IzRutqi2OdtYbNA FO0lWkHuVn0ei4UOifBkWKH+0TAcjwE2Vi+gO2IgAM2WQ2eCcf1G2ZBztNKbC1EcNPg+ b2Hw== X-Gm-Message-State: ALyK8tIcR1D9XiQ1pQX2RpImStiQppj9IacfTYsL7qVA0wxRvkW1Rv6QW1KNregjMMv+2nNQ X-Received: by 10.194.157.162 with SMTP id wn2mr2198778wjb.103.1467357552734; Fri, 01 Jul 2016 00:19:12 -0700 (PDT) Received: from [192.168.1.15] (LFbn-1-8274-170.w81-254.abo.wanadoo.fr. [81.254.171.170]) by smtp.gmail.com with ESMTPSA id f73sm1277229wmg.1.2016.07.01.00.19.11 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 01 Jul 2016 00:19:11 -0700 (PDT) To: "Mrzyglod, DanielX T" , "dev@dpdk.org" References: <1460638879-45680-1-git-send-email-danielx.t.mrzyglod@intel.com> <57275802.7090606@6wind.com> <7ADD74816B4C8A45B56203CBA65FE5A63779734C@IRSMSX107.ger.corp.intel.com> From: Olivier MATZ Message-ID: Date: Fri, 1 Jul 2016 09:19:01 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.1.0 MIME-Version: 1.0 In-Reply-To: <7ADD74816B4C8A45B56203CBA65FE5A63779734C@IRSMSX107.ger.corp.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] cmdline: fix unchecked return value X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Jul 2016 07:19:13 -0000 Hi Daniel, >>> --- a/lib/librte_cmdline/cmdline_rdline.c >>> +++ b/lib/librte_cmdline/cmdline_rdline.c >>> @@ -377,7 +377,10 @@ rdline_char_in(struct rdline *rdl, char c) >>> case CMDLINE_KEY_CTRL_K: >>> cirbuf_get_buf_head(&rdl->right, rdl->kill_buf, >> RDLINE_BUF_SIZE); >>> rdl->kill_size = CIRBUF_GET_LEN(&rdl->right); >>> - cirbuf_del_buf_head(&rdl->right, rdl->kill_size); >>> + >>> + if (cirbuf_del_buf_head(&rdl->right, rdl->kill_size) < 0) >>> + return -EINVAL; >>> + >>> rdline_puts(rdl, vt100_clear_right); >>> break; >>> >> >> I wonder if a better way to fix wouldn't be to remove the checks >> introduced in http://dpdk.org/browse/dpdk/commit/?id=ab971e562860 >> >> There is no reason to check that in cirbuf_get_buf_head/tail(): >> if (!cbuf || !c) >> >> The function should never fail, it just returns the number of >> copied chars. This is the responsibility of the caller to ensure >> that the pointer to the circular buffer is not NULL. >> >> Also, rdline_char_in() is not expected to return -EINVAL, but >> RDLINE_RES_* instead. >> >> So I think that partially revert ab971e562860 would fix the >> coverity warning. >> >> Regards, >> Olivier > > Removing checks probably will generate more Coverity errors somewhere. > I see that only places where we test negative values are in unit tests. > > Reverting changes I think is overhead and maybe ignoring this patch and set is as false positive in Coverity is better idea ? We can mark the warning as false positive because this cannot happen right now (the calller checks the validity of cbuf/c). But this is probably something I'll come back on with a patch since there is no reason to check that pointers are not NULL in cirbuf_get_buf_head/tail(). Regards, Olivier