Re: [Qemu-devel] [PATCH 15/18] xen: add a mechanism to automatically create XenDevice-s...

[Paul Durrant]

> -----Original Message-----
> From: Anthony PERARD [mailto:address@hidden
> Sent: 04 December 2018 15:35
> To: Paul Durrant <address@hidden>
> Cc: address@hidden; address@hidden; xen-
> address@hidden; Stefano Stabellini <address@hidden>
> Subject: Re: [PATCH 15/18] xen: add a mechanism to automatically create
> XenDevice-s...
> 
> On Wed, Nov 21, 2018 at 03:12:08PM +0000, Paul Durrant wrote:
> > +    xen_backend_device_create(BUS(xenbus), type, name, opts,
> &local_err);
> > +    qobject_unref(opts);
> > +
> > +    if (local_err) {
> > +        const char *msg = error_get_pretty(local_err);
> > +
> > +        error_report("failed to create '%s' device '%s': %s", type,
> name,
> > +                     msg);
> > +        error_free(local_err);
> 
> You can use error_reportf_err() instead of those three calls. I may have
> only suggest error_report_err in a previous patch, but error_reportf_err
> does the error_prepend as well.
> 

Ah. I'll go back over the patches and use that where necessary.

> > +    }
> > +}
> > +
> > +static void xen_bus_type_enumerate(XenBus *xenbus, const char *type)
> > +{
> > +    char *domain_path = g_strdup_printf("backend/%s/%u", type,
> xen_domid);
> > +    char **backend;
> > +    unsigned int i, n;
> > +
> > +    trace_xen_bus_type_enumerate(type);
> > +
> > +    backend = xs_directory(xenbus->xsh, XBT_NULL, domain_path, &n);
> > +    if (!backend) {
> 
> domain_path isn't free here, you probably want a `goto out` which would
> free everything.

Ok.

> 
> > +        return;
> > +    }
> > +
> > @@ -193,6 +302,17 @@ static void xen_bus_realize(BusState *bus, Error
> **errp)
> >      notifier_list_init(&xenbus->watch_notifiers);
> >      qemu_set_fd_handler(xs_fileno(xenbus->xsh), xen_bus_watch, NULL,
> >                          xenbus);
> > +
> > +    module_call_init(MODULE_INIT_XEN_BACKEND);
> > +
> > +    xenbus->backend_watch =
> > +        xen_bus_add_watch(xenbus, "", /* domain root node */
> > +                          "backend", xen_bus_enumerate, xenbus,
> &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        error_prepend(errp, "failed to set up enumeration watch: ");
> 
> You should use error_propagate_prepend instead
> error_propagate;error_prepend. And it looks like there is the same
> mistake in other patches that I haven't notice.
> 

Oh, I didn't know about that one either... I've only seen the separate calls 
used elsewhere.

> Also you probably want goto fail here.
> 

Not sure about that. Whilst the bus scan won't happen, it doesn't mean devices 
can't be added via QMP.

> 
> > +static void xen_device_backend_changed(void *opaque)
> > +{
> > +    XenDevice *xendev = opaque;
> > +    const char *type = object_get_typename(OBJECT(xendev));
> > +    enum xenbus_state state;
> > +    unsigned int online;
> > +
> > +    trace_xen_device_backend_changed(type, xendev->name);
> > +
> > +    if (xen_device_backend_scanf(xendev, "state", "%u", &state) != 1) {
> > +        state = XenbusStateUnknown;
> > +    }
> > +
> > +    xen_device_backend_set_state(xendev, state);
> 
> It's kind of weird to set the internal state base on the external one
> that something else may have modified. Shouldn't we check that it is
> fine for something else to modify the state and that it is a correct
> transition?

The only thing (apart from this code) that's going to have perms to write the 
backend state is the toolstack... which is, of course, be definition trusted.

> 
> Also aren't we going in a loop by having QEMU set the state, then the
> watch fires again? (Not really a loop since the function _set_state
> check for changes.

No. It's de-bounced inside the set_state function.

> 
> Also maybe we should watch for the state changes only when something
> else like libxl creates (ask for) the backend, and ignore changes when
> QEMU did it itself.

I don't think it's necessary to add that complexity.

> 
> > +
> > +    if (xen_device_backend_scanf(xendev, "online", "%u", &online) != 1)
> {
> > +        online = 0;
> > +    }
> > +
> > +    xen_device_backend_set_online(xendev, !!online);
> > +
> > +    /*
> > +     * If a backend is still 'online' then its state should be cycled
> > +     * back round to InitWait in order for a new frontend instance to
> > +     * connect. This may happen when, for example, a frontend driver is
> > +     * re-installed or updated.
> > +     * If a backend id not 'online' then the device should be
> destroyed.
> 
> s/id/is/

Ok.

> 
> > +     */
> > +    if (xendev->backend_online &&
> > +        xendev->backend_state == XenbusStateClosed) {
> > +        xen_device_backend_set_state(xendev, XenbusStateInitWait);
> > +    } else if (!xendev->backend_online &&
> > +               (xendev->backend_state == XenbusStateClosed ||
> > +                xendev->backend_state == XenbusStateInitialising ||
> > +                xendev->backend_state == XenbusStateInitWait ||
> > +                xendev->backend_state == XenbusStateUnknown)) {
> > +        object_unparent(OBJECT(xendev));
> > +    }
> > +}
> > +
> >  static void xen_device_backend_create(XenDevice *xendev, Error **errp)
> >  {
> >      XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> > @@ -289,12 +463,38 @@ static void xen_device_backend_create(XenDevice
> *xendev, Error **errp)
> >          error_propagate(errp, local_err);
> >          error_prepend(errp, "failed to create backend: ");
> 
> It looks like there is a missing return here.
> 
> >      }
> > +
> > +    xendev->backend_state_watch =
> > +        xen_bus_add_watch(xenbus, xendev->backend_path,
> > +                          "state", xen_device_backend_changed,
> > +                          xendev, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        error_prepend(errp, "failed to watch backend state: ");
> 
> You should return here, as local_err mustn't be reused.
> 
> > +    }
> > +
> > +    xendev->backend_online_watch =
> > +        xen_bus_add_watch(xenbus, xendev->backend_path,
> > +                          "online", xen_device_backend_changed,
> > +                          xendev, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        error_prepend(errp, "failed to watch backend online: ");
> 
> You probably want a return here, in case there is more code added after.

Yes, there should be returns in all three cases above.

> 
> > +    }
> 
> Other instances of error_propagate;error_prepend to be replaced by
> error_propagate_prepend.

Yes, will do.

  Paul

> 
> >  }
> >
> 
> Thanks,
> 
> --
> Anthony PERARD