From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51328) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VtpDF-0006uG-Uc for qemu-devel@nongnu.org; Thu, 19 Dec 2013 20:49:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VtpD9-0002Ma-Cf for qemu-devel@nongnu.org; Thu, 19 Dec 2013 20:49:13 -0500 Received: from mx1.redhat.com ([209.132.183.28]:10211) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VtpD9-0002M9-45 for qemu-devel@nongnu.org; Thu, 19 Dec 2013 20:49:07 -0500 Message-ID: <52B3A204.1030808@redhat.com> Date: Fri, 20 Dec 2013 09:48:52 +0800 From: Fam Zheng MIME-Version: 1.0 References: <1387281600-2111-1-git-send-email-famz@redhat.com> <20131219131232.GC2537@stefanha-thinkpad.redhat.com> In-Reply-To: <20131219131232.GC2537@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] vmdk: Allow vmdk_create to work with protocol List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, pl@kamp.de, qemu-devel@nongnu.org, stefanha@redhat.com On 2013=E5=B9=B412=E6=9C=8819=E6=97=A5 21:12, Stefan Hajnoczi wrote: > On Tue, Dec 17, 2013 at 08:00:00PM +0800, Fam Zheng wrote: >> @@ -1511,48 +1521,55 @@ static int vmdk_create_extent(const char *file= name, int64_t filesize, >> header.check_bytes[3] =3D 0xa; >> >> /* write all the data */ >> - ret =3D qemu_write_full(fd, &magic, sizeof(magic)); >> - if (ret !=3D sizeof(magic)) { >> - ret =3D -errno; >> + ret =3D bdrv_pwrite(bs, 0, &magic, sizeof(magic)); >> + if (ret < 0) { >> + error_set(errp, QERR_IO_ERROR); >> goto exit; >> } >> - ret =3D qemu_write_full(fd, &header, sizeof(header)); >> + ret =3D bdrv_pwrite(bs, sizeof(magic), &header, sizeof(header)); >> if (ret !=3D sizeof(header)) { >> + error_set(errp, QERR_IO_ERROR); >> ret =3D -errno; > > This line should be deleted. > > Also, I noticed you changed ret !=3D sizeof(magic) to ret < 0 for the > magic number bdrv_pwrite() but did not change the condition for the > header write. Please keep the error handling condition consistent. > OK. >> goto exit; >> } >> >> - ret =3D ftruncate(fd, le64_to_cpu(header.grain_offset) << 9); >> + ret =3D bdrv_truncate(bs, (le64_to_cpu(header.grain_offset)) << 9= ); > > Why add parentheses around le64_to_cpu()? > Unintended. Will remove. >> if (ret < 0) { >> - ret =3D -errno; >> - goto exit; >> + error_setg(errp, "Could not truncate file"); > > goto exit? > Yes, thanks. >> } >> >> /* write grain directory */ >> - lseek(fd, le64_to_cpu(header.rgd_offset) << 9, SEEK_SET); >> - for (i =3D 0, tmp =3D le64_to_cpu(header.rgd_offset) + gd_size; >> + gd_buf_size =3D gd_sectors * BDRV_SECTOR_SIZE * sizeof(*gd_buf); >> + gd_buf =3D g_malloc0(gd_buf_size); >> + for (i =3D 0, tmp =3D le64_to_cpu(header.rgd_offset) + gd_sectors= ; >> i < gt_count; i++, tmp +=3D gt_size) { >> - ret =3D qemu_write_full(fd, &tmp, sizeof(tmp)); >> - if (ret !=3D sizeof(tmp)) { >> - ret =3D -errno; >> - goto exit; >> - } > > Was this old code not endian-safe? It appears to be writing native > endian values. The new code is different. > Yes, a bonus bug fix. I'll add note in the commit message. >> @@ -1771,33 +1791,34 @@ static int vmdk_create(const char *filename, Q= EMUOptionParameter *options, >> total_size / (int64_t)(63 * number_heads = * 512), >> number_heads, >> adapter_type); >> - if (split || flat) { >> - fd =3D qemu_open(filename, >> - O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LA= RGEFILE, >> - 0644); >> + desc_len =3D strlen(desc); >> + /* the descriptor offset =3D 0x200 */ >> + if (!split && !flat) { >> + desc_offset =3D 0x200; >> } else { >> - fd =3D qemu_open(filename, >> - O_WRONLY | O_BINARY | O_LARGEFILE, >> - 0644); >> + ret =3D bdrv_create_file(filename, options, &local_err); > > Missing error handling if bdrv_create_file() fails. > OK. >> } >> - if (fd < 0) { >> - ret =3D -errno; >> + ret =3D bdrv_file_open(&new_bs, filename, NULL, BDRV_O_RDWR, &loc= al_err); >> + if (ret < 0) { >> + error_setg_errno(errp, -ret, "Could not write description"); >> goto exit; >> } >> - /* the descriptor offset =3D 0x200 */ >> - if (!split && !flat && 0x200 !=3D lseek(fd, 0x200, SEEK_SET)) { >> - ret =3D -errno; >> - goto close_exit; >> + ret =3D bdrv_pwrite(new_bs, desc_offset, desc, desc_len); >> + if (ret < 0) { >> + error_setg_errno(errp, -ret, "Could not write description"); > > goto close_exit? > OK. >> } >> - ret =3D qemu_write_full(fd, desc, strlen(desc)); >> - if (ret !=3D strlen(desc)) { >> - ret =3D -errno; >> - goto close_exit; >> + /* bdrv_pwrite write padding zeros to align to sector, we don't n= eed that >> + * for description file */ >> + if (desc_offset =3D=3D 0) { >> + ret =3D bdrv_truncate(new_bs, desc_offset + desc_len); > > We know desc_offset =3D=3D 0, so desc_offset (0) + desc_len is really j= ust > desc_len. > OK. Thanks for the review! Fam