[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [RFC PATCH] block: local qiov helper
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [Qemu-block] [RFC PATCH] block: local qiov helper |
Date: |
Tue, 29 Jan 2019 11:31:55 +0800 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Fri, Jan 25, 2019 at 07:46:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all.
>
> What about such a simple helper for a very often patter around
> qemu_iovec_init_external ?
Sounds good, qemu_iovec_init() has 55 references vs
qemu_iovec_init_external() with 51. It's worth making
qemu_iovec_init_external() nicer to use.
> If we like it, I'll update other callers of qemu_iovec_init_external.
>
> Possible interface change would be
> LOCAL_QIOV(lc, buf, len);
> instead of
> LocalQiov lc = LOCAL_QIOV(lc, buf, len);
>
> or, may be, someone has a better idea?
Bike-shedding territory, but I prefer LocalQiov lc = LOCAL_QIOV(lc, buf,
len) because it reveals the type. This makes the code easier to read
than just LOCAL_QIOV(lc, buf, len) by itself - the reader is forced to
look up the macro definition to figure out what magic happens.
> diff --git a/block/io.c b/block/io.c
> index bd9d688f8b..c7d7b199c1 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -949,18 +949,13 @@ int bdrv_preadv(BdrvChild *child, int64_t offset,
> QEMUIOVector *qiov)
>
> int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes)
> {
> - QEMUIOVector qiov;
> - struct iovec iov = {
> - .iov_base = (void *)buf,
> - .iov_len = bytes,
> - };
> + LocalQiov lq = LOCAL_QIOV(lq, buf, bytes);
>
> if (bytes < 0) {
> return -EINVAL;
> }
>
> - qemu_iovec_init_external(&qiov, &iov, 1);
> - return bdrv_preadv(child, offset, &qiov);
> + return bdrv_preadv(child, offset, &lq.qiov);
I think it's unfortunate that LocalQiov is necessary since the caller
only needs the qiov. Can we afford to embed the struct iovec into
QEMUIOVector?
That way callers don't need a separate LocalQiov type:
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
...
return bdrv_preadv(child, offset, &qiov);
signature.asc
Description: PGP signature