From patchwork Mon Mar 25 13:40:44 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Maydell X-Patchwork-Id: 15596 Return-Path: X-Original-To: patchwork@peony.canonical.com Delivered-To: patchwork@peony.canonical.com Received: from fiordland.canonical.com (fiordland.canonical.com [91.189.94.145]) by peony.canonical.com (Postfix) with ESMTP id 0557323DEE for ; Mon, 25 Mar 2013 13:40:51 +0000 (UTC) Received: from mail-la0-f45.google.com (mail-la0-f45.google.com [209.85.215.45]) by fiordland.canonical.com (Postfix) with ESMTP id A1D52A193B5 for ; Mon, 25 Mar 2013 13:40:50 +0000 (UTC) Received: by mail-la0-f45.google.com with SMTP id er20so11409029lab.18 for ; Mon, 25 Mar 2013 06:40:50 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:x-forwarded-to:x-forwarded-for:delivered-to:x-received :received-spf:from:to:cc:subject:date:message-id:x-mailer :x-gm-message-state; bh=GLHlcy/sFDk3/4emfTaL1K6X8NEqjx+kwRfTus+QkWM=; b=BXGr9Msvw+hdYvXTkrxtoMlUSNMJ99xs6fcxkaFr6O2hwcNCGjtpCFhTD2GVruxHNM hgkcsTmg3TV7ar4VMTAEPXhYxIV0lD3mNbaR2v4aMd3247CPX/pcXJ/IYOYcSq7JWgGy nLhpYghAhk0pwGs8MuCjxnvdQ0I5IylifVeKAKDdUqO2lPVcCNqpVHyR3SJcdSYDte0H DXJb3z57uucRiKmgd0hh6ze8dX5/GUboZ1WeoZ7EWmjcEPCek69RUrQx9D+N58HJVxwk u0E4goyVVyhbVivegPjEShaQ9CF9goDw6GB4XzVAWQMRoB61pCo5YpiZDBafKZzH5v3a jhPA== X-Received: by 10.112.9.10 with SMTP id v10mr6206094lba.47.1364218850115; Mon, 25 Mar 2013 06:40:50 -0700 (PDT) X-Forwarded-To: linaro-patchwork@canonical.com X-Forwarded-For: patch@linaro.org linaro-patchwork@canonical.com Delivered-To: patches@linaro.org Received: by 10.112.147.5 with SMTP id tg5csp48441lbb; Mon, 25 Mar 2013 06:40:49 -0700 (PDT) X-Received: by 10.194.176.34 with SMTP id cf2mr6308665wjc.59.1364218848963; Mon, 25 Mar 2013 06:40:48 -0700 (PDT) Received: from mnementh.archaic.org.uk (1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.d.1.0.0.b.8.0.1.0.0.2.ip6.arpa. [2001:8b0:1d0::1]) by mx.google.com with ESMTPS id t6si7044122wif.110.2013.03.25.06.40.48 (version=TLSv1 cipher=RC4-SHA bits=128/128); Mon, 25 Mar 2013 06:40:48 -0700 (PDT) Received-SPF: neutral (google.com: 2001:8b0:1d0::1 is neither permitted nor denied by best guess record for domain of pm215@archaic.org.uk) client-ip=2001:8b0:1d0::1; Authentication-Results: mx.google.com; spf=neutral (google.com: 2001:8b0:1d0::1 is neither permitted nor denied by best guess record for domain of pm215@archaic.org.uk) smtp.mail=pm215@archaic.org.uk Received: from pm215 by mnementh.archaic.org.uk with local (Exim 4.72) (envelope-from ) id 1UK7dk-0001xY-1P; Mon, 25 Mar 2013 13:40:44 +0000 From: Peter Maydell To: qemu-devel@nongnu.org Cc: patches@linaro.org, =?UTF-8?q?Andreas=20F=C3=A4rber?= , Paolo Bonzini , Igor Mitsyanko , Anthony Liguori Subject: [PATCH v4] hw/qdev-properties.c: Improve diagnostic for setting property after realize Date: Mon, 25 Mar 2013 13:40:44 +0000 Message-Id: <1364218844-7509-1-git-send-email-peter.maydell@linaro.org> X-Mailer: git-send-email 1.7.2.5 X-Gm-Message-State: ALoCoQnjDx0yr+8sU2+wZ9fV86s+dqnz7Yu6Xx2SVmCSKX0P/DuhcJd7GYCk4/+6CVpO7xuZ/BS2 Now we have error_setg() we can improve the error message emitted if you attempt to set a property of a device after the device is realized (the previous message was "permission denied" which was not very informative). Signed-off-by: Peter Maydell Reviewed-by: Igor Mitsyanko --- Another attempt at updating this patch (v3 October, v2 July). Changes v3->v4: * rebased; some functions have moved file, so the 'set the error' function can't be file-local. Renamed it to something more suitable for a public function and added a doc comment. * added set_taddr to the set of errors changed (and also checked we now have the complete set) Not changed: Igor suggested using "after it was initialized" rather than "after it was realized"; I think the original text is better, because (a) we're moving towards having all devices implement realize rather than init and (b) it's mostly an error message for QEMU developers rather than end-users, so the technical terminology is not inappropriate. hw/qdev-addr.c | 2 +- hw/qdev-properties-system.c | 4 ++-- hw/qdev-properties.c | 40 +++++++++++++++++++++++++++------------- hw/qdev-properties.h | 12 ++++++++++++ hw/qdev.c | 2 +- 5 files changed, 43 insertions(+), 17 deletions(-) diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c index 2398b4a..80a38bb 100644 --- a/hw/qdev-addr.c +++ b/hw/qdev-addr.c @@ -42,7 +42,7 @@ static void set_taddr(Object *obj, Visitor *v, void *opaque, int64_t value; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c index 8795144..28813d3 100644 --- a/hw/qdev-properties-system.c +++ b/hw/qdev-properties-system.c @@ -43,7 +43,7 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop, int ret; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -287,7 +287,7 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque, NetClientState *hubport; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c index 247ca6c..168c466 100644 --- a/hw/qdev-properties.c +++ b/hw/qdev-properties.c @@ -7,6 +7,20 @@ #include "qapi/visitor.h" #include "char/char.h" +void qdev_prop_set_after_realize(DeviceState *dev, const char *name, + Error **errp) +{ + if (dev->id) { + error_setg(errp, "Attempt to set property '%s' on device '%s' " + "(type '%s') after it was realized", name, dev->id, + object_get_typename(OBJECT(dev))); + } else { + error_setg(errp, "Attempt to set property '%s' on anonymous device " + "(type '%s') after it was realized", name, + object_get_typename(OBJECT(dev))); + } +} + void *qdev_get_prop_ptr(DeviceState *dev, Property *prop) { void *ptr = dev; @@ -33,7 +47,7 @@ static void set_enum(Object *obj, Visitor *v, void *opaque, int *ptr = qdev_get_prop_ptr(dev, prop); if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -86,7 +100,7 @@ static void set_bit(Object *obj, Visitor *v, void *opaque, bool value; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -126,7 +140,7 @@ static void set_uint8(Object *obj, Visitor *v, void *opaque, uint8_t *ptr = qdev_get_prop_ptr(dev, prop); if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -193,7 +207,7 @@ static void set_uint16(Object *obj, Visitor *v, void *opaque, uint16_t *ptr = qdev_get_prop_ptr(dev, prop); if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -226,7 +240,7 @@ static void set_uint32(Object *obj, Visitor *v, void *opaque, uint32_t *ptr = qdev_get_prop_ptr(dev, prop); if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -251,7 +265,7 @@ static void set_int32(Object *obj, Visitor *v, void *opaque, int32_t *ptr = qdev_get_prop_ptr(dev, prop); if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -324,7 +338,7 @@ static void set_uint64(Object *obj, Visitor *v, void *opaque, uint64_t *ptr = qdev_get_prop_ptr(dev, prop); if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -414,7 +428,7 @@ static void set_string(Object *obj, Visitor *v, void *opaque, char *str; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -478,7 +492,7 @@ static void set_mac(Object *obj, Visitor *v, void *opaque, char *str, *p; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -570,7 +584,7 @@ static void set_pci_devfn(Object *obj, Visitor *v, void *opaque, char *str; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -641,7 +655,7 @@ static void set_blocksize(Object *obj, Visitor *v, void *opaque, const int64_t max = 32768; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -709,7 +723,7 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, void *opaque, unsigned int slot = 0, func = 0; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } @@ -824,7 +838,7 @@ static void set_prop_arraylen(Object *obj, Visitor *v, void *opaque, int i; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; } if (*alenptr) { diff --git a/hw/qdev-properties.h b/hw/qdev-properties.h index c9bb246..a379339 100644 --- a/hw/qdev-properties.h +++ b/hw/qdev-properties.h @@ -167,4 +167,16 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev, */ void qdev_property_add_static(DeviceState *dev, Property *prop, Error **errp); +/** + * @qdev_prop_set_after_realize: + * @dev: device + * @name: name of property + * @errp: indirect pointer to Error to be set + * Set the Error object to report that an attempt was made to set a property + * on a device after it has already been realized. This is a utility function + * which allows property-setter functions to easily report the error in + * a friendly format identifying both the device and the property. + */ +void qdev_prop_set_after_realize(DeviceState *dev, const char *name, + Error **errp); #endif diff --git a/hw/qdev.c b/hw/qdev.c index 909c405..6b1947e 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -562,7 +562,7 @@ static void qdev_set_legacy_property(Object *obj, Visitor *v, void *opaque, int ret; if (dev->realized) { - error_set(errp, QERR_PERMISSION_DENIED); + qdev_prop_set_after_realize(dev, name, errp); return; }