From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57822) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ys99A-0000Xg-Pi for qemu-devel@nongnu.org; Tue, 12 May 2015 08:18:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ys992-0002W3-2W for qemu-devel@nongnu.org; Tue, 12 May 2015 08:18:52 -0400 Date: Tue, 12 May 2015 13:18:38 +0100 From: Stefan Hajnoczi Message-ID: <20150512121838.GC11497@stefanha-thinkpad.redhat.com> References: <1431410972-13087-1-git-send-email-famz@redhat.com> <1431410972-13087-3-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="OBd5C1Lgu00Gd/Tn" Content-Disposition: inline In-Reply-To: <1431410972-13087-3-git-send-email-famz@redhat.com> Subject: Re: [Qemu-devel] [PATCH v6 2/3] block: Fix NULL deference for unaligned write if qiov is NULL List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: Kevin Wolf , pbonzini@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org, qemu-stable@nongnu.org --OBd5C1Lgu00Gd/Tn Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, May 12, 2015 at 02:09:31PM +0800, Fam Zheng wrote: > +static int coroutine_fn bdrv_co_do_zero_pwritev(BlockDriverState *bs, > + int64_t offset, > + unsigned int bytes, > + BdrvRequestFlags flags) > +{ > + BdrvTrackedRequest req; > + uint8_t *buf = NULL; > + QEMUIOVector local_qiov; > + struct iovec iov; > + uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment); > + unsigned int head_padding_bytes, tail_padding_bytes; > + int ret; > + > + head_padding_bytes = offset & (align - 1); > + tail_padding_bytes = align - ((offset + bytes) & (align - 1)); > + tracked_request_begin(&req, bs, offset, bytes, true); > + > + mark_request_serialising(&req, align); This is only necessary if (head_padding_bytes || tail_padding_bytes). Serialized requests are more expensive so let's only do it when necessary. > + wait_serialising_requests(&req); > + > + assert(flags & BDRV_REQ_ZERO_WRITE); > + if (head_padding_bytes || tail_padding_bytes) { > + buf = qemu_blockalign(bs, align); > + iov = (struct iovec) { > + .iov_base = buf, > + .iov_len = align, > + }; > + qemu_iovec_init_external(&local_qiov, &iov, 1); > + } > + if (head_padding_bytes) { > + uint64_t zero_bytes = MIN(bytes, align - head_padding_bytes); > + > + /* RMW the unaligned part before head. */ > + BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_HEAD); > + ret = bdrv_aligned_preadv(bs, &req, offset & ~(align - 1), align, > + align, &local_qiov, 0); > + if (ret < 0) { > + goto fail; > + } > + BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD); > + > + memset(buf + head_padding_bytes, 0, zero_bytes); > + ret = bdrv_aligned_pwritev(bs, &req, offset & ~(align - 1), align, > + &local_qiov, > + flags & ~BDRV_REQ_ZERO_WRITE); > + if (ret < 0) { > + goto fail; > + } > + offset += zero_bytes; > + bytes -= zero_bytes; > + } > + > + assert((offset & (align - 1)) == 0); > + if (bytes >= align) { > + /* Write the aligned part in the middle. */ > + uint64_t aligned_bytes = bytes & ~(align - 1); > + ret = bdrv_aligned_pwritev(bs, &req, offset, aligned_bytes, > + NULL, flags); > + if (ret < 0) { > + goto fail; > + } > + bytes -= aligned_bytes; > + offset += aligned_bytes; > + } > + > + assert((offset & (align - 1)) == 0); > + if (bytes) { > + assert(align == tail_padding_bytes + bytes); > + /* RMW the unaligned part after tail. */ > + BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_TAIL); > + ret = bdrv_aligned_preadv(bs, &req, offset, align, > + align, &local_qiov, 0); > + if (ret < 0) { > + goto fail; > + } > + BLKDBG_EVENT(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL); > + > + memset(buf, 0, bytes); > + printf("tail part %ld %d\n", offset, bytes); Please drop the debug printf. > + ret = bdrv_aligned_pwritev(bs, &req, offset, align, > + &local_qiov, flags & ~BDRV_REQ_ZERO_WRITE); > + } > +fail: > + tracked_request_end(&req); > + if (buf) { > + qemu_vfree(buf); > + } if (buf) is unnecessary. qemu_vfree(NULL) is a nop. --OBd5C1Lgu00Gd/Tn Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVUe+eAAoJEJykq7OBq3PINAEH+wWdyRiybyT4+t3IDUtOP7V2 W7I/mcOnZy2Dq5FaOKX/lvCxZegWluLZe3PFLiPJ6JFOpjioPDdgSZO6i+7fEqL5 uwOWyolkxDYJEXRMkbP1loxmbsCZwKKqZEfRxv7fZG24Rmss1uaWWyOyRvq0taNL 4m5SEpUtL+0QSvrBlCYbt2DYELHKL8u1+Ko5E29YpNzcjMRBHuaaLxQoO+g1hdla 1DnqgFCutejtPjCw0meaISItYFbjWQ5GCCTOrXGUI3V6YxSOYRl3++fC+Ln8CR65 WQH2casYMfpO7DrX3XkqKq7t8rFaNVFZRVLgRtwkD1kISWQfMX3Q+AkangP/YGs= =8zDh -----END PGP SIGNATURE----- --OBd5C1Lgu00Gd/Tn--