Re: [Qemu-block] [PATCH 2/3] block-backend: Remember if attached device is non-qdev

2016-10-06 Thread Kevin Wolf 
Am 05.10.2016 um 20:01 hat Max Reitz geschrieben:
> On 05.10.2016 11:26, Kevin Wolf wrote:
> > Almost all block devices are qdevified by now. This allows us to go back
> > from the BlockBackend to the DeviceState. xen_disk is the last device
> > that is missing. We'll remember in the BlockBackend if a xen_disk is
> > attached and can then disable any features that require going from a BB
> > to the DeviceState.
> > 
> > While at it, clearly mark the function used by xen_disk as legacy even
> > in its name, not just in TODO comments.
> > 
> > Signed-off-by: Kevin Wolf 

> > +/*
> > + * Attach device model @dev to @blk.
> >   * @blk must not have a device model attached already.
> >   * TODO qdevified devices don't use this, remove when devices are qdevified
> >   */
> > -void blk_attach_dev_nofail(BlockBackend *blk, void *dev)
> > +void blk_attach_dev_legacy(BlockBackend *blk, void *dev)
> >  {
> >  if (blk_attach_dev(blk, dev) < 0) {
> 
> I'd make this a blk_do_attach_dev(), but it's not syntactically wrong,
> so the choice is up to you.

Thanks for catching this, what you suggest is what I intended. Maybe we
should just enable the gcc --do-what-i-mean option.

Kevin


pgpJdrkx7tdF_.pgp
Description: PGP signature

Re: [Qemu-block] [PATCH 2/3] block-backend: Remember if attached device is non-qdev

2016-10-05 Thread Eric Blake 
On 10/05/2016 01:01 PM, Max Reitz wrote:

>> +static int blk_do_attach_dev(BlockBackend *blk, void *dev)

>> + */
>> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev)
>> +{
>> +return blk_do_attach_dev(blk, dev);

>> +void blk_attach_dev_legacy(BlockBackend *blk, void *dev)
>>  {
>>  if (blk_attach_dev(blk, dev) < 0) {
> 
> I'd make this a blk_do_attach_dev(), but it's not syntactically wrong,
> so the choice is up to you.

It's technically undefined C behavior to cast from void* to an unrelated
pointer and back to void* (you are only guaranteed a round trip from
type to void* and back to original type, not with unrelated types).  So
syntactically valid but semantically undefined.

On the other hand, ABI-wise, I'd be shocked if our compilers are able to
exploit our use of undefined behavior.

At any rate, using blk_do_attach_dev() would avoid the definedness
problem, so I'd go ahead and make the change.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 2/3] block-backend: Remember if attached device is non-qdev

2016-10-05 Thread Max Reitz 
On 05.10.2016 11:26, Kevin Wolf wrote:
> Almost all block devices are qdevified by now. This allows us to go back
> from the BlockBackend to the DeviceState. xen_disk is the last device
> that is missing. We'll remember in the BlockBackend if a xen_disk is
> attached and can then disable any features that require going from a BB
> to the DeviceState.
> 
> While at it, clearly mark the function used by xen_disk as legacy even
> in its name, not just in TODO comments.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/block-backend.c  | 26 +++---
>  hw/block/xen_disk.c|  2 +-
>  include/sysemu/block-backend.h |  4 ++--
>  3 files changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 911254a..39c5613 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c

[...]

> @@ -507,32 +508,38 @@ void blk_insert_bs(BlockBackend *blk, BlockDriverState 
> *bs)
>  }
>  }
>  
> -/*
> - * Attach device model @dev to @blk.
> - * Return 0 on success, -EBUSY when a device model is attached already.
> - */
> -int blk_attach_dev(BlockBackend *blk, void *dev)
> -/* TODO change to DeviceState *dev when all users are qdevified */
> +static int blk_do_attach_dev(BlockBackend *blk, void *dev)
>  {
>  if (blk->dev) {
>  return -EBUSY;
>  }
>  blk_ref(blk);
>  blk->dev = dev;
> +blk->legacy_dev = false;
>  blk_iostatus_reset(blk);
>  return 0;
>  }
>  
>  /*
>   * Attach device model @dev to @blk.
> + * Return 0 on success, -EBUSY when a device model is attached already.
> + */
> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev)
> +{
> +return blk_do_attach_dev(blk, dev);
> +}
> +
> +/*
> + * Attach device model @dev to @blk.
>   * @blk must not have a device model attached already.
>   * TODO qdevified devices don't use this, remove when devices are qdevified
>   */
> -void blk_attach_dev_nofail(BlockBackend *blk, void *dev)
> +void blk_attach_dev_legacy(BlockBackend *blk, void *dev)
>  {
>  if (blk_attach_dev(blk, dev) < 0) {

I'd make this a blk_do_attach_dev(), but it's not syntactically wrong,
so the choice is up to you.

Thus, either way:

Reviewed-by: Max Reitz 

>  abort();
>  }
> +blk->legacy_dev = true;
>  }
>  
>  /*



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH 2/3] block-backend: Remember if attached device is non-qdev

2016-10-05 Thread Kevin Wolf 
Almost all block devices are qdevified by now. This allows us to go back
from the BlockBackend to the DeviceState. xen_disk is the last device
that is missing. We'll remember in the BlockBackend if a xen_disk is
attached and can then disable any features that require going from a BB
to the DeviceState.

While at it, clearly mark the function used by xen_disk as legacy even
in its name, not just in TODO comments.

Signed-off-by: Kevin Wolf 
---
 block/block-backend.c  | 26 +++---
 hw/block/xen_disk.c|  2 +-
 include/sysemu/block-backend.h |  4 ++--
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 911254a..39c5613 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -38,6 +38,7 @@ struct BlockBackend {
 BlockBackendPublic public;
 
 void *dev;  /* attached device model, if any */
+bool legacy_dev;/* true if dev is not a DeviceState */
 /* TODO change to DeviceState when all users are qdevified */
 const BlockDevOps *dev_ops;
 void *dev_opaque;
@@ -507,32 +508,38 @@ void blk_insert_bs(BlockBackend *blk, BlockDriverState 
*bs)
 }
 }
 
