Re: [Qemu-devel] [PATCH v7 13/24] virtio-scsi: Catch BDS-BB removal/insertion

2015-11-25 Thread Max Reitz 
On 25.11.2015 17:03, Kevin Wolf wrote:
> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
>> Make use of the BDS-BB removal and insertion notifiers to remove or set
>> up, respectively, virtio-scsi's op blockers.
>>
>> Signed-off-by: Max Reitz 
> 
>> @@ -797,6 +830,29 @@ static void virtio_scsi_hotunplug(HotplugHandler 
>> *hotplug_dev, DeviceState *dev,
>>  if (s->ctx) {
>>  blk_op_unblock_all(sd->conf.blk, s->blocker);
>>  }
>> +
>> +QTAILQ_FOREACH(insert_notifier, &s->insert_notifiers, next) {
>> +if (insert_notifier->sd == sd) {
>> +break;
>> +}
>> +}
>> +if (insert_notifier) {
>> +notifier_remove(&insert_notifier->n);
>> +QTAILQ_REMOVE(&s->insert_notifiers, insert_notifier, next);
>> +g_free(insert_notifier);
>> +}
> 
> Why a separate if block instead of just doing that inside the loop?

Because I unconsciously try to reduce indentation.

Also, because when I wrote the code, to me it was "Find the relevant
notifier -- destroy that notifier", and that's how this pattern came about.

I personally still like it more the way I did it, but I can very well
imagine that I'm the only one who thinks so. Therefore, I wouldn't mind
changing it (besides the effort involved to change it).

Max

>> +QTAILQ_FOREACH(remove_notifier, &s->remove_notifiers, next) {
>> +if (remove_notifier->sd == sd) {
>> +break;
>> +}
>> +}
>> +if (remove_notifier) {
>> +notifier_remove(&remove_notifier->n);
>> +QTAILQ_REMOVE(&s->remove_notifiers, remove_notifier, next);
>> +g_free(remove_notifier);
>> +}
>> +
>>  qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
>>  }
> 
> Kevin
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v7 13/24] virtio-scsi: Catch BDS-BB removal/insertion

2015-11-25 Thread Kevin Wolf 
Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> Make use of the BDS-BB removal and insertion notifiers to remove or set
> up, respectively, virtio-scsi's op blockers.
> 
> Signed-off-by: Max Reitz 

> @@ -797,6 +830,29 @@ static void virtio_scsi_hotunplug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  if (s->ctx) {
>  blk_op_unblock_all(sd->conf.blk, s->blocker);
>  }
> +
> +QTAILQ_FOREACH(insert_notifier, &s->insert_notifiers, next) {
> +if (insert_notifier->sd == sd) {
> +break;
> +}
> +}
> +if (insert_notifier) {
> +notifier_remove(&insert_notifier->n);
> +QTAILQ_REMOVE(&s->insert_notifiers, insert_notifier, next);
> +g_free(insert_notifier);
> +}

Why a separate if block instead of just doing that inside the loop?

> +QTAILQ_FOREACH(remove_notifier, &s->remove_notifiers, next) {
> +if (remove_notifier->sd == sd) {
> +break;
> +}
> +}
> +if (remove_notifier) {
> +notifier_remove(&remove_notifier->n);
> +QTAILQ_REMOVE(&s->remove_notifiers, remove_notifier, next);
> +g_free(remove_notifier);
> +}
> +
>  qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
>  }

Kevin

[Qemu-devel] [PATCH v7 13/24] virtio-scsi: Catch BDS-BB removal/insertion

2015-11-09 Thread Max Reitz 
Make use of the BDS-BB removal and insertion notifiers to remove or set
up, respectively, virtio-scsi's op blockers.

Signed-off-by: Max Reitz 
---
 hw/scsi/virtio-scsi.c   | 59 +
 include/hw/virtio/virtio-scsi.h | 10 +++
 2 files changed, 69 insertions(+)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 7655401..02606e6 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -757,6 +757,22 @@ static void virtio_scsi_change(SCSIBus *bus, SCSIDevice 
*dev, SCSISense sense)
 }
 }
 
