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

[Juergen Gross <jgross@xxxxxxxx>]

On 25/10/16 11:06, Jan Beulich wrote:
>>>> 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?

It will be allocated only when needed and its size is less than that of
the node to be read.

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

I followed coding style from xenstored. Modifying this style should be
another patch set IMHO (I'd be fine doing this).

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

In theory a single entry could be up to 2048 bytes long. I wanted to
keep the logic simple by not trying to iterate until the limit is
reached. I can change that, of course.

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

Each children member is a nul terminated string, so I need to keep
the nul bytes in the result. And the final nul byte is the indicator
for the last entry being reached.

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

It is using the xenstored_transaction.c private struct transaction.
Putting it in xenstored_core.c would have required to move the
struct definition into a header file.


Juergen

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