From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH] kvm: x86: correct the misleading comment in vmx_handle_external_intr Date: Wed, 12 Oct 2016 16:05:09 +0200 Message-ID: <20161012140508.GC30525@potion> References: <1476059023-33224-1-git-send-email-longpeng2@huawei.com> <20161011182337.GB16406@potion> <57FD8EB9.9020400@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: pbonzini@redhat.com, tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, x86@kernel.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, zhaoshenglong@huawei.com, richard.weiyang@huawei.com To: "Longpeng (Mike)" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:60886 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755122AbcJLOFO (ORCPT ); Wed, 12 Oct 2016 10:05:14 -0400 Content-Disposition: inline In-Reply-To: <57FD8EB9.9020400@huawei.com> Sender: kvm-owner@vger.kernel.org List-ID: 2016-10-12 09:15+0800, Longpeng (Mike): > On 2016/10/12 2:23, Radim Krčmář wrote: >> This part is acceptable as it gives a new information code, yet the >> function does not modify flags, which makes it unremarkable. >> And dependencies on the caller would be better described in a header >> (if we cannot express them well in the code). >> >> The most comment-worthy thing about this function is the reason why we >> execute the interrupt handler manually, i.e. the dependency on >> VM_EXIT_ACK_INTR_ON_EXIT, but that is easy to tell from the commit >> message and convenient access to git history is essential in a workflow, >> so providing a leeway could be counter-productive. >> >> I would go with no comment for now. > > Thanks for your patience, and your advice is useful for me. I appreciate the patch, I just didn't want to repeat the same mistake that you were fixing in the patch, which made me go into rambling mode. Please send v2 with a simpler code comment (or no comment). And you are more than welcome to improve the code even further! > In addition, the comment below is misleading too, hope you can fix it > simultaneously. > > /* Interrupt is enabled by handle_external_intr() */ > kvm_x86_ops->handle_external_intr(vcpu); Yep, this comment should have been expressed in a function name. Paolo already fixed it in 1a6982353db9 ("KVM: x86: remove stale comments").