Re: [Qemu-devel] [PATCH] ceph/rbd block driver for qemu-kvm (v6)

[Yehuda Sadeh Weinraub]

See my comments below, updated patch will follow later:

On Tue, Oct 12, 2010 at 3:57 PM, Anthony Liguori <address@hidden> wrote:
...
>> +
>> +static int rbd_parsename(const char *filename, char *pool, char **snap,
>> +                         char *name)
>> +{
>> +    const char *rbdname;
>> +    char *p;
>> +    int l;
>> +
>> +    if (!strstart(filename, "rbd:",&rbdname)) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    pstrcpy(pool, 2 * RBD_MAX_SEG_NAME_SIZE, rbdname);
>>
>
> Passing strings that are expected to be of a certain size is usually bad
> form.  Probably better to pass in the capacity of pool as an additional
> argument.

Yep. Rewrote the function, now gets explicit buffer size for each param.

>
> The limits are really weird here.   Is this a RBD limit or just an internal
> thing?

These are the limits that are used elsewhere for rbd.

>
> So snap ends up pointing to a substring in name?  This memory allocation is
> super frail.  I'd suggest rewriting this to make it more robust such that
> all the strings had clear allocation life cycles.  I'd be amazed if you
> didn't have an overflow/leak today.

Fixed that, was a bad practice.

>> +    *id = out[0];
>> +    le64_to_cpus(out);
>>
>
> You're doing the assignment of the return value before you actually do the
> endian conversion.

Fixed.

>> +
>> +    snprintf(n, RBD_MAX_SEG_NAME_SIZE, "%s%s", name, RBD_SUFFIX);
>>
>>
>
> sizeof(n) is more defensive.

Changed all of these.

>> +    do {
>> +        if ((ret = read(s->fds[RBD_FD_READ],&rcb, sizeof(rcb)))>  0) {
>>
>
> This is sufficiently exotic that it needs a comment.  I think most people's
> first reaction is that the code is a bug and that it should be 'rcb,
> sizeof(*rcb)'.  Passing pointers over a socket is unusual.

Added a comment.

>> +
>> +    return;
>>
>
> The return is unnecessary.

Yeah.

>> +    r = rados_stat(s->header_pool, n,&len, NULL);
>> +    if (r<  0)
>> +        goto failed;
>>
>
> Missing {}s here and many other places.  Please make sure you're following
> CODING_STYLE.
Fixed these and some others.

>> +    char pool[RBD_MAX_SEG_NAME_SIZE];
>> +    char *snap;
>> +    char *hbuf = NULL;
>> +    int r;
>> +
>> +    if (rbd_parsename(filename, pool,&snap, s->name)<  0) {
>> +        return -EINVAL;
>> +    }
>>
>
> In rbd_parsename() you assume pool is 2*RBD_MAX_SEG_NAME_SIZE and here it's
> only RBD_MAX_SEG_NAME_SIZE.

No more such an assumption. pool is only RBD_MAX_SEG_NAME_SIZE.

>> +    if (strncmp(hbuf + 68, RBD_HEADER_VERSION, 8)) {
>> +        error_report("Unknown image version %s", hbuf + 68);
>> +        r = -EMEDIUMTYPE;
>> +        goto failed;
>> +    }
>>
>
> Is strncmp really the right function as opposed to memcmp?

Changed to memcpy.

>> +    fcntl(s->fds[0], F_SETFL, O_NONBLOCK);
>> +    fcntl(s->fds[1], F_SETFL, O_NONBLOCK);
>>
>
> You set this to be O_NONBLOCK but in rbd_aio_event_reader you're not
> gracefully handling EAGAIN and partial reads.

Handling EAGAIN now for both reads and writes.

>> +    RbdHeader1 *header;
>> +
>> +    header = (RbdHeader1 *) hbuf;
>>
>
> Mixing variable definitions with code (didn't I mention this in the last
> review?).

I believe you did. Fixed it now.

>> +    while (1) {
>> +        qemu_free(outbuf);
>> +        outbuf = qemu_malloc(len);
>> +
>> +        r = rados_exec(s->header_pool, n, "rbd", "snap_list", NULL, 0,
>> +                       outbuf, len);
>> +        if (r<  0) {
>> +            error_report("rbd.snap_list execution failed failed: %s",
>> strerror(-r));
>> +            return r;
>> +        }
>> +        if (r != len)
>> +            break;
>> +
>> +        len *= 2;
>> +    }
>>
>
> Is this really the only way to figure out how large the buffer should be??

This is the easiest way. Was trying to avoid reading the rbd header
explicitly for that. Now reading size information from the header, so
that we have an educated guess at what the size would be, however,
this is racy and we'll still loop in case the buffer was too small
(e.g., size was changed after reading the header).

>> +struct rbd_obj_header_ondisk {
>> +    char text[40];
>> +    char block_name[RBD_MAX_BLOCK_NAME_SIZE];
>> +    char signature[4];
>> +    char version[8];
>> +    struct {
>> +        uint8_t order;
>> +        uint8_t crypt_type;
>> +        uint8_t comp_type;
>> +        uint8_t unused;
>> +    } __attribute__((packed)) options;
>> +    uint64_t image_size;
>> +    uint64_t snap_seq;
>> +    uint32_t snap_count;
>> +    uint32_t reserved;
>> +    uint64_t snap_names_len;
>> +    struct rbd_obj_snap_ondisk snaps[0];
>> +} __attribute__((packed));
>> +
>> +
>> +#endif
>>
>
> These types don't match QEMU CODING_STYLE.
>

Might be, however, we also sync this file in two other projects that
have other coding style. It'd be easier for us to keep it as is.


Thanks,
Yehuda