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