| Message ID | 1462406126-22946-3-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 05.05.2016 um 01:55 hat Eric Blake geschrieben: > I was thrown by the fact that the public type BlockRequest had > an anonymous union, but no obvious discriminator. Turns out > that the only client of the second branch of the union was code > internal to io.c, and that with a slight abuse of QEMUIOVector* > to pass a void* pointer, we can make the public interface less > confusing. > > (Yes, I know that strict C doesn't guarantee that you can cast > void* to the wrong type and then back to void* - it only > guarantees the reverse direction of the original pointer to > void* and back to the original type - but we already have other > assumptions throughout the qemu code base that assume that all > pointers are interchangeable in representation). > > Signed-off-by: Eric Blake <eblake@redhat.com> Do you really think abusing fields makes things clearer than using a union? We could just add comments instead that tell which branch is used for what. And after my patch "block: Remove bdrv_aio_multiwrite()", I think the struct isn't part of a public interface any more anyway. Kevin
On 05/06/2016 04:37 AM, Kevin Wolf wrote: > Am 05.05.2016 um 01:55 hat Eric Blake geschrieben: >> I was thrown by the fact that the public type BlockRequest had >> an anonymous union, but no obvious discriminator. Turns out >> that the only client of the second branch of the union was code >> internal to io.c, and that with a slight abuse of QEMUIOVector* >> to pass a void* pointer, we can make the public interface less >> confusing. >> >> (Yes, I know that strict C doesn't guarantee that you can cast >> void* to the wrong type and then back to void* - it only >> guarantees the reverse direction of the original pointer to >> void* and back to the original type - but we already have other >> assumptions throughout the qemu code base that assume that all >> pointers are interchangeable in representation). >> >> Signed-off-by: Eric Blake <eblake@redhat.com> > > Do you really think abusing fields makes things clearer than using a > union? We could just add comments instead that tell which branch is used > for what. And after my patch "block: Remove bdrv_aio_multiwrite()", I > think the struct isn't part of a public interface any more anyway. Oh, you're right! Consider this patch severed from the series (I may still send a documentation patch, and/or move the struct out of the public header into block_int.h, but it is sufficiently unrelated to the rest of the series).
diff --git a/include/block/block.h b/include/block/block.h index 0e8b4d1..754aae3 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -334,18 +334,10 @@ void bdrv_aio_cancel_async(BlockAIOCB *acb); typedef struct BlockRequest { /* Fields to be filled by multiwrite caller */ - union { - struct { - int64_t sector; - int nb_sectors; - int flags; - QEMUIOVector *qiov; - }; - struct { - int req; - void *buf; - }; - }; + int64_t sector; + int nb_sectors; + int flags; + QEMUIOVector *qiov; BlockCompletionFunc *cb; void *opaque; diff --git a/block/io.c b/block/io.c index 0db1146..f15c0f4 100644 --- a/block/io.c +++ b/block/io.c @@ -2614,7 +2614,7 @@ static void coroutine_fn bdrv_co_aio_ioctl_entry(void *opaque) { BlockAIOCBCoroutine *acb = opaque; acb->req.error = bdrv_co_do_ioctl(acb->common.bs, - acb->req.req, acb->req.buf); + acb->req.flags, acb->req.qiov); bdrv_co_complete(acb); } @@ -2628,8 +2628,10 @@ BlockAIOCB *bdrv_aio_ioctl(BlockDriverState *bs, acb->need_bh = true; acb->req.error = -EINPROGRESS; - acb->req.req = req; - acb->req.buf = buf; + /* Slight type abuse here, so we don't have to expose extra fields + * in the public struct BlockRequest */ + acb->req.flags = req; + acb->req.qiov = buf; co = qemu_coroutine_create(bdrv_co_aio_ioctl_entry); qemu_coroutine_enter(co, acb);
I was thrown by the fact that the public type BlockRequest had an anonymous union, but no obvious discriminator. Turns out that the only client of the second branch of the union was code internal to io.c, and that with a slight abuse of QEMUIOVector* to pass a void* pointer, we can make the public interface less confusing. (Yes, I know that strict C doesn't guarantee that you can cast void* to the wrong type and then back to void* - it only guarantees the reverse direction of the original pointer to void* and back to the original type - but we already have other assumptions throughout the qemu code base that assume that all pointers are interchangeable in representation). Signed-off-by: Eric Blake <eblake@redhat.com> --- include/block/block.h | 16 ++++------------ block/io.c | 8 +++++--- 2 files changed, 9 insertions(+), 15 deletions(-)