diff mbox series

[v6,8/9] chardev/char.c: Check for duplicate id before creating chardev

Message ID b6d4b5712afc095f3d315818723809a20a2de21e.1596184200.git.lukasstraub2@web.de
State New
Headers show
Series [v6,1/9] Introduce yank feature | expand

Commit Message

Lukas Straub July 31, 2020, 9:27 a.m. UTC 
yank_register_instance (called when creating the new chardev object)
aborts if the instance already exists. So check for duplicate id before
creating the new chardev to prevent this.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 chardev/char.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

--
2.20.1

Comments

Daniel P. Berrangé July 31, 2020, 9:37 a.m. UTC | #1 
On Fri, Jul 31, 2020 at 11:27:04AM +0200, Lukas Straub wrote:
> yank_register_instance (called when creating the new chardev object)
> aborts if the instance already exists. So check for duplicate id before
> creating the new chardev to prevent this.

I feel the right answer here is for yank_register_instance to not
use abort().

Instead have it take a 'Error **errp' and report the error normally.
The caller can then propagate the errors in the same way it does
for the duplicate ID errors. If a particular caller can't handle
errors gracefully, then it can pass "&error_abort" to the
yank_register_instance() to get the same abort semantics as now.

Regards,
Daniel
Lukas Straub July 31, 2020, 1:29 p.m. UTC | #2 
On Fri, 31 Jul 2020 10:37:34 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Fri, Jul 31, 2020 at 11:27:04AM +0200, Lukas Straub wrote:
> > yank_register_instance (called when creating the new chardev object)
> > aborts if the instance already exists. So check for duplicate id before
> > creating the new chardev to prevent this.  
> 
> I feel the right answer here is for yank_register_instance to not
> use abort().
> 
> Instead have it take a 'Error **errp' and report the error normally.
> The caller can then propagate the errors in the same way it does
> for the duplicate ID errors. If a particular caller can't handle
> errors gracefully, then it can pass "&error_abort" to the
> yank_register_instance() to get the same abort semantics as now.
> 
> Regards,
> Daniel

Yes, makes sense for yank_register_instance not to kill qemu. Although (with this fix in) it always indicates a bug due to a leaked yank instance.

Regards,
Lukas Straub
diff mbox series

Patch

diff --git a/chardev/char.c b/chardev/char.c
index 77e7ec814f..ce041dface 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -951,6 +951,11 @@  static Chardev *chardev_new(const char *id, const char *typename,

     assert(g_str_has_prefix(typename, "chardev-"));

+    if (id && object_resolve_path_component(get_chardevs_root(), id)) {
+        error_setg(errp, "Chardev '%s' already exists", id);
+        return NULL;
+    }
+
     obj = object_new(typename);
     chr = CHARDEV(obj);
     chr->label = g_strdup(id);
@@ -969,11 +974,7 @@  static Chardev *chardev_new(const char *id, const char *typename,
     }

     if (id) {
-        object_property_try_add_child(get_chardevs_root(), id, obj,
-                                      &local_err);
-        if (local_err) {
-            goto end;
-        }
+        object_property_add_child(get_chardevs_root(), id, obj);
         object_unref(obj);
     }