From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37165) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alBzr-0004Sa-3I for qemu-devel@nongnu.org; Wed, 30 Mar 2016 05:01:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1alBzk-00045Q-SH for qemu-devel@nongnu.org; Wed, 30 Mar 2016 05:01:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36625) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alBzk-00045M-JV for qemu-devel@nongnu.org; Wed, 30 Mar 2016 05:00:56 -0400 References: <1459161888-32566-1-git-send-email-caoj.fnst@cn.fujitsu.com> <56FAEBE6.2000609@redhat.com> <56FB51AD.7000107@cn.fujitsu.com> From: Marcel Apfelbaum Message-ID: <56FB95C3.9020508@redhat.com> Date: Wed, 30 Mar 2016 12:00:51 +0300 MIME-Version: 1.0 In-Reply-To: <56FB51AD.7000107@cn.fujitsu.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable 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: Cao jin , qemu-devel@nongnu.org Cc: mst@redhat.com, jasowang@redhat.com, Markus Armbruster , alex.williamson@redhat.com, hare@suse.de, dmitry@daynix.com, pbonzini@redhat.com, jsnow@redhat.com, kraxel@redhat.com On 03/30/2016 07:10 AM, Cao jin wrote: > 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 =3D PCI_DEVICE(s); >>> - int res; >>> - >>> - res =3D msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INT= RS, >>> - VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK); >>> - if (0 > res) { >>> - VMW_WRPRN("Failed to initialize MSI, error %d", res); >>> - s->msi_used =3D false; >>> - } else { >>> - s->msi_used =3D 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 =3D 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 =3D 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. >> Hi, > > 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 err= or message, should use error_report_err ? No, it is not related to VMW_WRPRN. On this subject, those are debugging = warnings and we should keep them the same as before. About the "warning only" error type: if an error is returned, the caller assumes that the initialization failed and it will return with failure. T= hat means that you cannot return a non-null error if you want the process to finish= OK. This is why you had to: - report the error (even if this function should not be a reporter b= ecause it receives an Error parameter) - empty the error: so why use error at all, right? My point is if the caller had a way to check what kind of error this is and act accordingly, it would be nicer. But again, this is out of the sco= pe of this patch, only some thoughts. > >>> - 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 =3D PCI_BRIDGE(dev); >>> PCIBridgeDev *bridge_dev =3D PCI_BRIDGE_DEV(dev); >>> int err; >>> + Error *local_err =3D NULL; >> >> You use both local_err and err for local error names. It doesn't reall= y >> 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", s= o 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 fa= miliar with error handling, I turn to like err, just because it is shorte= r:p) Indeed is a little confusing, this is why I prefer "local_error" because = it is used widely and became a familiar pattern. > >>> >>> 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 &=3D ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ); >>> } >>> + >>> err =3D 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 p= atch. >> > > 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 patc= h since then:) Oh, I hate when it happens to me, tho different opinions, how can you dea= l with that, right ? :) > > I don`t know, coding style & indentation patch really matters or is jus= t a personal taste thing? Coding style and indentation *are important* IMHO. For this case, what I would do is put the new lines and comments in a sep= arate patch,\ but send it with *the same* series, not through trivial list, saying some= thing like: "fixed some code style problems while resolving the ... problem". > >>> +++ b/hw/scsi/mptsas.c >>> @@ -1274,10 +1274,22 @@ static void mptsas_scsi_init(PCIDevice *dev, >>> Error **errp) >>> { >>> DeviceState *d =3D DEVICE(dev); >>> MPTSASState *s =3D MPT_SAS(dev); >>> + Error *err =3D NULL; >>> >>> dev->config[PCI_LATENCY_TIMER] =3D 0; >>> dev->config[PCI_INTERRUPT_PIN] =3D 0x01; >>> >>> + if (s->msi_available) { >>> + if (msi_init(dev, 0, 1, true, false, &err) >=3D 0) { >>> + s->msi_in_use =3D 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 paramete= r. >> > > 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=EF=BC=8C= so I use a local *err* to report error message, while don`t > touch **errp. Is there any better way? Same problem as discussed before, Markus do you have any idea how to solv= e this elegantly? CC: Markus Armbruster Thanks, Marcel > >> > > For all the other comments, will fix them in next version.