From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45466) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VtdPU-0008IR-Az for qemu-devel@nongnu.org; Thu, 19 Dec 2013 08:13:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VtdPL-0003al-Tp for qemu-devel@nongnu.org; Thu, 19 Dec 2013 08:13:04 -0500 Received: from mail-we0-x236.google.com ([2a00:1450:400c:c03::236]:43523) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VtdPL-0003ah-JC for qemu-devel@nongnu.org; Thu, 19 Dec 2013 08:12:55 -0500 Received: by mail-we0-f182.google.com with SMTP id q59so1052484wes.13 for ; Thu, 19 Dec 2013 05:12:54 -0800 (PST) Date: Thu, 19 Dec 2013 14:12:32 +0100 From: Stefan Hajnoczi Message-ID: <20131219131232.GC2537@stefanha-thinkpad.redhat.com> References: <1387281600-2111-1-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1387281600-2111-1-git-send-email-famz@redhat.com> 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: Fam Zheng Cc: kwolf@redhat.com, pl@kamp.de, qemu-devel@nongnu.org, stefanha@redhat.com On Tue, Dec 17, 2013 at 08:00:00PM +0800, Fam Zheng wrote: > @@ -1511,48 +1521,55 @@ static int vmdk_create_extent(const char *filename, int64_t filesize, > header.check_bytes[3] = 0xa; > > /* write all the data */ > - ret = qemu_write_full(fd, &magic, sizeof(magic)); > - if (ret != sizeof(magic)) { > - ret = -errno; > + ret = bdrv_pwrite(bs, 0, &magic, sizeof(magic)); > + if (ret < 0) { > + error_set(errp, QERR_IO_ERROR); > goto exit; > } > - ret = qemu_write_full(fd, &header, sizeof(header)); > + ret = bdrv_pwrite(bs, sizeof(magic), &header, sizeof(header)); > if (ret != sizeof(header)) { > + error_set(errp, QERR_IO_ERROR); > ret = -errno; This line should be deleted. Also, I noticed you changed ret != 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. > goto exit; > } > > - ret = ftruncate(fd, le64_to_cpu(header.grain_offset) << 9); > + ret = bdrv_truncate(bs, (le64_to_cpu(header.grain_offset)) << 9); Why add parentheses around le64_to_cpu()? > if (ret < 0) { > - ret = -errno; > - goto exit; > + error_setg(errp, "Could not truncate file"); goto exit? > } > > /* write grain directory */ > - lseek(fd, le64_to_cpu(header.rgd_offset) << 9, SEEK_SET); > - for (i = 0, tmp = le64_to_cpu(header.rgd_offset) + gd_size; > + gd_buf_size = gd_sectors * BDRV_SECTOR_SIZE * sizeof(*gd_buf); > + gd_buf = g_malloc0(gd_buf_size); > + for (i = 0, tmp = le64_to_cpu(header.rgd_offset) + gd_sectors; > i < gt_count; i++, tmp += gt_size) { > - ret = qemu_write_full(fd, &tmp, sizeof(tmp)); > - if (ret != sizeof(tmp)) { > - ret = -errno; > - goto exit; > - } Was this old code not endian-safe? It appears to be writing native endian values. The new code is different. > @@ -1771,33 +1791,34 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options, > total_size / (int64_t)(63 * number_heads * 512), > number_heads, > adapter_type); > - if (split || flat) { > - fd = qemu_open(filename, > - O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, > - 0644); > + desc_len = strlen(desc); > + /* the descriptor offset = 0x200 */ > + if (!split && !flat) { > + desc_offset = 0x200; > } else { > - fd = qemu_open(filename, > - O_WRONLY | O_BINARY | O_LARGEFILE, > - 0644); > + ret = bdrv_create_file(filename, options, &local_err); Missing error handling if bdrv_create_file() fails. > } > - if (fd < 0) { > - ret = -errno; > + ret = bdrv_file_open(&new_bs, filename, NULL, BDRV_O_RDWR, &local_err); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Could not write description"); > goto exit; > } > - /* the descriptor offset = 0x200 */ > - if (!split && !flat && 0x200 != lseek(fd, 0x200, SEEK_SET)) { > - ret = -errno; > - goto close_exit; > + ret = bdrv_pwrite(new_bs, desc_offset, desc, desc_len); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Could not write description"); goto close_exit? > } > - ret = qemu_write_full(fd, desc, strlen(desc)); > - if (ret != strlen(desc)) { > - ret = -errno; > - goto close_exit; > + /* bdrv_pwrite write padding zeros to align to sector, we don't need that > + * for description file */ > + if (desc_offset == 0) { > + ret = bdrv_truncate(new_bs, desc_offset + desc_len); We know desc_offset == 0, so desc_offset (0) + desc_len is really just desc_len.