[03/12] block: Introduce BlockBackendPublic
diff mbox

Message ID 1458660792-3035-4-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf March 22, 2016, 3:33 p.m. UTC
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(+)

Comments

Eric Blake March 22, 2016, 9:53 p.m. UTC | #1
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?
Kevin Wolf March 23, 2016, 9:09 a.m. UTC | #2
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
Eric Blake March 23, 2016, 9:35 p.m. UTC | #3
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.
Kevin Wolf March 24, 2016, 8:06 a.m. UTC | #4
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

Patch
diff mbox

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);