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

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

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

    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 :)

Attachment: smime.p7s
Description: S/MIME cryptographic signature