[Qemu-devel] Re: [RFC PATCH v4 3/3] block: add sheepdog driver for distributed storage support

[MORITA Kazutaka]

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