From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH] svm: rephrase local variable use for Coverity. Date: Wed, 6 Jan 2016 09:33:26 -0500 Message-ID: <568D25B6.7020701@oracle.com> References: <56810224.208@citrix.com> <1451618075-1161-1-git-send-email-jtotto@uwaterloo.ca> <568D23AF02000078000C3F2F@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <568D23AF02000078000C3F2F@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Joshua Otto Cc: ian.campbell@citrix.com, andrew.cooper3@citrix.com, czylin@uwaterloo.ca, xen-devel@lists.xen.org, Aravind Gopalakrishnan , Suravee Suthikulpanit , hjarmstr@uwaterloo.ca List-Id: xen-devel@lists.xenproject.org On 01/06/2016 08:24 AM, Jan Beulich wrote: >>>> On 01.01.16 at 04:14, wrote: >> Coverity CID 1343310 >> >> No functional changes. >> >> Signed-off-by: Joshua Otto >> --- >> On Mon, Dec 28, 2015 at 09:34:28AM +0000, Andrew Cooper wrote: >>> The error message isn't fantastic, but the complaint that Coverity >>> has is that we store intr here, then unilaterally store it again >>> slightly lower in the function, no matter what value it had (with >>> the early return presumably not being taken into account). >>> >>> The error would probably be resolved if lines 95 and 96 turned into >>> "if ( vmcb_get_vintr(gvmcb).fields.irq )" >> This patch implements that change - as a general rule, is maintainer >> preference to resolve false positives like this by suppressing them in >> the tool or through code changes like this one? I'd rather suppress this in the tool as I am one of those people that Jan refers to below ;-) However, if it's too much of a hassle then this patch would be OK. -boris > Asking such a question it would be helpful if you included the > maintainers of the code in question, since to a good part this > is a matter of taste, especially when ... > >> --- a/xen/arch/x86/hvm/svm/intr.c >> +++ b/xen/arch/x86/hvm/svm/intr.c >> @@ -92,8 +92,7 @@ static void svm_enable_intr_window(struct vcpu *v, struct hvm_intack intack) >> * return here or l2 guest looses interrupts, otherwise. >> */ >> ASSERT(gvmcb != NULL); >> - intr = vmcb_get_vintr(gvmcb); >> - if ( intr.fields.irq ) >> + if ( vmcb_get_vintr(gvmcb).fields.irq ) > ... some people (not me) frown upon complex expressions like the > one resulting here. > > Also please note that while perhaps minor here, obvious with > the quote from an earlier mail conversation, naming the person > having suggested the change would be appropriate - if you > look for them, you'll find quite a few Suggested-by: tags in the > commit history. > > Jan >