From 1bc133365e39feb2a37747d47de281927ce1b853 Mon Sep 17 00:00:00 2001
From: Markus Armbruster <armbru@redhat.com>
Date: Mon, 22 Jun 2020 11:42:21 +0200
Subject: qdev: Improve netdev property override error a bit

qdev_prop_set_netdev() fails when the property already has a non-null
value.  Seems to go back to commit 30c367ed44
"qdev-properties-system.c: Allow vlan or netdev for -device, not
both", v1.7.0.  Board code doesn't expect failure, and crashes:

    $ qemu-system-x86_64 --nodefaults -nic user -netdev user,id=nic0 -global e1000.netdev=nic0
    Unexpected error in error_set_from_qdev_prop_error() at /work/armbru/qemu/hw/core/qdev-properties.c:1101:
    qemu-system-x86_64: Property 'e1000.netdev' doesn't take value '__org.qemu.nic0
    '
    Aborted (core dumped)

-device and device_add handle the failure:

    $ qemu-system-x86_64 -nodefaults -netdev user,id=net0 -netdev user,id=net1 -device e1000,netdev=net0,netdev=net1
    qemu-system-x86_64: -device e1000,netdev=net0,netdev=net1: Property 'e1000.netdev' doesn't take value 'net1'
    $ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -netdev user,id=net0 -netdev user,id=net1 -global e1000.netdev=net0
    QEMU 5.0.50 monitor - type 'help' for more information
    (qemu) qemu-system-x86_64: warning: netdev net0 has no peer
    qemu-system-x86_64: warning: netdev net1 has no peer
    device_add e1000,netdev=net1
    Error: Property 'e1000.netdev' doesn't take value 'net1'

Perhaps netdev property override could be made to work.  Perhaps it
should.  I'm not the right guy to figure this out.  What I can do is
improve the error message a bit:

    (qemu) device_add e1000,netdev=net1
    Error: -global e1000.netdev=... conflicts with netdev=net1

Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20200622094227.1271650-11-armbru@redhat.com>
---
 hw/core/qdev-properties-system.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

(limited to 'hw/core/qdev-properties-system.c')

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 9aa80495ee..3f84309b7e 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -25,6 +25,28 @@
 #include "sysemu/iothread.h"
 #include "sysemu/tpm_backend.h"
 
+static bool check_prop_still_unset(DeviceState *dev, const char *name,
+                                   const void *old_val, const char *new_val,
+                                   Error **errp)
+{
+    const GlobalProperty *prop = qdev_find_global_prop(dev, name);
+
+    if (!old_val) {
+        return true;
+    }
+
+    if (prop) {
+        error_setg(errp, "-global %s.%s=... conflicts with %s=%s",
+                   prop->driver, prop->property, name, new_val);
+    } else {
+        /* Error message is vague, but a better one would be hard */
+        error_setg(errp, "%s=%s conflicts, and override is not implemented",
+                   name, new_val);
+    }
+    return false;
+}
+
+
 /* --- drive --- */
 
 static void get_drive(Object *obj, Visitor *v, const char *name, void *opaque,
@@ -299,14 +321,16 @@ static void set_netdev(Object *obj, Visitor *v, const char *name,
     }
 
     for (i = 0; i < queues; i++) {
-
         if (peers[i]->peer) {
             err = -EEXIST;
             goto out;
         }
 
-        if (ncs[i]) {
-            err = -EINVAL;
+        /*
+         * TODO Should this really be an error?  If no, the old value
+         * needs to be released before we store the new one.
+         */
+        if (!check_prop_still_unset(dev, name, ncs[i], str, errp)) {
             goto out;
         }
 
-- 
cgit 1.4.1