[Xen-devel] [PATCH v2 6/7] xenstore: add support for reading directory with many children

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.10.16 at 11:55, <JGross@xxxxxxxx> wrote:
> +static void send_directory_part(struct connection *conn,
> +                             struct buffered_data *in)
> +{
> +     unsigned int off, len, maxlen, genlen;
> +     char *name, *child, *data;
> +     struct node *node;
> +     char gen[24];
> +
> +     if (xs_count_strings(in->buffer, in->used) != 2) {
> +             send_error(conn, EINVAL);
> +             return;
> +     }
> +
> +     /* First arg is node name. */
> +     name = canonicalize(conn, in->buffer);
> +
> +     /* Second arg is childlist offset. */
> +     off = atoi(in->buffer + strlen(in->buffer) + 1);

Not being a user land person, I consider this too weak: Imo using
strtoul() and properly handling conversion errors would be better
here.

> +     node = get_node(conn, in, name, XS_PERM_READ);
> +     if (!node) {
> +             send_error(conn, errno);
> +             return;
> +     }
> +
> +     genlen = snprintf(gen, sizeof(gen), "%"PRIu64, node->generation) + 1;

Why not use the shorter hex representation here?

> +     /* Offset behind list: just return a list with an empty string. */
> +     if (off >= node->childlen) {

Is it perhaps worth separating the == and > cases? The former is
just EOL, while the latter is really an error.

> +             len = strlen(gen) + 2;
> +             gen[len - 1] = 0;
> +             send_reply(conn, XS_DIRECTORY_PART, gen, len);

Any reason not to utilize genlen here, instead of the extra strlen()?

> +             return;
> +     }
> +
> +     len = 0;
> +     maxlen = XENSTORE_PAYLOAD_MAX - genlen - 1;
> +     child = node->children + off;
> +
> +     while (len + strlen(child) < maxlen) {
> +             len += strlen(child) + 1;
> +             child += strlen(child) + 1;
> +             if (off + len == node->childlen)
> +                     break;
> +     }
> +
> +     data = talloc_array(in, char, genlen + len + 1);
> +     if (!data) {
> +             send_error(conn, ENOMEM);
> +             return;
> +     }
> +
> +     strcpy(data, gen);

memcpy(data, gen, genlen); ?

> +     memcpy(data + genlen, node->children + off, len);
> +     if (off + len == node->childlen) {
> +             len++;
> +             data[genlen + len] = 0;
> +     }
> +
> +     send_reply(conn, XS_DIRECTORY_PART, data, genlen + len);
> +}

Since you don't return the offset, am I understanding right that the
remote side is expected to accumulate that value?

Jan

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