Re: [PATCH 06/12] hw/xen: add get_frontend_path() method to XenDeviceClass

[Paul Durrant]

On 24/10/2023 14:29, David Woodhouse wrote:
> On Tue, 2023-10-24 at 13:59 +0100, Paul Durrant wrote:
> > On 24/10/2023 13:56, David Woodhouse wrote:
> > > On Tue, 2023-10-24 at 13:42 +0100, Paul Durrant wrote:
> > > > > --- a/hw/xen/xen-bus.c
> > > > > +++ b/hw/xen/xen-bus.c
> > > > > @@ -711,8 +711,16 @@ static void xen_device_frontend_create(XenDevice *xendev, 
> > > > > Error **errp)
> > > > >      {
> > > > >          ERRP_GUARD();
> > > > >          XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> > > > > +    XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
> > > > >      
> > > > > -    xendev->frontend_path = xen_device_get_frontend_path(xendev);
> > > > > +    if (xendev_class->get_frontend_path) {
> > > > > +        xendev->frontend_path = xendev_class->get_frontend_path(xendev, errp);
> > > > > +        if (!xendev->frontend_path) {
> > > > > +            return;
> > > >
> > > > I think you need to update errp here to note that you are failing to
> > > > create the frontend.
> > >
> > > If xendev_class->get_frontend_path returned NULL it will have filled in errp.
> >
> > Ok, but a prepend to say that a lack of path there means we skip
> > frontend creation seems reasonable?
>
> No, it *is* returning an error. Perhaps I can make it

I understand it is returning an error. I thought the point of the 
cascading error handling was to prepend text at each (meaningful) layer 
such that the eventual message conveyed what failed and also what the 
consequences of that failure were.

  Paul

>      if (!xendev->frontend_path) {
>          /*
>           * If the ->get_frontend_path() method returned NULL, it will
>           * already have set *errp accordingly. Return the error.
>           */
>          return /* false */;
>      }
>
>
> > > As a general rule (I'll be doing a bombing run on xen-bus once I get my
> > > patch queue down into single digits) we should never check 'if (*errp)'
> > > to check if a function had an error. It should *also* return a success
> > > or failure indication, and we should cope with errp being NULL.
> >
> > I'm pretty sure someone told me the exact opposite a few years back.
>
> Then they were wrong :)