[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [RFC PATCH v4 3/3] block: add sheepdog driver for distr
From: |
MORITA Kazutaka |
Subject: |
[Qemu-devel] Re: [RFC PATCH v4 3/3] block: add sheepdog driver for distributed storage support |
Date: |
Fri, 04 Jun 2010 00:31:09 +0900 |
User-agent: |
Wanderlust/2.14.0 (Africa) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (Gojō) APEL/10.7 Emacs/22.3 (x86_64-pc-linux-gnu) MULE/5.0 (SAKAKI) |
At Tue, 01 Jun 2010 09:58:04 -0500,
Thanks for your comments!
Chris Krumme wrote:
>
> On 05/27/2010 09:44 PM, MORITA Kazutaka wrote:
> > Sheepdog is a distributed storage system for QEMU. It provides highly
> > +
> > +static int connect_to_sdog(const char *addr)
> > +{
> > + char buf[64];
> > + char hbuf[NI_MAXHOST], sbuf[NI_MAXSERV];
> > + char name[256], *p;
> > + int fd, ret;
> > + struct addrinfo hints, *res, *res0;
> > + int port = 0;
> > +
> > + if (!addr) {
> > + addr = SD_DEFAULT_ADDR;
> > + }
> > +
> > + strcpy(name, addr);
> >
>
> Can strlen(addr) be > sizeof(name)?
>
Yes, we should check the length of addr. This would causes overflows.
> > +
> > + p = name;
> > + while (*p) {
> > + if (*p == ':') {
> > + *p++ = '\0';
> >
>
> May also need to check for p > name + sizeof(name).
>
p should be NULL-terminated, so the check is not required, I think.
> > + break;
> > + } else {
> > + p++;
> > + }
> > + }
> > +
> > + if (*p == '\0') {
> > + error_report("cannot find a port number, %s\n", name);
> > + return -1;
> > + }
> > + port = strtol(p, NULL, 10);
> >
>
> Are negative numbers valid here?
>
No. It is better to use strtoul.
> > +
> > +static int parse_vdiname(BDRVSheepdogState *s, const char *filename,
> > + char *vdi, int vdi_len, uint32_t *snapid)
> > +{
> > + char *p, *q;
> > + int nr_sep;
> > +
> > + p = q = strdup(filename);
> > +
> > + if (!p) {
> >
>
> I think Qemu has a version of strdup that will not return NULL.
>
Yes. We can use qemu_strdup here.
> > +
> > +/* TODO: error cleanups */
> > +static int sd_open(BlockDriverState *bs, const char *filename, int flags)
> > +{
> > + int ret, fd;
> > + uint32_t vid = 0;
> > + BDRVSheepdogState *s = bs->opaque;
> > + char vdi[256];
> > + uint32_t snapid;
> > + int for_snapshot = 0;
> > + char *buf;
> > +
> > + strstart(filename, "sheepdog:", (const char **)&filename);
> > +
> > + buf = qemu_malloc(SD_INODE_SIZE);
> > +
> > + memset(vdi, 0, sizeof(vdi));
> > + if (parse_vdiname(s, filename, vdi, sizeof(vdi),&snapid)< 0) {
> > + goto out;
> > + }
> > + s->fd = get_sheep_fd(s);
> > + if (s->fd< 0) {
> >
>
> buf is not freed, goto out maybe.
>
Yes, we should goto out here.
> > +
> > +static int do_sd_create(const char *addr, char *filename, char *tag,
> > + int64_t total_sectors, uint32_t base_vid,
> > + uint32_t *vdi_id, int snapshot)
> > +{
> > + SheepdogVdiReq hdr;
> > + SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
> > + int fd, ret;
> > + unsigned int wlen, rlen = 0;
> > + char buf[SD_MAX_VDI_LEN];
> > +
> > + fd = connect_to_sdog(addr);
> > + if (fd< 0) {
> > + return -1;
> > + }
> > +
> > + strncpy(buf, filename, SD_MAX_VDI_LEN);
> > +
> > + memset(&hdr, 0, sizeof(hdr));
> > + hdr.opcode = SD_OP_NEW_VDI;
> > + hdr.base_vdi_id = base_vid;
> > +
> > + wlen = SD_MAX_VDI_LEN;
> > +
> > + hdr.flags = SD_FLAG_CMD_WRITE;
> > + hdr.snapid = snapshot;
> > +
> > + hdr.data_length = wlen;
> > + hdr.vdi_size = total_sectors * 512;
> >
>
> There is another patch on the list changing 512 to a define for sector size.
>
OK. We'll define SECTOR_SIZE.
> > +
> > + ret = do_req(fd, (SheepdogReq *)&hdr, buf,&wlen,&rlen);
> > +
> > + close(fd);
> > +
> > + if (ret) {
> > + return -1;
> > + }
> > +
> > + if (rsp->result != SD_RES_SUCCESS) {
> > + error_report("%s, %s\n", sd_strerror(rsp->result), filename);
> > + return -1;
> > + }
> > +
> > + if (vdi_id) {
> > + *vdi_id = rsp->vdi_id;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int sd_create(const char *filename, QEMUOptionParameter *options)
> > +{
> > + int ret;
> > + uint32_t vid = 0;
> > + int64_t total_sectors = 0;
> > + char *backing_file = NULL;
> > +
> > + strstart(filename, "sheepdog:", (const char **)&filename);
> > +
> > + while (options&& options->name) {
> > + if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> > + total_sectors = options->value.n / 512;
> >
> Use define.
> > + } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
> > + backing_file = options->value.s;
> > + }
> > + options++;
> > + }
> > +
> > + if (backing_file) {
> > + BlockDriverState bs;
> > + char vdi[SD_MAX_VDI_LEN];
> > + uint32_t snapid;
> > +
> > + strstart(backing_file, "sheepdog:", (const char
> > **)&backing_file);
> > + memset(&bs, 0, sizeof(bs));
> > +
> > + bs.opaque = qemu_malloc(sizeof(BDRVSheepdogState));
> >
>
> bs seems to have a short life span, is opaque getting freed?
>
No, we should free it.
> > +
> > +static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo
> > *sn_info)
> > +{
> > + BDRVSheepdogState *s = bs->opaque;
> > + int ret, fd;
> > + uint32_t new_vid;
> > + SheepdogInode *inode;
> > + unsigned int datalen;
> > + uint64_t offset;
> > +
> > + dprintf("sn_info: name %s id_str %s s: name %s vm_state_size %d "
> > + "is_current %d\n", sn_info->name, sn_info->id_str,
> > + s->name, sn_info->vm_state_size, s->is_current);
> > +
> > + if (!s->is_current) {
> > + error_report("You can't create a snapshot of "
> > + "a non current VDI, %s (%" PRIu32 ").\n",
> > + s->name, s->inode.vdi_id);
> > +
> > + return -1;
> > + }
> > +
> > + dprintf("%s %s\n", sn_info->name, sn_info->id_str);
> > +
> > + s->inode.vm_state_size = sn_info->vm_state_size;
> > + s->inode.vm_clock_nsec = sn_info->vm_clock_nsec;
> > + offset = 0;
> > + /* we don't need to read entire object */
> > + datalen = SD_INODE_SIZE - sizeof(s->inode.data_vdi_id);
> > +
> > + /* refresh inode. */
> > + fd = connect_to_sdog(s->addr);
> > + if (fd< 0) {
> > + ret = -EIO;
> > + goto cleanup;
> > + }
> > +
> > + ret = write_object(fd, (char *)&s->inode,
> > vid_to_vdi_oid(s->inode.vdi_id),
> > + s->inode.nr_copies, datalen, offset, 0);
> > + if (ret< 0) {
> > + error_report("failed to write snapshot's inode.\n");
> > + ret = -EIO;
> > + goto cleanup;
> > + }
> > +
> > + ret = do_sd_create(s->addr, s->name, NULL, s->inode.vdi_size>> 9,
> > + s->inode.vdi_id,&new_vid, 1);
> > + if (ret< 0) {
> > + error_report("failed to create inode for snapshot. %m\n");
> > + ret = -EIO;
> > + goto cleanup;
> > + }
> > +
> > + inode = (SheepdogInode *)qemu_malloc(datalen);
> > +
> > + ret = read_object(fd, (char *)inode, vid_to_vdi_oid(new_vid),
> > + s->inode.nr_copies, datalen, offset);
> > +
> > + close(fd);
> >
>
> Should you close fd twice, or let it fall through.
>
We should remove close() here.
I'll address the comments in my next post.
Thanks,
Kazutaka