-/*
- * Attach device model @dev to @blk.
- * Return 0 on success, -EBUSY when a device model is attached already.
- */
-int blk_attach_dev(BlockBackend *blk, void *dev)
-/* TODO change to DeviceState *dev when all users are qdevified */
+static int blk_do_attach_dev(BlockBackend *blk, void *dev)
 {
 if (blk->dev) {
 return -EBUSY;
 }
 blk_ref(blk);
 blk->dev = dev;
+blk->legacy_dev = false;
 blk_iostatus_reset(blk);
 return 0;
 }
 
 /*
  * Attach device model @dev to @blk.
+ * Return 0 on success, -EBUSY when a device model is attached already.
+ */
+int blk_attach_dev(BlockBackend *blk, DeviceState *dev)
+{
+return blk_do_attach_dev(blk, dev);
+}
+
+/*
+ * Attach device model @dev to @blk.
  * @blk must not have a device model attached already.
  * TODO qdevified devices don't use this, remove when devices are qdevified
  */
-void blk_attach_dev_nofail(BlockBackend *blk, void *dev)
+void blk_attach_dev_legacy(BlockBackend *blk, void *dev)
 {
 if (blk_attach_dev(blk, dev) < 0) {
 abort();
 }
+blk->legacy_dev = true;
 }
 
 /*
@@ -586,6 +593,11 @@ BlockBackend *blk_by_dev(void *dev)
 void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops,
  void *opaque)
 {
+/* All drivers that use blk_set_dev_ops() are qdevified and we want to keep
+ * it that way, so we can assume blk->dev is a DeviceState if blk->dev_ops
+ * is set. */
+assert(!blk->legacy_dev);
+
 blk->dev_ops = ops;
 blk->dev_opaque = opaque;
 }
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 5aa350a..1292a4b 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -1079,7 +1079,7 @@ static int blk_connect(struct XenDevice *xendev)
  * so we can blk_unref() unconditionally */
 blk_ref(blkdev->blk);
 }
-blk_attach_dev_nofail(blkdev->blk, blkdev);
+blk_attach_dev_legacy(blkdev->blk, blkdev);
 blkdev->file_size = blk_getlength(blkdev->blk);
 if (blkdev->file_size < 0) {
 BlockDriverState *bs = blk_bs(blkdev->blk);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index a7993af..b07159b 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -107,8 +107,8 @@ BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk);
 void blk_iostatus_disable(BlockBackend *blk);
 void blk_iostatus_reset(BlockBackend *blk);
 void blk_iostatus_set_err(BlockBackend *blk, int error);
-int blk_attach_dev(BlockBackend *blk, void *dev);
-void blk_attach_dev_nofail(BlockBackend *blk, void *dev);
+int blk_attach_dev(BlockBackend *blk, DeviceState *dev);
+void blk_attach_dev_legacy(BlockBackend *blk, void *dev);
 void blk_detach_dev(BlockBackend *blk, void *dev);
 void *blk_get_attached_dev(BlockBackend *blk);
 BlockBackend *blk_by_dev(void *dev);
-- 
1.8.3.1