* [PATCH 1/4] net/virtio: fix dev_unplug_pending
@ 2019-11-14 14:16 Jens Freimann
2019-11-14 14:16 ` [PATCH 2/4] net/virtio: return early when failover primary alread added Jens Freimann
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Jens Freimann @ 2019-11-14 14:16 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, jasowang, dgilbert, mst
.dev_unplug_pending is set up by virtio-net code indepent of whether
failover=on was set for the device or not. This gives a wrong result when
we check for existing primary devices in migration code.
Fix this by actually calling dev_unplug_pending() instead of just
checking if the function pointer was set. When the feature was not
negotiated dev_unplug_pending() will always return false. This prevents
us from going into the wait-unplug state when there's no primary device
present.
Fixes: 9711cd0dfc3f ("net/virtio: add failover support")
Signed-off-by: Jens Freimann <jfreimann@redhat.com>
Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
hw/net/virtio-net.c | 3 +++
migration/savevm.c | 3 ++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 97a5113f7e..946039c0dc 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3124,6 +3124,9 @@ static bool primary_unplug_pending(void *opaque)
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
VirtIONet *n = VIRTIO_NET(vdev);
+ if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_STANDBY)) {
+ return false;
+ }
return n->primary_dev ? n->primary_dev->pending_deleted_event : false;
}
diff --git a/migration/savevm.c b/migration/savevm.c
index 966a9c3bdb..a71b930b91 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1119,7 +1119,8 @@ int qemu_savevm_nr_failover_devices(void)
int n = 0;
QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
- if (se->vmsd && se->vmsd->dev_unplug_pending) {
+ if (se->vmsd && se->vmsd->dev_unplug_pending &&
+ se->vmsd->dev_unplug_pending(se->opaque)) {
n++;
}
}
--
2.21.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/4] net/virtio: return early when failover primary alread added
2019-11-14 14:16 [PATCH 1/4] net/virtio: fix dev_unplug_pending Jens Freimann
@ 2019-11-14 14:16 ` Jens Freimann
2019-11-20 10:40 ` Michael S. Tsirkin
2019-11-14 14:16 ` [PATCH 3/4] net/virtio: avoid passing NULL to qdev_set_parent_bus Jens Freimann
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Jens Freimann @ 2019-11-14 14:16 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, jasowang, dgilbert, mst
Bail out when primary device was already added before.
This avoids printing a wrong warning message during reboot.
Fixes: 9711cd0dfc3f ("net/virtio: add failover support")
Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
hw/net/virtio-net.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 946039c0dc..ac4d19109e 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -759,6 +759,10 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
{
Error *err = NULL;
+ if (n->primary_dev) {
+ return;
+ }
+
n->primary_device_opts = qemu_opts_find(qemu_find_opts("device"),
n->primary_device_id);
if (n->primary_device_opts) {
--
2.21.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/4] net/virtio: avoid passing NULL to qdev_set_parent_bus
2019-11-14 14:16 [PATCH 1/4] net/virtio: fix dev_unplug_pending Jens Freimann
2019-11-14 14:16 ` [PATCH 2/4] net/virtio: return early when failover primary alread added Jens Freimann
@ 2019-11-14 14:16 ` Jens Freimann
2019-11-20 10:46 ` Michael S. Tsirkin
2019-11-14 14:16 ` [PATCH 4/4] net/virtio: return error when device_opts arg is NULL Jens Freimann
2019-11-20 10:39 ` [PATCH 1/4] net/virtio: fix dev_unplug_pending Michael S. Tsirkin
3 siblings, 1 reply; 10+ messages in thread
From: Jens Freimann @ 2019-11-14 14:16 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, jasowang, dgilbert, mst
Make sure no arguments for qdev_set_parent_bus are NULL.
This fixes CID 1407224.
Fixes: 9711cd0dfc3f ("net/virtio: add failover support")
Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
hw/net/virtio-net.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index ac4d19109e..81650d4dc0 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2809,8 +2809,16 @@ static bool failover_replug_primary(VirtIONet *n, Error **errp)
if (n->primary_device_opts) {
if (n->primary_dev) {
n->primary_bus = n->primary_dev->parent_bus;
+ if (n->primary_bus) {
+ qdev_set_parent_bus(n->primary_dev, n->primary_bus);
+ } else {
+ error_setg(errp, "virtio_net: couldn't set bus for primary");
+ goto out;
+ }
+ } else {
+ error_setg(errp, "virtio_net: couldn't find primary device");
+ goto out;
}
- qdev_set_parent_bus(n->primary_dev, n->primary_bus);
n->primary_should_be_hidden = false;
qemu_opt_set_bool(n->primary_device_opts,
"partially_hotplugged", true, errp);
@@ -2819,10 +2827,8 @@ static bool failover_replug_primary(VirtIONet *n, Error **errp)
hotplug_handler_pre_plug(hotplug_ctrl, n->primary_dev, errp);
hotplug_handler_plug(hotplug_ctrl, n->primary_dev, errp);
}
- if (!n->primary_dev) {
- error_setg(errp, "virtio_net: couldn't find primary device");
- }
}
+out:
return *errp != NULL;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] net/virtio: return error when device_opts arg is NULL
2019-11-14 14:16 [PATCH 1/4] net/virtio: fix dev_unplug_pending Jens Freimann
2019-11-14 14:16 ` [PATCH 2/4] net/virtio: return early when failover primary alread added Jens Freimann
2019-11-14 14:16 ` [PATCH 3/4] net/virtio: avoid passing NULL to qdev_set_parent_bus Jens Freimann
@ 2019-11-14 14:16 ` Jens Freimann
2019-11-20 10:47 ` Michael S. Tsirkin
2019-11-20 10:39 ` [PATCH 1/4] net/virtio: fix dev_unplug_pending Michael S. Tsirkin
3 siblings, 1 reply; 10+ messages in thread
From: Jens Freimann @ 2019-11-14 14:16 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, jasowang, dgilbert, mst
device_opts could be NULL. Make sure we don't pass it to
qemu_opts_to_dict. When we made sure it can't be NULL we can also remove
it from the if condition.
This fixes CID 1407222.
Fixes: 9711cd0dfc3f ("net/virtio: add failover support")
Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
hw/net/virtio-net.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 81650d4dc0..d53aa5796b 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2878,9 +2878,12 @@ static int virtio_net_primary_should_be_hidden(DeviceListener *listener,
QemuOpts *device_opts)
{
VirtIONet *n = container_of(listener, VirtIONet, primary_listener);
- bool match_found;
- bool hide;
+ bool match_found = false;
+ bool hide = false;
+ if (!device_opts) {
+ return -1;
+ }
n->primary_device_dict = qemu_opts_to_qdict(device_opts,
n->primary_device_dict);
if (n->primary_device_dict) {
@@ -2888,7 +2891,7 @@ static int virtio_net_primary_should_be_hidden(DeviceListener *listener,
n->standby_id = g_strdup(qdict_get_try_str(n->primary_device_dict,
"failover_pair_id"));
}
- if (device_opts && g_strcmp0(n->standby_id, n->netclient_name) == 0) {
+ if (g_strcmp0(n->standby_id, n->netclient_name) == 0) {
match_found = true;
} else {
match_found = false;
--
2.21.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] net/virtio: fix dev_unplug_pending
2019-11-14 14:16 [PATCH 1/4] net/virtio: fix dev_unplug_pending Jens Freimann
` (2 preceding siblings ...)
2019-11-14 14:16 ` [PATCH 4/4] net/virtio: return error when device_opts arg is NULL Jens Freimann
@ 2019-11-20 10:39 ` Michael S. Tsirkin
3 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2019-11-20 10:39 UTC (permalink / raw)
To: Jens Freimann; +Cc: peter.maydell, jasowang, qemu-devel, dgilbert
On Thu, Nov 14, 2019 at 03:16:10PM +0100, Jens Freimann wrote:
> .dev_unplug_pending is set up by virtio-net code indepent of whether
> failover=on was set for the device or not. This gives a wrong result when
> we check for existing primary devices in migration code.
>
> Fix this by actually calling dev_unplug_pending() instead of just
> checking if the function pointer was set. When the feature was not
> negotiated dev_unplug_pending() will always return false. This prevents
> us from going into the wait-unplug state when there's no primary device
> present.
>
> Fixes: 9711cd0dfc3f ("net/virtio: add failover support")
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
This isn't really a series, is it?
Just a bunch of independent patches...
anyway:
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/net/virtio-net.c | 3 +++
> migration/savevm.c | 3 ++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 97a5113f7e..946039c0dc 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3124,6 +3124,9 @@ static bool primary_unplug_pending(void *opaque)
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> VirtIONet *n = VIRTIO_NET(vdev);
>
> + if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_STANDBY)) {
> + return false;
> + }
> return n->primary_dev ? n->primary_dev->pending_deleted_event : false;
> }
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 966a9c3bdb..a71b930b91 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1119,7 +1119,8 @@ int qemu_savevm_nr_failover_devices(void)
> int n = 0;
>
> QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> - if (se->vmsd && se->vmsd->dev_unplug_pending) {
> + if (se->vmsd && se->vmsd->dev_unplug_pending &&
> + se->vmsd->dev_unplug_pending(se->opaque)) {
> n++;
> }
> }
> --
> 2.21.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] net/virtio: return early when failover primary alread added
2019-11-14 14:16 ` [PATCH 2/4] net/virtio: return early when failover primary alread added Jens Freimann
@ 2019-11-20 10:40 ` Michael S. Tsirkin
0 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2019-11-20 10:40 UTC (permalink / raw)
To: Jens Freimann; +Cc: peter.maydell, jasowang, qemu-devel, dgilbert
On Thu, Nov 14, 2019 at 03:16:11PM +0100, Jens Freimann wrote:
> Bail out when primary device was already added before.
> This avoids printing a wrong warning message during reboot.
>
> Fixes: 9711cd0dfc3f ("net/virtio: add failover support")
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/net/virtio-net.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 946039c0dc..ac4d19109e 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -759,6 +759,10 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
> {
> Error *err = NULL;
>
> + if (n->primary_dev) {
> + return;
> + }
> +
> n->primary_device_opts = qemu_opts_find(qemu_find_opts("device"),
> n->primary_device_id);
> if (n->primary_device_opts) {
> --
> 2.21.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] net/virtio: avoid passing NULL to qdev_set_parent_bus
2019-11-14 14:16 ` [PATCH 3/4] net/virtio: avoid passing NULL to qdev_set_parent_bus Jens Freimann
@ 2019-11-20 10:46 ` Michael S. Tsirkin
2019-11-20 12:03 ` Jens Freimann
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2019-11-20 10:46 UTC (permalink / raw)
To: Jens Freimann; +Cc: peter.maydell, jasowang, qemu-devel, dgilbert
On Thu, Nov 14, 2019 at 03:16:12PM +0100, Jens Freimann wrote:
> Make sure no arguments for qdev_set_parent_bus are NULL.
> This fixes CID 1407224.
>
> Fixes: 9711cd0dfc3f ("net/virtio: add failover support")
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
> hw/net/virtio-net.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index ac4d19109e..81650d4dc0 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2809,8 +2809,16 @@ static bool failover_replug_primary(VirtIONet *n, Error **errp)
> if (n->primary_device_opts) {
> if (n->primary_dev) {
> n->primary_bus = n->primary_dev->parent_bus;
> + if (n->primary_bus) {
> + qdev_set_parent_bus(n->primary_dev, n->primary_bus);
> + } else {
> + error_setg(errp, "virtio_net: couldn't set bus for primary");
> + goto out;
> + }
> + } else {
> + error_setg(errp, "virtio_net: couldn't find primary device");
> + goto out;
> }
A bit less messy:
if (!n->primary_dev) {
error_setg(errp, "virtio_net: couldn't find primary device");
goto err;
}
n->primary_bus = n->primary_dev->parent_bus;
if (!n->primary_bus) {
error_setg(errp, "virtio_net: couldn't find primary device");
goto err;
}
qdev_set_parent_bus(n->primary_dev, n->primary_bus);
err:
return false;
also is it valid for primary_device_opts to not be present at all?
Maybe we should check that too.
> - qdev_set_parent_bus(n->primary_dev, n->primary_bus);
> n->primary_should_be_hidden = false;
> qemu_opt_set_bool(n->primary_device_opts,
> "partially_hotplugged", true, errp);
> @@ -2819,10 +2827,8 @@ static bool failover_replug_primary(VirtIONet *n, Error **errp)
> hotplug_handler_pre_plug(hotplug_ctrl, n->primary_dev, errp);
> hotplug_handler_plug(hotplug_ctrl, n->primary_dev, errp);
> }
> - if (!n->primary_dev) {
> - error_setg(errp, "virtio_net: couldn't find primary device");
> - }
> }
> +out:
> return *errp != NULL;
> }
I'd handle errors separately from good path.
BTW I don't understand something here:
*errp != NULL is true on error, no?
Why are we returning success?
Confused.
Just return true would be cleaner imho.
>
> --
> 2.21.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] net/virtio: return error when device_opts arg is NULL
2019-11-14 14:16 ` [PATCH 4/4] net/virtio: return error when device_opts arg is NULL Jens Freimann
@ 2019-11-20 10:47 ` Michael S. Tsirkin
0 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2019-11-20 10:47 UTC (permalink / raw)
To: Jens Freimann; +Cc: peter.maydell, jasowang, qemu-devel, dgilbert
On Thu, Nov 14, 2019 at 03:16:13PM +0100, Jens Freimann wrote:
> device_opts could be NULL. Make sure we don't pass it to
> qemu_opts_to_dict. When we made sure it can't be NULL we can also remove
> it from the if condition.
> This fixes CID 1407222.
>
> Fixes: 9711cd0dfc3f ("net/virtio: add failover support")
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/net/virtio-net.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 81650d4dc0..d53aa5796b 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2878,9 +2878,12 @@ static int virtio_net_primary_should_be_hidden(DeviceListener *listener,
> QemuOpts *device_opts)
> {
> VirtIONet *n = container_of(listener, VirtIONet, primary_listener);
> - bool match_found;
> - bool hide;
> + bool match_found = false;
> + bool hide = false;
>
> + if (!device_opts) {
> + return -1;
> + }
> n->primary_device_dict = qemu_opts_to_qdict(device_opts,
> n->primary_device_dict);
> if (n->primary_device_dict) {
> @@ -2888,7 +2891,7 @@ static int virtio_net_primary_should_be_hidden(DeviceListener *listener,
> n->standby_id = g_strdup(qdict_get_try_str(n->primary_device_dict,
> "failover_pair_id"));
> }
> - if (device_opts && g_strcmp0(n->standby_id, n->netclient_name) == 0) {
> + if (g_strcmp0(n->standby_id, n->netclient_name) == 0) {
> match_found = true;
> } else {
> match_found = false;
> --
> 2.21.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] net/virtio: avoid passing NULL to qdev_set_parent_bus
2019-11-20 10:46 ` Michael S. Tsirkin
@ 2019-11-20 12:03 ` Jens Freimann
2019-11-20 12:05 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Jens Freimann @ 2019-11-20 12:03 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: peter.maydell, jasowang, qemu-devel, dgilbert
On Wed, Nov 20, 2019 at 05:46:36AM -0500, Michael S. Tsirkin wrote:
>On Thu, Nov 14, 2019 at 03:16:12PM +0100, Jens Freimann wrote:
>> Make sure no arguments for qdev_set_parent_bus are NULL.
>> This fixes CID 1407224.
>>
>> Fixes: 9711cd0dfc3f ("net/virtio: add failover support")
>> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
>> ---
>> hw/net/virtio-net.c | 14 ++++++++++----
>> 1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index ac4d19109e..81650d4dc0 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -2809,8 +2809,16 @@ static bool failover_replug_primary(VirtIONet *n, Error **errp)
>> if (n->primary_device_opts) {
>> if (n->primary_dev) {
>> n->primary_bus = n->primary_dev->parent_bus;
>> + if (n->primary_bus) {
>> + qdev_set_parent_bus(n->primary_dev, n->primary_bus);
>> + } else {
>> + error_setg(errp, "virtio_net: couldn't set bus for primary");
>> + goto out;
>> + }
>> + } else {
>> + error_setg(errp, "virtio_net: couldn't find primary device");
>> + goto out;
>> }
>
>
>A bit less messy:
>
>if (!n->primary_dev) {
> error_setg(errp, "virtio_net: couldn't find primary device");
> goto err;
>}
>
>n->primary_bus = n->primary_dev->parent_bus;
>if (!n->primary_bus) {
> error_setg(errp, "virtio_net: couldn't find primary device");
> goto err;
>}
>
>qdev_set_parent_bus(n->primary_dev, n->primary_bus);
>
>err:
> return false;
>
>also is it valid for primary_device_opts to not be present at all?
>Maybe we should check that too.
>
>
>
>> - qdev_set_parent_bus(n->primary_dev, n->primary_bus);
>> n->primary_should_be_hidden = false;
>> qemu_opt_set_bool(n->primary_device_opts,
>> "partially_hotplugged", true, errp);
>> @@ -2819,10 +2827,8 @@ static bool failover_replug_primary(VirtIONet *n, Error **errp)
>> hotplug_handler_pre_plug(hotplug_ctrl, n->primary_dev, errp);
>> hotplug_handler_plug(hotplug_ctrl, n->primary_dev, errp);
>> }
>> - if (!n->primary_dev) {
>> - error_setg(errp, "virtio_net: couldn't find primary device");
>> - }
>> }
>> +out:
>> return *errp != NULL;
>> }
>
>I'd handle errors separately from good path.
>BTW I don't understand something here:
>*errp != NULL is true on error, no?
>Why are we returning success?
>Confused.
>Just return true would be cleaner imho.
I'll fix this and sent a new version as a single patch. As you said
this is not really a series, just individual patches.
Thanks!
regards
Jens
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] net/virtio: avoid passing NULL to qdev_set_parent_bus
2019-11-20 12:03 ` Jens Freimann
@ 2019-11-20 12:05 ` Michael S. Tsirkin
0 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2019-11-20 12:05 UTC (permalink / raw)
To: Jens Freimann; +Cc: peter.maydell, jasowang, qemu-devel, dgilbert
On Wed, Nov 20, 2019 at 01:03:57PM +0100, Jens Freimann wrote:
> On Wed, Nov 20, 2019 at 05:46:36AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Nov 14, 2019 at 03:16:12PM +0100, Jens Freimann wrote:
> > > Make sure no arguments for qdev_set_parent_bus are NULL.
> > > This fixes CID 1407224.
> > >
> > > Fixes: 9711cd0dfc3f ("net/virtio: add failover support")
> > > Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> > > ---
> > > hw/net/virtio-net.c | 14 ++++++++++----
> > > 1 file changed, 10 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index ac4d19109e..81650d4dc0 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -2809,8 +2809,16 @@ static bool failover_replug_primary(VirtIONet *n, Error **errp)
> > > if (n->primary_device_opts) {
> > > if (n->primary_dev) {
> > > n->primary_bus = n->primary_dev->parent_bus;
> > > + if (n->primary_bus) {
> > > + qdev_set_parent_bus(n->primary_dev, n->primary_bus);
> > > + } else {
> > > + error_setg(errp, "virtio_net: couldn't set bus for primary");
> > > + goto out;
> > > + }
> > > + } else {
> > > + error_setg(errp, "virtio_net: couldn't find primary device");
> > > + goto out;
> > > }
> >
> >
> > A bit less messy:
> >
> > if (!n->primary_dev) {
> > error_setg(errp, "virtio_net: couldn't find primary device");
> > goto err;
> > }
> >
> > n->primary_bus = n->primary_dev->parent_bus;
> > if (!n->primary_bus) {
> > error_setg(errp, "virtio_net: couldn't find primary device");
> > goto err;
> > }
> >
> > qdev_set_parent_bus(n->primary_dev, n->primary_bus);
> >
> > err:
> > return false;
> >
> > also is it valid for primary_device_opts to not be present at all?
> > Maybe we should check that too.
> >
> >
> >
> > > - qdev_set_parent_bus(n->primary_dev, n->primary_bus);
> > > n->primary_should_be_hidden = false;
> > > qemu_opt_set_bool(n->primary_device_opts,
> > > "partially_hotplugged", true, errp);
> > > @@ -2819,10 +2827,8 @@ static bool failover_replug_primary(VirtIONet *n, Error **errp)
> > > hotplug_handler_pre_plug(hotplug_ctrl, n->primary_dev, errp);
> > > hotplug_handler_plug(hotplug_ctrl, n->primary_dev, errp);
> > > }
> > > - if (!n->primary_dev) {
> > > - error_setg(errp, "virtio_net: couldn't find primary device");
> > > - }
> > > }
> > > +out:
> > > return *errp != NULL;
> > > }
> >
> > I'd handle errors separately from good path.
> > BTW I don't understand something here:
> > *errp != NULL is true on error, no?
> > Why are we returning success?
> > Confused.
> > Just return true would be cleaner imho.
>
> I'll fix this and sent a new version as a single patch. As you said
> this is not really a series, just individual patches.
>
> Thanks!
>
> regards
> Jens
I'd just repost them all then. The series was also threaded weirdly
(e.g. there's no cover letter instead patches 2 and on link to patch 1).
--
MST
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-11-20 12:08 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-14 14:16 [PATCH 1/4] net/virtio: fix dev_unplug_pending Jens Freimann
2019-11-14 14:16 ` [PATCH 2/4] net/virtio: return early when failover primary alread added Jens Freimann
2019-11-20 10:40 ` Michael S. Tsirkin
2019-11-14 14:16 ` [PATCH 3/4] net/virtio: avoid passing NULL to qdev_set_parent_bus Jens Freimann
2019-11-20 10:46 ` Michael S. Tsirkin
2019-11-20 12:03 ` Jens Freimann
2019-11-20 12:05 ` Michael S. Tsirkin
2019-11-14 14:16 ` [PATCH 4/4] net/virtio: return error when device_opts arg is NULL Jens Freimann
2019-11-20 10:47 ` Michael S. Tsirkin
2019-11-20 10:39 ` [PATCH 1/4] net/virtio: fix dev_unplug_pending Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).