+static void virtio_scsi_blk_insert_notifier(Notifier *n, void *data)
+{
+VirtIOSCSIBlkChangeNotifier *cn = DO_UPCAST(VirtIOSCSIBlkChangeNotifier,
+n, n);
+assert(cn->sd->conf.blk == data);
+blk_op_block_all(cn->sd->conf.blk, cn->s->blocker);
+}
+
+static void virtio_scsi_blk_remove_notifier(Notifier *n, void *data)
+{
+VirtIOSCSIBlkChangeNotifier *cn = DO_UPCAST(VirtIOSCSIBlkChangeNotifier,
+n, n);
+assert(cn->sd->conf.blk == data);
+blk_op_unblock_all(cn->sd->conf.blk, cn->s->blocker);
+}
+
 static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
 Error **errp)
 {
@@ -765,6 +781,22 @@ static void virtio_scsi_hotplug(HotplugHandler 
*hotplug_dev, DeviceState *dev,
 SCSIDevice *sd = SCSI_DEVICE(dev);
 
 if (s->ctx && !s->dataplane_disabled) {
+VirtIOSCSIBlkChangeNotifier *insert_notifier, *remove_notifier;
+
+insert_notifier = g_new0(VirtIOSCSIBlkChangeNotifier, 1);
+insert_notifier->n.notify = virtio_scsi_blk_insert_notifier;
+insert_notifier->s = s;
+insert_notifier->sd = sd;
+blk_add_insert_bs_notifier(sd->conf.blk, &insert_notifier->n);
+QTAILQ_INSERT_TAIL(&s->insert_notifiers, insert_notifier, next);
+
+remove_notifier = g_new0(VirtIOSCSIBlkChangeNotifier, 1);
+remove_notifier->n.notify = virtio_scsi_blk_remove_notifier;
+remove_notifier->s = s;
+remove_notifier->sd = sd;
+blk_add_remove_bs_notifier(sd->conf.blk, &remove_notifier->n);
+QTAILQ_INSERT_TAIL(&s->remove_notifiers, remove_notifier, next);
+
 if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
 return;
 }
@@ -787,6 +819,7 @@ static void virtio_scsi_hotunplug(HotplugHandler 
*hotplug_dev, DeviceState *dev,
 VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev);
 VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 SCSIDevice *sd = SCSI_DEVICE(dev);
+VirtIOSCSIBlkChangeNotifier *insert_notifier, *remove_notifier;
 
 if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
 virtio_scsi_push_event(s, sd,
@@ -797,6 +830,29 @@ static void virtio_scsi_hotunplug(HotplugHandler 
*hotplug_dev, DeviceState *dev,
 if (s->ctx) {
 blk_op_unblock_all(sd->conf.blk, s->blocker);
 }
+
+QTAILQ_FOREACH(insert_notifier, &s->insert_notifiers, next) {
+if (insert_notifier->sd == sd) {
+break;
+}
+}
+if (insert_notifier) {
+notifier_remove(&insert_notifier->n);
+QTAILQ_REMOVE(&s->insert_notifiers, insert_notifier, next);
+g_free(insert_notifier);
+}
+
+QTAILQ_FOREACH(remove_notifier, &s->remove_notifiers, next) {
+if (remove_notifier->sd == sd) {
+break;
+}
+}
+if (remove_notifier) {
+notifier_remove(&remove_notifier->n);
+QTAILQ_REMOVE(&s->remove_notifiers, remove_notifier, next);
+g_free(remove_notifier);
+}
+
 qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
 }
 
@@ -911,6 +967,9 @@ static void virtio_scsi_device_realize(DeviceState *dev, 
Error **errp)
 add_migration_state_change_notifier(&s->migration_state_notifier);
 
 error_setg(&s->blocker, "block device is in use by data plane");
+
+QTAILQ_INIT(&s->insert_notifiers);
+QTAILQ_INIT(&s->remove_notifiers);
 }
 
 static void virtio_scsi_instance_init(Object *obj)
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 088fe9f..0394eb2 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -76,6 +76,13 @@ typedef struct VirtIOSCSICommon {
 VirtQueue **cmd_vqs;
 } VirtIOSCSICommon;
 
+typedef struct VirtIOSCSIBlkChangeNotifier {
+Notifier n;
+struct VirtIOSCSI *s;
+SCSIDevice *sd;
+QTAILQ_ENTRY(VirtIOSCSIBlkChangeNotifier) next;
+} VirtIOSCSIBlkChangeNotifier;
+
 typedef struct VirtIOSCSI {
 VirtIOSCSICommon parent_obj;
 
@@ -86,6 +93,9 @@ typedef struct VirtIOSCSI {
 /* Fields for dataplane below */
 AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
 
+QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) insert_notifiers;
+QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) remove_notifiers;
+
 /* Vring is used instead of vq in dataplane code, becau