From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36318) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1al7Qo-0006MG-GV for qemu-devel@nongnu.org; Wed, 30 Mar 2016 00:08:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1al7Ql-0003eK-9Z for qemu-devel@nongnu.org; Wed, 30 Mar 2016 00:08:34 -0400 Received: from [59.151.112.132] (port=16499 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1al7Qj-0003cu-AV for qemu-devel@nongnu.org; Wed, 30 Mar 2016 00:08:31 -0400 References: <1459161888-32566-1-git-send-email-caoj.fnst@cn.fujitsu.com> <56FAEBE6.2000609@redhat.com> From: Cao jin Message-ID: <56FB51AD.7000107@cn.fujitsu.com> Date: Wed, 30 Mar 2016 12:10:21 +0800 MIME-Version: 1.0 In-Reply-To: <56FAEBE6.2000609@redhat.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v3] Add param Error ** for msi_init() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum , qemu-devel@nongnu.org Cc: mst@redhat.com, jasowang@redhat.com, armbru@redhat.com, alex.williamson@redhat.com, hare@suse.de, dmitry@daynix.com, pbonzini@redhat.com, jsnow@redhat.com, kraxel@redhat.com Hi Marcel, Thanks for your quick review for this big fat patch:) please see my comments inline. On 03/30/2016 04:56 AM, Marcel Apfelbaum wrote: > On 03/28/2016 01:44 PM, Cao jin wrote: >> >> -#define VMXNET3_USE_64BIT (true) >> -#define VMXNET3_PER_VECTOR_MASK (false) >> - >> -static bool >> -vmxnet3_init_msi(VMXNET3State *s) >> -{ >> - PCIDevice *d = PCI_DEVICE(s); >> - int res; >> - >> - res = msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, >> - VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK); >> - if (0 > res) { >> - VMW_WRPRN("Failed to initialize MSI, error %d", res); >> - s->msi_used = false; >> - } else { >> - s->msi_used = true; >> - } >> - >> - return s->msi_used; >> -} >> - >> static void >> vmxnet3_cleanup_msi(VMXNET3State *s) >> { >> @@ -2271,6 +2250,10 @@ static uint8_t >> *vmxnet3_device_serial_num(VMXNET3State *s) >> return dsnp; >> } >> >> + >> +#define VMXNET3_USE_64BIT (true) >> +#define VMXNET3_PER_VECTOR_MASK (false) >> + >> static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) >> { >> DeviceState *dev = DEVICE(pci_dev); >> @@ -2278,6 +2261,16 @@ static void vmxnet3_pci_realize(PCIDevice >> *pci_dev, Error **errp) >> >> VMW_CBPRN("Starting init..."); >> >> + msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, >> + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, errp); >> + if (*errp) { >> + error_report_err(*errp); >> + *errp = NULL; > > You suppress prev debug message: VMW_WRPRN("Failed to initialize MSI, > error %d", res), > but you replace with one of yourself. If the vmxnet maintainers have > nothing against, > I suppose is OK. > > Then, you don't propagate the error because you don't want realize > to fail, so you report it and NULL it. I suppose we should have > a "warning only" error type so the reporting party can decide to issue a > warning > or to fail, but is out of the scope of this patch, of course. > Yes, I should add more hint message. I don`t quite understand about: /have a "warning only" error type so the reporting party can decide to issue a warning or to fail/ Do you mean still using VMW_WRPRN or using error_append_hint? It seems VMW_WRPRN should only work in debug time? and if user should see this error message, should use error_report_err ? >> - if (!vmxnet3_init_msi(s)) { >> - VMW_WRPRN("Failed to initialize MSI, configuration is >> inconsistent."); > > Here again you remove an existent debug warning, maybe you should keep it. > >> - } >> - >> vmxnet3_net_init(s); >> >> diff --git a/hw/pci-bridge/pci_bridge_dev.c >> b/hw/pci-bridge/pci_bridge_dev.c >> index 862a236..07c7bf8 100644 >> --- a/hw/pci-bridge/pci_bridge_dev.c >> +++ b/hw/pci-bridge/pci_bridge_dev.c >> @@ -52,6 +52,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) >> PCIBridge *br = PCI_BRIDGE(dev); >> PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); >> int err; >> + Error *local_err = NULL; > > You use both local_err and err for local error names. It doesn't really > matter > the name, but please be consistent. I personally like local_error but is > up to you, of course. > Yes, I prefer consistent way, but here, you see there is a "int err", so I thought two different variants using same name maybe not so good for reading code? what would you choose?(I like local_err at first because it is easy to understand for newbie, but when I get familiar with error handling, I turn to like err, just because it is shorter:p) >> >> pci_bridge_initfn(dev, TYPE_PCI_BUS); >> >> @@ -67,17 +68,21 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) >> /* MSI is not applicable without SHPC */ >> bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ); >> } >> + >> err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0); >> if (err) { >> goto slotid_error; >> } >> + > > You have several new lines (before and after this comment) that have > nothing > to do with the patch. I suggest putting them into a trivial separate patch. > see what I was told before: http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00116.html http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00123.html Actually I am ok with both sides. I just stop sending coding style patch since then:) I don`t know, coding style & indentation patch really matters or is just a personal taste thing? >> +++ b/hw/scsi/mptsas.c >> @@ -1274,10 +1274,22 @@ static void mptsas_scsi_init(PCIDevice *dev, >> Error **errp) >> { >> DeviceState *d = DEVICE(dev); >> MPTSASState *s = MPT_SAS(dev); >> + Error *err = NULL; >> >> dev->config[PCI_LATENCY_TIMER] = 0; >> dev->config[PCI_INTERRUPT_PIN] = 0x01; >> >> + if (s->msi_available) { >> + if (msi_init(dev, 0, 1, true, false, &err) >= 0) { >> + s->msi_in_use = true; >> + } >> + else { >> + error_append_hint(&err, "MSI init fail, will use INTx >> instead"); > > The hint should end with a new line: "\n". > >> + error_report_err(err); > > You should not report the error if the fucntion has an **errp parameter. > it will use INTx even if msi_init fail, so it should not break realize. But I think user should know it if something wrong happened thereļ¼Œso I use a local *err* to report error message, while don`t touch **errp. Is there any better way? > For all the other comments, will fix them in next version. -- Yours Sincerely, Cao jin