| Message ID | 1458660792-3035-4-git-send-email-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/22/2016 09:33 AM, Kevin Wolf wrote: > Some features, like I/O throttling, are implemented outside > block-backend.c, but still want to keep BlockBackends in a list. In > order to avoid exposing the whole struct layout in the public header > file, this patch introduces an embedded public struct where list entry > structs can be added and a pair of functions to convert between > BlockBackend and BlockBackendPublic. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/block-backend.c | 17 +++++++++++++++++ > include/sysemu/block-backend.h | 9 +++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/block/block-backend.c b/block/block-backend.c > index df8f717..4394950 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -33,6 +33,7 @@ struct BlockBackend { > DriveInfo *legacy_dinfo; /* null unless created by drive_new() */ > QTAILQ_ENTRY(BlockBackend) link; /* for block_backends */ > QTAILQ_ENTRY(BlockBackend) monitor_link; /* for monitor_block_backends */ > + BlockBackendPublic public; Any reason to not put the public struct at offset 0? > + > +/* > + * Returns a BlockBackend given the associated @public fields. > + */ > +BlockBackend *blk_by_public(BlockBackendPublic *public) > +{ > + return container_of(public, BlockBackend, public); > +} At least container_of() doesn't care, so I guess it doesn't matter. > +/* This struct is embedded in (the private) BlockBackend struct and contains > + * fields that must be public. This is in particular for QLIST_ENTRY() and > + * friends so that BlockBackends can be kept in lists outside block-backend.c */ > +typedef struct BlockBackendPublic { > +} BlockBackendPublic; No fields? So really all we need this for is so that we can have an address of a member of the larger struct, so that we can do list operations based on that address?
Am 22.03.2016 um 22:53 hat Eric Blake geschrieben: > On 03/22/2016 09:33 AM, Kevin Wolf wrote: > > Some features, like I/O throttling, are implemented outside > > block-backend.c, but still want to keep BlockBackends in a list. In > > order to avoid exposing the whole struct layout in the public header > > file, this patch introduces an embedded public struct where list entry > > structs can be added and a pair of functions to convert between > > BlockBackend and BlockBackendPublic. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block/block-backend.c | 17 +++++++++++++++++ > > include/sysemu/block-backend.h | 9 +++++++++ > > 2 files changed, 26 insertions(+) > > > > diff --git a/block/block-backend.c b/block/block-backend.c > > index df8f717..4394950 100644 > > --- a/block/block-backend.c > > +++ b/block/block-backend.c > > @@ -33,6 +33,7 @@ struct BlockBackend { > > DriveInfo *legacy_dinfo; /* null unless created by drive_new() */ > > QTAILQ_ENTRY(BlockBackend) link; /* for block_backends */ > > QTAILQ_ENTRY(BlockBackend) monitor_link; /* for monitor_block_backends */ > > + BlockBackendPublic public; > > Any reason to not put the public struct at offset 0? No, but also no reason to put it there. :-) > > + > > +/* > > + * Returns a BlockBackend given the associated @public fields. > > + */ > > +BlockBackend *blk_by_public(BlockBackendPublic *public) > > +{ > > + return container_of(public, BlockBackend, public); > > +} > > At least container_of() doesn't care, so I guess it doesn't matter. > > > +/* This struct is embedded in (the private) BlockBackend struct and contains > > + * fields that must be public. This is in particular for QLIST_ENTRY() and > > + * friends so that BlockBackends can be kept in lists outside block-backend.c */ > > +typedef struct BlockBackendPublic { > > +} BlockBackendPublic; > > No fields? So really all we need this for is so that we can have an > address of a member of the larger struct, so that we can do list > operations based on that address? Well, a list still needs a QLIST_ENTRY, otherwise I could have directly used BlockBackend. I just tried to keep the introduction of .public separate from the addition of a specific list. I think it's strictly speaking invalid C to have an empty struct, but gcc compiles it and so I thought it should be acceptable to have one for a single commit until something is added there. Kevin
On 03/23/2016 03:09 AM, Kevin Wolf wrote: >>> +++ b/block/block-backend.c >>> @@ -33,6 +33,7 @@ struct BlockBackend { >>> DriveInfo *legacy_dinfo; /* null unless created by drive_new() */ >>> QTAILQ_ENTRY(BlockBackend) link; /* for block_backends */ >>> QTAILQ_ENTRY(BlockBackend) monitor_link; /* for monitor_block_backends */ >>> + BlockBackendPublic public; >> >> Any reason to not put the public struct at offset 0? > > No, but also no reason to put it there. :-) True. >>> +typedef struct BlockBackendPublic { >>> +} BlockBackendPublic; >> >> No fields? So really all we need this for is so that we can have an >> address of a member of the larger struct, so that we can do list >> operations based on that address? > > Well, a list still needs a QLIST_ENTRY, otherwise I could have directly > used BlockBackend. I just tried to keep the introduction of .public > separate from the addition of a specific list. > > I think it's strictly speaking invalid C to have an empty struct, but > gcc compiles it and so I thought it should be acceptable to have one for > a single commit until something is added there. clang gripes; see commit 83ecb22ba. But breaking bisection for one patch on clang-only is different than breaking bisection on all compilers, so up to you what you want to do about it.
Am 23.03.2016 um 22:35 hat Eric Blake geschrieben: > On 03/23/2016 03:09 AM, Kevin Wolf wrote: > >>> +++ b/block/block-backend.c > >>> @@ -33,6 +33,7 @@ struct BlockBackend { > >>> DriveInfo *legacy_dinfo; /* null unless created by drive_new() */ > >>> QTAILQ_ENTRY(BlockBackend) link; /* for block_backends */ > >>> QTAILQ_ENTRY(BlockBackend) monitor_link; /* for monitor_block_backends */ > >>> + BlockBackendPublic public; > >> > >> Any reason to not put the public struct at offset 0? > > > > No, but also no reason to put it there. :-) > > True. > > >>> +typedef struct BlockBackendPublic { > >>> +} BlockBackendPublic; > >> > >> No fields? So really all we need this for is so that we can have an > >> address of a member of the larger struct, so that we can do list > >> operations based on that address? > > > > Well, a list still needs a QLIST_ENTRY, otherwise I could have directly > > used BlockBackend. I just tried to keep the introduction of .public > > separate from the addition of a specific list. > > > > I think it's strictly speaking invalid C to have an empty struct, but > > gcc compiles it and so I thought it should be acceptable to have one for > > a single commit until something is added there. > > clang gripes; see commit 83ecb22ba. > > But breaking bisection for one patch on clang-only is different than > breaking bisection on all compilers, so up to you what you want to do > about it. We can always have an int dummy in there... Kevin
diff --git a/block/block-backend.c b/block/block-backend.c index df8f717..4394950 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -33,6 +33,7 @@ struct BlockBackend { DriveInfo *legacy_dinfo; /* null unless created by drive_new() */ QTAILQ_ENTRY(BlockBackend) link; /* for block_backends */ QTAILQ_ENTRY(BlockBackend) monitor_link; /* for monitor_block_backends */ + BlockBackendPublic public; void *dev; /* attached device model, if any */ /* TODO change to DeviceState when all users are qdevified */ @@ -410,6 +411,22 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo) } /* + * Returns a pointer to the publicly accessible fields of @blk. + */ +BlockBackendPublic *blk_get_public(BlockBackend *blk) +{ + return &blk->public; +} + +/* + * Returns a BlockBackend given the associated @public fields. + */ +BlockBackend *blk_by_public(BlockBackendPublic *public) +{ + return container_of(public, BlockBackend, public); +} + +/* * Disassociates the currently associated BlockDriverState from @blk. */ void blk_remove_bs(BlockBackend *blk) diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index d839bff..bb4ff6f 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -59,6 +59,12 @@ typedef struct BlockDevOps { void (*resize_cb)(void *opaque); } BlockDevOps; +/* This struct is embedded in (the private) BlockBackend struct and contains + * fields that must be public. This is in particular for QLIST_ENTRY() and + * friends so that BlockBackends can be kept in lists outside block-backend.c */ +typedef struct BlockBackendPublic { +} BlockBackendPublic; + BlockBackend *blk_new(Error **errp); BlockBackend *blk_new_with_bs(Error **errp); BlockBackend *blk_new_open(const char *filename, const char *reference, @@ -74,6 +80,9 @@ BlockDriverState *blk_next_root_bs(BlockDriverState *bs); bool monitor_add_blk(BlockBackend *blk, const char *name, Error **errp); void monitor_remove_blk(BlockBackend *blk); +BlockBackendPublic *blk_get_public(BlockBackend *blk); +BlockBackend *blk_by_public(BlockBackendPublic *public); + BlockDriverState *blk_bs(BlockBackend *blk); void blk_remove_bs(BlockBackend *blk); void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs);
Some features, like I/O throttling, are implemented outside block-backend.c, but still want to keep BlockBackends in a list. In order to avoid exposing the whole struct layout in the public header file, this patch introduces an embedded public struct where list entry structs can be added and a pair of functions to convert between BlockBackend and BlockBackendPublic. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/block-backend.c | 17 +++++++++++++++++ include/sysemu/block-backend.h | 9 +++++++++ 2 files changed, 26 insertions(+)