[Qemu-devel] [PATCH] pci: Clean up error reporting in pci_nic_init()

[Qemu-devel] [PATCH] pci: Clean up error reporting in pci_nic_init()

[Thomas Huth]

The error reporting in pci_nic_init() is quite erratic: Some errors
are printed directly with error_report(), and some are passed back
to the (only) caller pci_nic_init_nofail() via an Error pointer.
Let's fix up this inconsistency by always printing the error in
pci_nic_init() and by getting rid of the Error pointer this way.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/pci/pci.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b3d5100..6dac107 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1613,8 +1613,7 @@ static const char * const pci_nic_names[] = {
 /* Initialize a PCI NIC.  */
 static PCIDevice *pci_nic_init(NICInfo *nd, PCIBus *rootbus,
                                const char *default_model,
-                               const char *default_devaddr,
-                               Error **errp)
+                               const char *default_devaddr)
 {
     const char *devaddr = nd->devaddr ? nd->devaddr : default_devaddr;
     Error *err = NULL;
@@ -1641,7 +1640,7 @@ static PCIDevice *pci_nic_init(NICInfo *nd, PCIBus *rootbus,
 
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
     if (err) {
-        error_propagate(errp, err);
+        error_report_err(err);
         object_unparent(OBJECT(dev));
         return NULL;
     }
@@ -1652,17 +1651,13 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus,
                                const char *default_model,
                                const char *default_devaddr)
 {
-    Error *err = NULL;
     PCIDevice *res;
 
     if (qemu_show_nic_models(nd->model, pci_nic_models))
         exit(0);
 
-    res = pci_nic_init(nd, rootbus, default_model, default_devaddr, &err);
+    res = pci_nic_init(nd, rootbus, default_model, default_devaddr);
     if (!res) {
-        if (err) {
-            error_report_err(err);
-        }
         exit(1);
     }
     return res;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] pci: Clean up error reporting in pci_nic_init()

[Peter Crosthwaite]

On Mon, Apr 27, 2015 at 10:51 AM, Thomas Huth <thuth@redhat.com> wrote:
> The error reporting in pci_nic_init() is quite erratic: Some errors
> are printed directly with error_report(), and some are passed back
> to the (only) caller pci_nic_init_nofail() via an Error pointer.
> Let's fix up this inconsistency by always printing the error in
> pci_nic_init() and by getting rid of the Error pointer this way.
>

Can it be made consistent the other way? - always propagate? Usually
we move towards consistent error propagation rather that
deep-call-chain error reports.

Regards,
Peter

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/pci/pci.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index b3d5100..6dac107 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1613,8 +1613,7 @@ static const char * const pci_nic_names[] = {
>  /* Initialize a PCI NIC.  */
>  static PCIDevice *pci_nic_init(NICInfo *nd, PCIBus *rootbus,
>                                 const char *default_model,
> -                               const char *default_devaddr,
> -                               Error **errp)
> +                               const char *default_devaddr)
>  {
>      const char *devaddr = nd->devaddr ? nd->devaddr : default_devaddr;
>      Error *err = NULL;
> @@ -1641,7 +1640,7 @@ static PCIDevice *pci_nic_init(NICInfo *nd, PCIBus *rootbus,
>
>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>      if (err) {
> -        error_propagate(errp, err);
> +        error_report_err(err);
>          object_unparent(OBJECT(dev));
>          return NULL;
>      }
> @@ -1652,17 +1651,13 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus,
>                                 const char *default_model,
>                                 const char *default_devaddr)
>  {
> -    Error *err = NULL;
>      PCIDevice *res;
>
>      if (qemu_show_nic_models(nd->model, pci_nic_models))
>          exit(0);
>
> -    res = pci_nic_init(nd, rootbus, default_model, default_devaddr, &err);
> +    res = pci_nic_init(nd, rootbus, default_model, default_devaddr);
>      if (!res) {
> -        if (err) {
> -            error_report_err(err);
> -        }
>          exit(1);
>      }
>      return res;
> --
> 1.8.3.1
>
>

Re: [Qemu-devel] [PATCH] pci: Clean up error reporting in pci_nic_init()

[Thomas Huth]

On Mon, 27 Apr 2015 10:55:41 -0700
Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> On Mon, Apr 27, 2015 at 10:51 AM, Thomas Huth <thuth@redhat.com> wrote:
> > The error reporting in pci_nic_init() is quite erratic: Some errors
> > are printed directly with error_report(), and some are passed back
> > to the (only) caller pci_nic_init_nofail() via an Error pointer.
> > Let's fix up this inconsistency by always printing the error in
> > pci_nic_init() and by getting rid of the Error pointer this way.
> >
> 
> Can it be made consistent the other way? - always propagate? Usually
> we move towards consistent error propagation rather that
> deep-call-chain error reports.

In that case, I'd need to rework qemu_find_nic_model() in net/net.c,
too, since this is also printing errors directly. ... I could certainly
do that, too, but I slowly start wondering whether this all is worth
the effort, just to make the code for the _legacy_ "-net" option a
little bit nicer. Maybe we should rather focus on thinking about ways
to finally get rid of "-net" one day?

Another idea: What about merging pci_nic_init() into
pci_nic_init_no_fail()? pci_nic_init() is only used by the _no_fail()
function, so there seems very few benefit by having this code in a
separate function. If they got merged, the error printing inconsistency
would be solved, too.

 Thomas

Re: [Qemu-devel] [PATCH] pci: Clean up error reporting in pci_nic_init()

[Michael S. Tsirkin]

On Tue, Apr 28, 2015 at 09:26:47AM +0200, Thomas Huth wrote:
> On Mon, 27 Apr 2015 10:55:41 -0700
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
> > On Mon, Apr 27, 2015 at 10:51 AM, Thomas Huth <thuth@redhat.com> wrote:
> > > The error reporting in pci_nic_init() is quite erratic: Some errors
> > > are printed directly with error_report(), and some are passed back
> > > to the (only) caller pci_nic_init_nofail() via an Error pointer.
> > > Let's fix up this inconsistency by always printing the error in
> > > pci_nic_init() and by getting rid of the Error pointer this way.
> > >
> > 
> > Can it be made consistent the other way? - always propagate? Usually
> > we move towards consistent error propagation rather that
> > deep-call-chain error reports.
> 
> In that case, I'd need to rework qemu_find_nic_model() in net/net.c,
> too, since this is also printing errors directly. ... I could certainly
> do that, too, but I slowly start wondering whether this all is worth
> the effort, just to make the code for the _legacy_ "-net" option a
> little bit nicer. Maybe we should rather focus on thinking about ways
> to finally get rid of "-net" one day?

Yes please.
I think what it would take is supporting dump option in netdevs.
Need some careful coding to bail out if people try to use
this with vhost.
Want to work on it?

> Another idea: What about merging pci_nic_init() into
> pci_nic_init_no_fail()? pci_nic_init() is only used by the _no_fail()
> function, so there seems very few benefit by having this code in a
> separate function. If they got merged, the error printing inconsistency
> would be solved, too.
> 
>  Thomas

This later one sounds good to me.

-- 
MST

Re: [Qemu-devel] [PATCH] pci: Clean up error reporting in pci_nic_init()

[Thomas Huth]

On Tue, 28 Apr 2015 10:40:09 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Apr 28, 2015 at 09:26:47AM +0200, Thomas Huth wrote:
> > On Mon, 27 Apr 2015 10:55:41 -0700
> > Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> > 
> > > On Mon, Apr 27, 2015 at 10:51 AM, Thomas Huth <thuth@redhat.com> wrote:
> > > > The error reporting in pci_nic_init() is quite erratic: Some errors
> > > > are printed directly with error_report(), and some are passed back
> > > > to the (only) caller pci_nic_init_nofail() via an Error pointer.
> > > > Let's fix up this inconsistency by always printing the error in
> > > > pci_nic_init() and by getting rid of the Error pointer this way.
> > > >
> > > 
> > > Can it be made consistent the other way? - always propagate? Usually
> > > we move towards consistent error propagation rather that
> > > deep-call-chain error reports.
> > 
> > In that case, I'd need to rework qemu_find_nic_model() in net/net.c,
> > too, since this is also printing errors directly. ... I could certainly
> > do that, too, but I slowly start wondering whether this all is worth
> > the effort, just to make the code for the _legacy_ "-net" option a
> > little bit nicer. Maybe we should rather focus on thinking about ways
> > to finally get rid of "-net" one day?
> 
> Yes please.
> I think what it would take is supporting dump option in netdevs.
> Need some careful coding to bail out if people try to use
> this with vhost.
> Want to work on it?

Sure, I can have a look.

> > Another idea: What about merging pci_nic_init() into
> > pci_nic_init_no_fail()? pci_nic_init() is only used by the _no_fail()
> > function, so there seems very few benefit by having this code in a
> > separate function. If they got merged, the error printing inconsistency
> > would be solved, too.
> > 
> >  Thomas
> 
> This later one sounds good to me.

Patch sent.

 Thomas