From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=48947 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1P6YBl-00051S-0B for qemu-devel@nongnu.org; Thu, 14 Oct 2010 20:30:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1P6YBj-0006zt-HR for qemu-devel@nongnu.org; Thu, 14 Oct 2010 20:30:24 -0400 Received: from mail-wy0-f173.google.com ([74.125.82.173]:51385) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1P6YBj-0006zO-AP for qemu-devel@nongnu.org; Thu, 14 Oct 2010 20:30:23 -0400 Received: by wyb42 with SMTP id 42so187691wyb.4 for ; Thu, 14 Oct 2010 17:30:22 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <4CB4E7C3.8090908@codemonkey.ws> References: <20101012231854.GA4535@dell.home> <4CB4E7C3.8090908@codemonkey.ws> Date: Thu, 14 Oct 2010 17:30:21 -0700 Message-ID: Subject: Re: [Qemu-devel] [PATCH] ceph/rbd block driver for qemu-kvm (v6) From: Yehuda Sadeh Weinraub Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: kwolf@redhat.com, ceph-devel@vger.kernel.org, Christian Brunner , kvm@vger.kernel.org, qemu-devel@nongnu.org See my comments below, updated patch will follow later: On Tue, Oct 12, 2010 at 3:57 PM, Anthony Liguori wr= ote: ... >> + >> +static int rbd_parsename(const char *filename, char *pool, char **snap, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 char *name) >> +{ >> + =A0 =A0const char *rbdname; >> + =A0 =A0char *p; >> + =A0 =A0int l; >> + >> + =A0 =A0if (!strstart(filename, "rbd:",&rbdname)) { >> + =A0 =A0 =A0 =A0return -EINVAL; >> + =A0 =A0} >> + >> + =A0 =A0pstrcpy(pool, 2 * RBD_MAX_SEG_NAME_SIZE, rbdname); >> > > Passing strings that are expected to be of a certain size is usually bad > form. =A0Probably 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. =A0 Is this a RBD limit or just an inte= rnal > thing? These are the limits that are used elsewhere for rbd. > > So snap ends up pointing to a substring in name? =A0This memory allocatio= n is > super frail. =A0I'd suggest rewriting this to make it more robust such th= at > all the strings had clear allocation life cycles. =A0I'd be amazed if you > didn't have an overflow/leak today. Fixed that, was a bad practice. >> + =A0 =A0*id =3D out[0]; >> + =A0 =A0le64_to_cpus(out); >> > > You're doing the assignment of the return value before you actually do th= e > endian conversion. Fixed. >> + >> + =A0 =A0snprintf(n, RBD_MAX_SEG_NAME_SIZE, "%s%s", name, RBD_SUFFIX); >> >> > > sizeof(n) is more defensive. Changed all of these. >> + =A0 =A0do { >> + =A0 =A0 =A0 =A0if ((ret =3D read(s->fds[RBD_FD_READ],&rcb, sizeof(rcb)= ))> =A00) { >> > > This is sufficiently exotic that it needs a comment. =A0I think most peop= le's > first reaction is that the code is a bug and that it should be 'rcb, > sizeof(*rcb)'. =A0Passing pointers over a socket is unusual. Added a comment. >> + >> + =A0 =A0return; >> > > The return is unnecessary. Yeah. >> + =A0 =A0r =3D rados_stat(s->header_pool, n,&len, NULL); >> + =A0 =A0if (r< =A00) >> + =A0 =A0 =A0 =A0goto failed; >> > > Missing {}s here and many other places. =A0Please make sure you're follow= ing > CODING_STYLE. Fixed these and some others. >> + =A0 =A0char pool[RBD_MAX_SEG_NAME_SIZE]; >> + =A0 =A0char *snap; >> + =A0 =A0char *hbuf =3D NULL; >> + =A0 =A0int r; >> + >> + =A0 =A0if (rbd_parsename(filename, pool,&snap, s->name)< =A00) { >> + =A0 =A0 =A0 =A0return -EINVAL; >> + =A0 =A0} >> > > 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. >> + =A0 =A0if (strncmp(hbuf + 68, RBD_HEADER_VERSION, 8)) { >> + =A0 =A0 =A0 =A0error_report("Unknown image version %s", hbuf + 68); >> + =A0 =A0 =A0 =A0r =3D -EMEDIUMTYPE; >> + =A0 =A0 =A0 =A0goto failed; >> + =A0 =A0} >> > > Is strncmp really the right function as opposed to memcmp? Changed to memcpy. >> + =A0 =A0fcntl(s->fds[0], F_SETFL, O_NONBLOCK); >> + =A0 =A0fcntl(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. >> + =A0 =A0RbdHeader1 *header; >> + >> + =A0 =A0header =3D (RbdHeader1 *) hbuf; >> > > Mixing variable definitions with code (didn't I mention this in the last > review?). I believe you did. Fixed it now. >> + =A0 =A0while (1) { >> + =A0 =A0 =A0 =A0qemu_free(outbuf); >> + =A0 =A0 =A0 =A0outbuf =3D qemu_malloc(len); >> + >> + =A0 =A0 =A0 =A0r =3D rados_exec(s->header_pool, n, "rbd", "snap_list",= NULL, 0, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 outbuf, len); >> + =A0 =A0 =A0 =A0if (r< =A00) { >> + =A0 =A0 =A0 =A0 =A0 =A0error_report("rbd.snap_list execution failed fa= iled: %s", >> strerror(-r)); >> + =A0 =A0 =A0 =A0 =A0 =A0return r; >> + =A0 =A0 =A0 =A0} >> + =A0 =A0 =A0 =A0if (r !=3D len) >> + =A0 =A0 =A0 =A0 =A0 =A0break; >> + >> + =A0 =A0 =A0 =A0len *=3D 2; >> + =A0 =A0} >> > > 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 { >> + =A0 =A0char text[40]; >> + =A0 =A0char block_name[RBD_MAX_BLOCK_NAME_SIZE]; >> + =A0 =A0char signature[4]; >> + =A0 =A0char version[8]; >> + =A0 =A0struct { >> + =A0 =A0 =A0 =A0uint8_t order; >> + =A0 =A0 =A0 =A0uint8_t crypt_type; >> + =A0 =A0 =A0 =A0uint8_t comp_type; >> + =A0 =A0 =A0 =A0uint8_t unused; >> + =A0 =A0} __attribute__((packed)) options; >> + =A0 =A0uint64_t image_size; >> + =A0 =A0uint64_t snap_seq; >> + =A0 =A0uint32_t snap_count; >> + =A0 =A0uint32_t reserved; >> + =A0 =A0uint64_t snap_names_len; >> + =A0 =A0struct 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