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

[Juergen Gross <jgross@xxxxxxxx>]

On 27/10/16 15:56, Jan Beulich wrote:
>>>> 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.

Common practice in xenstored. In case the user is doing silly things
the worst case here is he will receive silly data (either an empty
children list or a list starting in the middle of a child's name).

>> +    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?

Numbers are normally transferred in decimal representation (domid,
transaction id).

>> +    /* 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.

Hmm, yes. I'll modify it.

>> +            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()?

No, you are right.

>> +            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); ?

I don't mind.

> 
>> +    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?

Yes.


Thanks for the review,

Juergen

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