From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KQOFo-0007Ko-EO for qemu-devel@nongnu.org; Tue, 05 Aug 2008 11:15:16 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KQOFn-0007Jd-S2 for qemu-devel@nongnu.org; Tue, 05 Aug 2008 11:15:15 -0400 Received: from [199.232.76.173] (port=41719 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KQOFn-0007JJ-C1 for qemu-devel@nongnu.org; Tue, 05 Aug 2008 11:15:15 -0400 Received: from mx2.suse.de ([195.135.220.15]:50631) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1KQOFn-0006Yu-13 for qemu-devel@nongnu.org; Tue, 05 Aug 2008 11:15:15 -0400 Message-ID: <48986E34.203@suse.de> Date: Tue, 05 Aug 2008 17:13:56 +0200 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [patch 2/5][v2] Divide get_cluster_offset() References: <20080729141352.573798859@bull.net> <20080729141447.315247848@bull.net> In-Reply-To: <20080729141447.315247848@bull.net> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: qemu-devel@nongnu.org Laurent Vivier schrieb: > - if (allocate == 1) { > - /* allocate a new cluster */ > - cluster_offset = alloc_clusters(bs, s->cluster_size); > - > - /* we must initialize the cluster content which won't be > - written */ > - if ((n_end - n_start) < s->cluster_sectors) { > - uint64_t start_sect; > - > - start_sect = (offset & ~(s->cluster_size - 1)) >> 9; > - ret = copy_sectors(bs, start_sect, > - cluster_offset, 0, n_start); > - if (ret < 0) > - return 0; > - ret = copy_sectors(bs, start_sect, > - cluster_offset, n_end, s->cluster_sectors); > - if (ret < 0) > - return 0; > - } > - tmp = cpu_to_be64(cluster_offset | QCOW_OFLAG_COPIED); > - } else { > + > + if (compressed_size) { > int nb_csectors; > cluster_offset = alloc_bytes(bs, compressed_size); > nb_csectors = ((cluster_offset + compressed_size - 1) >> 9) - > (cluster_offset >> 9); > cluster_offset |= QCOW_OFLAG_COMPRESSED | > ((uint64_t)nb_csectors << s->csize_shift); > - /* compressed clusters never have the copied flag */ > - tmp = cpu_to_be64(cluster_offset); > + > + /* update L2 table > + * compressed clusters never have the copied flag > + */ > + > + l2_table[l2_index] = cpu_to_be64(cluster_offset); > + if (bdrv_pwrite(s->hd, > + l2_offset + l2_index * sizeof(uint64_t), > + l2_table + l2_index, > + sizeof(uint64_t)) != sizeof(uint64_t)) > + return 0; > + > + return cluster_offset; > } > + > + /* allocate a new cluster */ > + > + cluster_offset = alloc_clusters(bs, s->cluster_size); > + > + /* we must initialize the cluster content which won't be > + written */ > + > + if ((n_end - n_start) < s->cluster_sectors) { > + uint64_t start_sect; > + > + start_sect = (offset & ~(s->cluster_size - 1)) >> 9; > + ret = copy_sectors(bs, start_sect, cluster_offset, 0, n_start); > + if (ret < 0) > + return 0; > + ret = copy_sectors(bs, start_sect, > + cluster_offset, n_end, s->cluster_sectors); > + if (ret < 0) > + return 0; > + } > + > /* update L2 table */ > - l2_table[l2_index] = tmp; > + > + l2_table[l2_index] = cpu_to_be64(cluster_offset | QCOW_OFLAG_COPIED); > if (bdrv_pwrite(s->hd, > - l2_offset + l2_index * sizeof(tmp), &tmp, sizeof(tmp)) != sizeof(tmp)) > + l2_offset + l2_index * sizeof(uint64_t), > + l2_table + l2_index, > + sizeof(uint64_t)) != sizeof(uint64_t)) > return 0; > + > return cluster_offset; > } Why are you moving that code around? I don't think it's needed for the get_cluster_offset/allocate_cluster_offset split. You're just duplicating the write and return. Otherwise, the patch looks fine and makes the whole thing much cleaner. Kevin