Hi,
Thank you very much for the reviewing!
At Fri, 14 May 2010 13:08:06 +0200,
Kevin Wolf wrote:
> > +
> > +struct sd_req {
> > + uint8_t proto_ver;
> > + uint8_t opcode;
> > + uint16_t flags;
> > + uint32_t epoch;
> > + uint32_t id;
> > + uint32_t data_length;
> > + uint32_t opcode_specific[8];
> > +};
>
> CODING_STYLE says that structs should be typedefed and their names
> should be in CamelCase. So something like this:
>
> typedef struct SheepdogReq {
> ...
> } SheepdogReq;
>
> (Or, if your prefer, SDReq; but with things like SDAIOCB I think it
> becomes hard to read)
>
I see. I use Sheepdog as a prefix, like SheepdogReq.
> > +/*
> > +
> > +#undef eprintf
> > +#define eprintf(fmt, args...) \
> > +do { \
> > + fprintf(stderr, "%s %d: " fmt, __func__, __LINE__, ##args); \
> > +} while (0)
>
> What about using error_report() instead of fprintf? Though it should be
> the same currently.
>
Yes, using common helper functions is better. I replaced all the
printf.
> > +
> > + for (i = 0; i < ARRAY_SIZE(errors); ++i)
> > + if (errors[i].err == err)
> > + return errors[i].desc;
>
> CODING_STYLE requires braces here.
>
I fixed all the missing braces.
> > +
> > + return "Invalid error code";
> > +}
> > +
> > +static inline int before(uint32_t seq1, uint32_t seq2)
> > +{
> > + return (int32_t)(seq1 - seq2) < 0;
> > +}
> > +
> > +static inline int after(uint32_t seq1, uint32_t seq2)
> > +{
> > + return (int32_t)(seq2 - seq1) < 0;
> > +}
>
> These functions look strange... Is the difference to seq1 < seq2 that
> the cast introduces intentional? (after(0x0, 0xabcdefff) == 1)
>
> If yes, why is this useful? This needs a comment. If no, why even bother
> to have this function instead of directly using < or > ?
>
These functions are used to compare sequential numbers which can be
wrap-around. For example, linux/net/tcp.h in the linux kernel.
Anyway, sheepdog doesn't use these functions, so I removed them.
> > + if (snapid)
> > + dprintf("%" PRIx32 " non current inode was open.\n", vid);
> > + else
> > + s->is_current = 1;
> > +
> > + fd = connect_to_sdog(s->addr);
>
> I wonder why you need to open another connection here instead of using
> s->fd. This pattern repeats at least in the snapshot functions, so I'm
> sure it's there for a reason. Maybe add a comment?
>
We can use s->fd only for aio read/write operations. It is because
the block driver may be during waiting response from the server, so we
cannot send other requests to the discriptor to avoid receiving wrong
data.
I added the comment to get_sheep_fd().
> > +
> > + iov.iov_base = &s->inode;
> > + iov.iov_len = sizeof(s->inode);
> > + aio_req = alloc_aio_req(s, acb, vid_to_vdi_oid(s->inode.vdi_id),
> > + data_len, offset, 0, 0, offset);
> > + if (!aio_req) {
> > + eprintf("too many requests\n");
> > + acb->ret = -EIO;
> > + goto out;
> > + }
>
> Randomly failing requests is probably not a good idea. The guest might
> decide that the disk/file system is broken and stop using it. Can't you
> use a list like in AIOPool, so you can dynamically add new requests as
> needed?
>
I agree. In the v3 patch, AIO requests are allocated dynamically, and
all the requests are linked to the outstanding_aio_head in the
BDRVSheepdogState.
> > +
> > +static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> > +{
> > + struct bdrv_sd_state *s = bs->opaque;
> > + struct bdrv_sd_state *old_s;
> > + char vdi[SD_MAX_VDI_LEN];
> > + char *buf = NULL;
> > + uint32_t vid;
> > + uint32_t snapid = 0;
> > + int ret = -ENOENT, fd;
> > +
> > + old_s = qemu_malloc(sizeof(struct bdrv_sd_state));
> > + if (!old_s) {
>
> qemu_malloc never returns NULL.
>
I removed all the NULL checks.
> > +
> > +BlockDriver bdrv_sheepdog = {
> > + .format_name = "sheepdog",
> > + .protocol_name = "sheepdog",
> > + .instance_size = sizeof(struct bdrv_sd_state),
> > + .bdrv_file_open = sd_open,
> > + .bdrv_close = sd_close,
> > + .bdrv_create = sd_create,
> > +
> > + .bdrv_aio_readv = sd_aio_readv,
> > + .bdrv_aio_writev = sd_aio_writev,
> > +
> > + .bdrv_snapshot_create = sd_snapshot_create,
> > + .bdrv_snapshot_goto = sd_snapshot_goto,
> > + .bdrv_snapshot_delete = sd_snapshot_delete,
> > + .bdrv_snapshot_list = sd_snapshot_list,
> > +
> > + .bdrv_save_vmstate = sd_save_vmstate,
> > + .bdrv_load_vmstate = sd_load_vmstate,
> > +
> > + .create_options = sd_create_options,
> > +};
>
> Please align the = to the same column, at least in each block.
>
I have aligned in the v3 patch.
Thanks,
Kazutaka
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
