|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH v2 09/10] plat/xen: Add driver state functions to client API
On 09/03/2018 08:42 PM, Yuri Volchkov wrote:
> Hi Costin,
>
> a couple of notes inline.
>
> Cheers, 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);
> Why not to use xs_read_integer instead? I actually don't think you need
> xenbus_intstr_to_state at all. Same about xenbus_wait_for_state_change
>
Fixed.
>> +
>> + if (current_state == state)
>> + goto exit; /* state already set */
>> +
>> + /* set new state */
>> + sprintf(new_state_str, "%d", state);
> You have a limited buffer. The snprintf is a must here. Applies to other
> sprintf-s as well.
Fixed.
>
> I guess you did not use xs_printf to safe a bit of a stack? I am not
> objecting to that, just asking.
Yes.
>
>> + 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) {
>> + /* 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |