[Xen-devel] [PATCH 1/2] xenstore: add support for reading directory with many children

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.10.16 at 07:52, <JGross@xxxxxxxx> wrote:
> --- a/tools/xenstore/xenstored_transaction.c
> +++ b/tools/xenstore/xenstored_transaction.c
> @@ -79,6 +79,11 @@ struct transaction
>  
>       /* List of changed domains - to record the changed domain entry number 
> */
>       struct list_head changed_domains;
> +
> +     /* Temporary buffer for XS_DIRECTORY_PART. */
> +     char *dirpart_buf;
> +     unsigned buf_off;
> +     unsigned buf_len;
>  };

The buffer you allocate for this can - by nature of this change - be
huge, and there's one per transaction. Isn't this causing a resource
utilization issue?

> @@ -280,6 +285,58 @@ void conn_delete_all_transactions(struct connection 
> *conn)
>       conn->transaction_started = 0;
>  }
>  
> +void send_directory_part(struct connection *conn, struct buffered_data *in)
> +{
> +     struct transaction *trans = conn->transaction;
> +     struct node *node;
> +     const char *name = onearg(in);
> +     unsigned len;
> +
> +     if (name == NULL || trans == NULL) {
> +             send_error(conn, EINVAL);
> +             return;
> +     }
> +
> +     if (name[0] == '@' && name[1] == 0) {
> +             if (trans->dirpart_buf == NULL) {
> +                     send_error(conn, EINVAL);
> +                     return;
> +             }
> +     } else {
> +             if (trans->dirpart_buf) {
> +                     talloc_free(trans->dirpart_buf);
> +                     trans->dirpart_buf = NULL;
> +             }
> +
> +             name = canonicalize(conn, name);
> +             node = get_node(conn, in, name, XS_PERM_READ);
> +             if (!node) {
> +                     send_error(conn, errno);
> +                     return;
> +             }
> +             trans->dirpart_buf = talloc_array(trans, char,
> +                                               node->childlen + 1);

Once again, especially considering the buffer possibly being huge,
shouldn't you check for allocation failure here?

> +             memcpy(trans->dirpart_buf, node->children, node->childlen);
> +             trans->dirpart_buf[node->childlen] = 0;
> +             trans->buf_off = 0;
> +             trans->buf_len = node->childlen + 1;
> +     }
> +
> +     if (trans->buf_len - trans->buf_off > 1024)

What has this 1024 been derived from? In particular, why is it not
(close to) the 4096 limit mentioned in the description?

> +             len = strlen(trans->dirpart_buf + trans->buf_off) + 1;

Looking at construct_node() I'm getting the impression that there
are embedded nul characters in the ->children array, in which case
this strlen() would likely chop off values which could still fit, requiring
needlessly many iterations. And the explicit nul termination after the
allocation above would then also appear to be unnecessary.

And then - why did you put this function here instead of in
xenstored_core.c, e.g. next to send_directory()?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel