Mailing List Archive

[PATCH 02/15] libxenstore: Provide xs_check_watch
Event-driven programs want to wait until the xs_fileno triggers for
reading, and then repeatedly call xs_check_watch.

Also xs_read_watch exposes a useless "num" out parameter, which should
always (if things aren't going hideously wrong) be at least 2 and
which the caller shouldn't be interested in. So xs_check_watch
doesn't have one of those.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/xenstore/xs.c | 89 ++++++++++++++++++++++++++++++++++++++++++++-------
tools/xenstore/xs.h | 16 +++++++++
2 files changed, 93 insertions(+), 12 deletions(-)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index df270f7..8e54fe0 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -132,7 +132,23 @@ struct xs_handle {

#endif

-static int read_message(struct xs_handle *h);
+static int read_message(struct xs_handle *h, int nonblocking);
+
+static void setnonblock(int fd, int nonblock) {
+ int esave = errno;
+ int flags = fcntl(fd, F_GETFL);
+ if (flags == -1)
+ goto out;
+
+ if (nonblock)
+ flags |= O_NONBLOCK;
+ else
+ flags &= ~O_NONBLOCK;
+
+ fcntl(fd, F_SETFL, flags);
+out:
+ errno = esave;
+}

int xs_fileno(struct xs_handle *h)
{
@@ -325,8 +341,16 @@ void xs_close(struct xs_handle* xsh)
xs_daemon_close(xsh);
}

-static bool read_all(int fd, void *data, unsigned int len)
+static bool read_all(int fd, void *data, unsigned int len, int nonblocking)
+ /* With nonblocking, either reads either everything requested,
+ * or nothing. */
{
+ if (!len)
+ return true;
+
+ if (nonblocking)
+ setnonblock(fd, 1);
+
while (len) {
int done;

@@ -334,18 +358,28 @@ static bool read_all(int fd, void *data, unsigned int len)
if (done < 0) {
if (errno == EINTR)
continue;
- return false;
+ goto out_false;
}
if (done == 0) {
/* It closed fd on us? EBADF is appropriate. */
errno = EBADF;
- return false;
+ goto out_false;
}
data += done;
len -= done;
+
+ if (nonblocking) {
+ setnonblock(fd, 0);
+ nonblocking = 0;
+ }
}

return true;
+
+out_false:
+ if (nonblocking)
+ setnonblock(fd, 0);
+ return false;
}

#ifdef XSTEST
@@ -374,7 +408,7 @@ static void *read_reply(
read_from_thread = read_thread_exists(h);

/* Read from comms channel ourselves if there is no reader thread. */
- if (!read_from_thread && (read_message(h) == -1))
+ if (!read_from_thread && (read_message(h, 0) == -1))
return NULL;

mutex_lock(&h->reply_mutex);
@@ -693,7 +727,8 @@ bool xs_watch(struct xs_handle *h, const char *path, const char *token)
* Returns array of two pointers: path and token, or NULL.
* Call free() after use.
*/
-char **xs_read_watch(struct xs_handle *h, unsigned int *num)
+static char **read_watch_internal(struct xs_handle *h, unsigned int *num,
+ int nonblocking)
{
struct xs_stored_msg *msg;
char **ret, *strings, c = 0;
@@ -707,14 +742,20 @@ char **xs_read_watch(struct xs_handle *h, unsigned int *num)
* we haven't called xs_watch. Presumably the application
* will do so later; in the meantime we just block.
*/
- while (list_empty(&h->watch_list) && h->fd != -1)
+ while (list_empty(&h->watch_list) && h->fd != -1) {
+ if (nonblocking) {
+ mutex_unlock(&h->watch_mutex);
+ errno = EAGAIN;
+ return 0;
+ }
condvar_wait(&h->watch_condvar, &h->watch_mutex);
+ }
#else /* !defined(USE_PTHREAD) */
/* Read from comms channel ourselves if there are no threads
* and therefore no reader thread. */

assert(!read_thread_exists(h)); /* not threadsafe but worth a check */
- if ((read_message(h) == -1))
+ if ((read_message(h, nonblocking) == -1))
return NULL;

#endif /* !defined(USE_PTHREAD) */
@@ -760,6 +801,24 @@ char **xs_read_watch(struct xs_handle *h, unsigned int *num)
return ret;
}

+char **xs_check_watch(struct xs_handle *h)
+{
+ unsigned int num;
+ char **ret;
+ ret = read_watch_internal(h, &num, 1);
+ if (ret) assert(num >= 2);
+ return ret;
+}
+
+/* Find out what node change was on (will block if nothing pending).
+ * Returns array of two pointers: path and token, or NULL.
+ * Call free() after use.
+ */
+char **xs_read_watch(struct xs_handle *h, unsigned int *num)
+{
+ return read_watch_internal(h, num, 0);
+}
+
/* Remove a watch on a node.
* Returns false on failure (no watch on that node).
*/
@@ -940,11 +999,17 @@ char *xs_debug_command(struct xs_handle *h, const char *cmd,
ARRAY_SIZE(iov), NULL);
}

-static int read_message(struct xs_handle *h)
+static int read_message(struct xs_handle *h, int nonblocking)
{
/* IMPORTANT: It is forbidden to call this function without
* acquiring the request lock and checking that h->read_thr_exists
* is false. See "Lock discipline" in struct xs_handle, above. */
+
+ /* If nonblocking==1, this function will always read either
+ * nothing, returning -1 and setting errno==EAGAIN, or we read
+ * whole amount requested. Ie as soon as we have the start of
+ * the message we block until we get all of it.
+ */

struct xs_stored_msg *msg = NULL;
char *body = NULL;
@@ -956,7 +1021,7 @@ static int read_message(struct xs_handle *h)
if (msg == NULL)
goto error;
cleanup_push(free, msg);
- if (!read_all(h->fd, &msg->hdr, sizeof(msg->hdr))) { /* Cancellation point */
+ if (!read_all(h->fd, &msg->hdr, sizeof(msg->hdr), nonblocking)) { /* Cancellation point */
saved_errno = errno;
goto error_freemsg;
}
@@ -966,7 +1031,7 @@ static int read_message(struct xs_handle *h)
if (body == NULL)
goto error_freemsg;
cleanup_push(free, body);
- if (!read_all(h->fd, body, msg->hdr.len)) { /* Cancellation point */
+ if (!read_all(h->fd, body, msg->hdr.len, 0)) { /* Cancellation point */
saved_errno = errno;
goto error_freebody;
}
@@ -1021,7 +1086,7 @@ static void *read_thread(void *arg)
struct xs_handle *h = arg;
int fd;

- while (read_message(h) != -1)
+ while (read_message(h, 0) != -1)
continue;

/* An error return from read_message leaves the socket in an undefined
diff --git a/tools/xenstore/xs.h b/tools/xenstore/xs.h
index 1cbe255..63f535d 100644
--- a/tools/xenstore/xs.h
+++ b/tools/xenstore/xs.h
@@ -135,6 +135,22 @@ bool xs_watch(struct xs_handle *h, const char *path, const char *token);
/* Return the FD to poll on to see if a watch has fired. */
int xs_fileno(struct xs_handle *h);

+/* Check for node changes. On success, returns a non-NULL pointer ret
+ * such that ret[0] and ret[1] are valid C strings, namely the
+ * triggering path (see docs/misc/xenstore.txt) and the token (from
+ * xs_watch). On error return value is NULL setting errno.
+ *
+ * Callers should, after xs_fileno has become readable, repeatedly
+ * call xs_check_watch until it returns NULL and sets errno to EAGAIN.
+ * (If the fd became readable, xs_check_watch is allowed to make it no
+ * longer show up as readable even if future calls to xs_check_watch
+ * will return more watch events.)
+ *
+ * After the caller is finished with the returned information it
+ * should be freed all in one go with free(ret).
+ */
+char **xs_check_watch(struct xs_handle *h);
+
/* Find out what node change was on (will block if nothing pending).
* Returns array containing the path and token. Use XS_WATCH_* to access these
* elements. Call free() after use.
--
1.7.2.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 02/15] libxenstore: Provide xs_check_watch [ In reply to ]
On Mon, 2011-12-05 at 18:10 +0000, Ian Jackson wrote:
> Event-driven programs want to wait until the xs_fileno triggers for
> reading, and then repeatedly call xs_check_watch.
>
> Also xs_read_watch exposes a useless "num" out parameter, which should
> always (if things aren't going hideously wrong) be at least 2 and
> which the caller shouldn't be interested in. So xs_check_watch
> doesn't have one of those.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Are we supposed to do something to the SONAME with this kind of change?
If not then:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
> tools/xenstore/xs.c | 89 ++++++++++++++++++++++++++++++++++++++++++++-------
> tools/xenstore/xs.h | 16 +++++++++
> 2 files changed, 93 insertions(+), 12 deletions(-)
>
> diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
> index df270f7..8e54fe0 100644
> --- a/tools/xenstore/xs.c
> +++ b/tools/xenstore/xs.c
> @@ -132,7 +132,23 @@ struct xs_handle {
>
> #endif
>
> -static int read_message(struct xs_handle *h);
> +static int read_message(struct xs_handle *h, int nonblocking);
> +
> +static void setnonblock(int fd, int nonblock) {
> + int esave = errno;
> + int flags = fcntl(fd, F_GETFL);
> + if (flags == -1)
> + goto out;
> +
> + if (nonblock)
> + flags |= O_NONBLOCK;
> + else
> + flags &= ~O_NONBLOCK;
> +
> + fcntl(fd, F_SETFL, flags);
> +out:
> + errno = esave;
> +}
>
> int xs_fileno(struct xs_handle *h)
> {
> @@ -325,8 +341,16 @@ void xs_close(struct xs_handle* xsh)
> xs_daemon_close(xsh);
> }
>
> -static bool read_all(int fd, void *data, unsigned int len)
> +static bool read_all(int fd, void *data, unsigned int len, int nonblocking)
> + /* With nonblocking, either reads either everything requested,
> + * or nothing. */
> {
> + if (!len)
> + return true;
> +
> + if (nonblocking)
> + setnonblock(fd, 1);
> +
> while (len) {
> int done;
>
> @@ -334,18 +358,28 @@ static bool read_all(int fd, void *data, unsigned int len)
> if (done < 0) {
> if (errno == EINTR)
> continue;
> - return false;
> + goto out_false;
> }
> if (done == 0) {
> /* It closed fd on us? EBADF is appropriate. */
> errno = EBADF;
> - return false;
> + goto out_false;
> }
> data += done;
> len -= done;
> +
> + if (nonblocking) {
> + setnonblock(fd, 0);
> + nonblocking = 0;
> + }
> }
>
> return true;
> +
> +out_false:
> + if (nonblocking)
> + setnonblock(fd, 0);
> + return false;
> }
>
> #ifdef XSTEST
> @@ -374,7 +408,7 @@ static void *read_reply(
> read_from_thread = read_thread_exists(h);
>
> /* Read from comms channel ourselves if there is no reader thread. */
> - if (!read_from_thread && (read_message(h) == -1))
> + if (!read_from_thread && (read_message(h, 0) == -1))
> return NULL;
>
> mutex_lock(&h->reply_mutex);
> @@ -693,7 +727,8 @@ bool xs_watch(struct xs_handle *h, const char *path, const char *token)
> * Returns array of two pointers: path and token, or NULL.
> * Call free() after use.
> */
> -char **xs_read_watch(struct xs_handle *h, unsigned int *num)
> +static char **read_watch_internal(struct xs_handle *h, unsigned int *num,
> + int nonblocking)
> {
> struct xs_stored_msg *msg;
> char **ret, *strings, c = 0;
> @@ -707,14 +742,20 @@ char **xs_read_watch(struct xs_handle *h, unsigned int *num)
> * we haven't called xs_watch. Presumably the application
> * will do so later; in the meantime we just block.
> */
> - while (list_empty(&h->watch_list) && h->fd != -1)
> + while (list_empty(&h->watch_list) && h->fd != -1) {
> + if (nonblocking) {
> + mutex_unlock(&h->watch_mutex);
> + errno = EAGAIN;
> + return 0;
> + }
> condvar_wait(&h->watch_condvar, &h->watch_mutex);
> + }
> #else /* !defined(USE_PTHREAD) */
> /* Read from comms channel ourselves if there are no threads
> * and therefore no reader thread. */
>
> assert(!read_thread_exists(h)); /* not threadsafe but worth a check */
> - if ((read_message(h) == -1))
> + if ((read_message(h, nonblocking) == -1))
> return NULL;
>
> #endif /* !defined(USE_PTHREAD) */
> @@ -760,6 +801,24 @@ char **xs_read_watch(struct xs_handle *h, unsigned int *num)
> return ret;
> }
>
> +char **xs_check_watch(struct xs_handle *h)
> +{
> + unsigned int num;
> + char **ret;
> + ret = read_watch_internal(h, &num, 1);
> + if (ret) assert(num >= 2);
> + return ret;
> +}
> +
> +/* Find out what node change was on (will block if nothing pending).
> + * Returns array of two pointers: path and token, or NULL.
> + * Call free() after use.
> + */
> +char **xs_read_watch(struct xs_handle *h, unsigned int *num)
> +{
> + return read_watch_internal(h, num, 0);
> +}
> +
> /* Remove a watch on a node.
> * Returns false on failure (no watch on that node).
> */
> @@ -940,11 +999,17 @@ char *xs_debug_command(struct xs_handle *h, const char *cmd,
> ARRAY_SIZE(iov), NULL);
> }
>
> -static int read_message(struct xs_handle *h)
> +static int read_message(struct xs_handle *h, int nonblocking)
> {
> /* IMPORTANT: It is forbidden to call this function without
> * acquiring the request lock and checking that h->read_thr_exists
> * is false. See "Lock discipline" in struct xs_handle, above. */
> +
> + /* If nonblocking==1, this function will always read either
> + * nothing, returning -1 and setting errno==EAGAIN, or we read
> + * whole amount requested. Ie as soon as we have the start of
> + * the message we block until we get all of it.
> + */
>
> struct xs_stored_msg *msg = NULL;
> char *body = NULL;
> @@ -956,7 +1021,7 @@ static int read_message(struct xs_handle *h)
> if (msg == NULL)
> goto error;
> cleanup_push(free, msg);
> - if (!read_all(h->fd, &msg->hdr, sizeof(msg->hdr))) { /* Cancellation point */
> + if (!read_all(h->fd, &msg->hdr, sizeof(msg->hdr), nonblocking)) { /* Cancellation point */
> saved_errno = errno;
> goto error_freemsg;
> }
> @@ -966,7 +1031,7 @@ static int read_message(struct xs_handle *h)
> if (body == NULL)
> goto error_freemsg;
> cleanup_push(free, body);
> - if (!read_all(h->fd, body, msg->hdr.len)) { /* Cancellation point */
> + if (!read_all(h->fd, body, msg->hdr.len, 0)) { /* Cancellation point */
> saved_errno = errno;
> goto error_freebody;
> }
> @@ -1021,7 +1086,7 @@ static void *read_thread(void *arg)
> struct xs_handle *h = arg;
> int fd;
>
> - while (read_message(h) != -1)
> + while (read_message(h, 0) != -1)
> continue;
>
> /* An error return from read_message leaves the socket in an undefined
> diff --git a/tools/xenstore/xs.h b/tools/xenstore/xs.h
> index 1cbe255..63f535d 100644
> --- a/tools/xenstore/xs.h
> +++ b/tools/xenstore/xs.h
> @@ -135,6 +135,22 @@ bool xs_watch(struct xs_handle *h, const char *path, const char *token);
> /* Return the FD to poll on to see if a watch has fired. */
> int xs_fileno(struct xs_handle *h);
>
> +/* Check for node changes. On success, returns a non-NULL pointer ret
> + * such that ret[0] and ret[1] are valid C strings, namely the
> + * triggering path (see docs/misc/xenstore.txt) and the token (from
> + * xs_watch). On error return value is NULL setting errno.
> + *
> + * Callers should, after xs_fileno has become readable, repeatedly
> + * call xs_check_watch until it returns NULL and sets errno to EAGAIN.
> + * (If the fd became readable, xs_check_watch is allowed to make it no
> + * longer show up as readable even if future calls to xs_check_watch
> + * will return more watch events.)
> + *
> + * After the caller is finished with the returned information it
> + * should be freed all in one go with free(ret).
> + */
> +char **xs_check_watch(struct xs_handle *h);
> +
> /* Find out what node change was on (will block if nothing pending).
> * Returns array containing the path and token. Use XS_WATCH_* to access these
> * elements. Call free() after use.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 02/15] libxenstore: Provide xs_check_watch [ In reply to ]
Ian Campbell writes ("Re: [Xen-devel] [PATCH 02/15] libxenstore: Provide xs_check_watch"):
> Are we supposed to do something to the SONAME with this kind of change?

Good point. I think that the MINOR should change since the change is
backwards- but not forwards-compatible.

So I have updated (in my tree) the MINOR and will add your ack.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel