Re: [Minios-devel] [UNIKRAFT PATCH v2 09/10] plat/xen: Add driver state functions to client API

[Costin Lupu <costin.lup@xxxxxxxxx>]

Hi Yuri,

On 09/04/2018 04:19 PM, Yuri Volchkov wrote:
> Hi Costin,
> 
> one more note inline
> 
> -Yuri.
> 
> Costin Lupu <costin.lupu@xxxxxxxxx> writes:
> 
>> Extend the client API with functions for dealing with Xenbus
>> driver states.
>>
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> ---
>>  plat/xen/include/xenbus/client.h |  36 +++++++++++
>>  plat/xen/xenbus/client.c         | 128 
>> +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 164 insertions(+)
>>
>> diff --git a/plat/xen/include/xenbus/client.h 
>> b/plat/xen/include/xenbus/client.h
>> index f3540b7..2add3be 100644
>> --- a/plat/xen/include/xenbus/client.h
>> +++ b/plat/xen/include/xenbus/client.h
>> @@ -86,4 +86,40 @@ int xenbus_watch_wait_event(struct xenbus_watch *watch);
>>   */
>>  int xenbus_watch_notify_event(struct xenbus_watch *watch);
>>  
>> +/*
>> + * Driver states
>> + */
>> +
>> +/*
>> + * Returns the driver state found at the given Xenstore path.
>> + *
>> + * @param path Xenstore path
>> + * @return The Xenbus driver state
>> + */
>> +XenbusState xenbus_read_driver_state(const char *path);
>> +
>> +/*
>> + * Changes the state of a Xen PV driver
>> + *
>> + * @param xendev Xenbus device
>> + * @param state The new Xenbus state
>> + * @param xbt Xenbus transaction id
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xenbus_switch_state(struct xenbus_device *xendev, XenbusState state,
>> +    xenbus_transaction_t xbt);
>> +
>> +/*
>> + * Waits for the driver state found at the given Xenstore path to change by
>> + * using watches.
>> + *
>> + * @param path Xenstore path
>> + * @param state The returned Xenbus state
>> + * @param watch Xenbus watch. It may be NULL, in which case a local watch
>> + * will be created.
>> + * @return 0 on success, a negative errno value on error.
>> + */
>> +int xenbus_wait_for_state_change(const char *path, XenbusState *state,
>> +    struct xenbus_watch *watch);
>> +
>>  #endif /* __XENBUS_CLIENT_H__ */
>> diff --git a/plat/xen/xenbus/client.c b/plat/xen/xenbus/client.c
>> index 3dbca0f..b45fd0c 100644
>> --- a/plat/xen/xenbus/client.c
>> +++ b/plat/xen/xenbus/client.c
>> @@ -44,6 +44,7 @@
>>  #include <string.h>
>>  #include <uk/errptr.h>
>>  #include <uk/wait.h>
>> +#include <xenbus/xs.h>
>>  #include <xenbus/client.h>
>>  
>>  
>> @@ -130,3 +131,130 @@ int xenbus_watch_notify_event(struct xenbus_watch 
>> *watch)
>>  
>>      return 0;
>>  }
>> +
>> +XenbusState xenbus_read_driver_state(const char *path)
>> +{
>> +    char state_path[strlen(path) + sizeof("/state")];
>> +    XenbusState state = XenbusStateUnknown;
>> +
>> +    sprintf(state_path, "%s/state", path);
>> +    xs_read_integer(state_path, (int *) &state);
>> +
>> +    return state;
>> +}
>> +
>> +static XenbusState xenbus_intstr_to_state(const char *intstr)
>> +{
>> +    return (XenbusState) atoi(intstr);
>> +}
>> +
>> +int xenbus_switch_state(struct xenbus_device *xendev, XenbusState state,
>> +            xenbus_transaction_t xbt)
>> +{
>> +    char state_path[strlen(xendev->nodename) + sizeof("/state")];
>> +    char *current_state_str, new_state_str[2];
>> +    XenbusState current_state;
>> +    int need_transaction_end = 0; /* non-zero if local transaction */
>> +    int abort;
>> +    int err;
>> +
>> +    if (xendev == NULL)
>> +            return -EINVAL;
>> +
>> +    sprintf(state_path, "%s/state", xendev->nodename);
>> +
>> +    do {
>> +            abort = 1;
>> +
>> +            if (xbt == XBT_NIL) {
>> +                    err = xs_transaction_start(&xbt);
>> +                    if (err)
>> +                            goto exit;
>> +                    need_transaction_end = 1;
>> +            }
>> +
>> +            /* check if state is already set */
>> +            current_state_str = xs_read(xbt, state_path, NULL);
>> +            if (PTRISERR(current_state_str)) {
>> +                    err = PTR2ERR(current_state_str);
>> +                    goto exit;
>> +            }
>> +
>> +            /* convert to int */
>> +            current_state = xenbus_intstr_to_state(current_state_str);
>> +            free(current_state_str);
>> +
>> +            if (current_state == state)
>> +                    goto exit; /* state already set */
>> +
>> +            /* set new state */
>> +            sprintf(new_state_str, "%d", state);
>> +            err = xs_write(xbt, state_path, NULL, new_state_str);
>> +
>> +            abort = 0;
>> +exit:
>> +            if (need_transaction_end) {
>> +                    int _err;
>> +
>> +                    _err = xs_transaction_end(xbt, abort);
>> +                    if (!err)
>> +                            err = _err;
>> +                    xbt = XBT_NIL;
>> +            }
>> +    } while (err == -EAGAIN);
>> +
>> +    if (err)
>> +            uk_printd(DLVL_ERR, "Error switching state to %s: %d\n",
>> +                    xenbus_state_to_str(state), err);
>> +
>> +    return err;
>> +}
>> +
>> +int xenbus_wait_for_state_change(const char *path, XenbusState *state,
>> +    struct xenbus_watch *watch)
>> +{
>> +    char *crnt_state_str;
>> +    XenbusState crnt_state;
>> +    int err = 0, watch_is_local = 0;
>> +
>> +    if (path == NULL || state == NULL) {
>> +            err = -EINVAL;
>> +            goto out;
>> +    }
>> +
>> +    for (;;) {
>> +            crnt_state_str = xs_read(XBT_NIL, path, NULL);
>> +            if (PTRISERR(crnt_state_str)) {
>> +                    err = PTR2ERR(crnt_state_str);
>> +                    goto out;
>> +            }
>> +
>> +            /* convert to int */
>> +            crnt_state = xenbus_intstr_to_state(crnt_state_str);
>> +            free(crnt_state_str);
>> +
>> +            if (crnt_state != *state) {
>> +                    *state = crnt_state;
>> +                    break;
>> +            }
>> +
>> +            if (watch == NULL) {
> If watch was not registered, and the state of the device is changed
> about this point in time (after xs_read and before xs_watch_path), we
> would not get a notification about this change. It might be that we will
> stuck in xenbus_watch_wait_event forever.
> 
> I propose to save the current number of watch->pending_events before the
> xs_read() call. And the watch itself has to be registered before xs_read
> too, if none provided by the caller.
> 
> And later, instead of doing xenbus_watch_wait_event, do:
> uk_waitq_wait_event(&watch->wq, watch->pending_events != 
> saved_counter_value));
> 
> 

As we agreed offline, for v3 I'll only fix the watch registration issue.

>> +                    /* create a local watch */
>> +                    watch = xs_watch_path(XBT_NIL, path);
>> +                    if (PTRISERR(watch)) {
>> +                            err = PTR2ERR(watch);
>> +                            goto out;
>> +                    }
>> +
>> +                    watch_is_local = 1;
>> +            }
>> +
>> +            xenbus_watch_wait_event(watch);
>> +    }
>> +
>> +out:
>> +    if (watch_is_local)
>> +            xs_unwatch(XBT_NIL, watch);
>> +
>> +    return err;
>> +}
>> -- 
>> 2.11.0
>>
> 

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel