Re: [Qemu-devel] [PATCH v2] Revised: Add object_property_get_child().
On 02/19/2012 06:04 PM, Alexander Barabash wrote: Add object_property_get_child(). Please disregard this patch. object_property_get_link() works for this purpose.
Re: [Qemu-devel] [PATCH v2] Revised: Add object_property_get_child().
On 02/19/2012 07:14 PM, Andreas Färber wrote:
Am 19.02.2012 17:04, schrieb Alexander Barabash:
...
Signed-off-by: Alexander Barabashalexander_barab...@mentor.com
Please use git-send-email to submit your patches: The commit message is
unnecessarily indented and the first line is duplicated. Instead of
Revised: (which v2 already indicates) the subject should mention the
topic, here qom: .
http://wiki.qemu.org/Contribute/SubmitAPatch
Thanks for the advice.
Your patch is doing too many things at once. Please split it up into a
series of easily digestable and bisectable patches, e.g.:
* ..._PREFIX/SUFFIX introduction and use in _add_child/link,
* ..._is_child/_is_link introduction and use in
_property_del_child/_get_canonical_path/_resolve_partial_path,
* link_type_to_type() and use in _set_link_property,
* REPORT_OBJECT_ERROR(),
* object_property_get_child().
OK
+/* Go from LINK_PROPERTY_TYPE_PREFIX FOO LINK_PROPERTY_TYPE_SUFFIX to
FOO. */
+static gchar *link_type_to_type(const gchar *type)
+{
+return g_strndup(type[sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1],
+ strlen(type)
+ - (sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1)
+ - (sizeof(LINK_PROPERTY_TYPE_SUFFIX) - 1));
Any reason not to use strlen()? I don't think this is a hot path, and
repeated sizeof() - 1 does not read so nice. The alternative would be to
hardcode the offsets.
I replaced 5 (i.e. the length link) with
sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1.
If we assume that strlen(link) is always optimized away,
then certainly using strlen is equivalent and looks better.
Andreas
Alex
[Qemu-devel] [PATCH v2] Revised: Add object_property_get_child().
Add object_property_get_child().
Adding a direct accessor to a child property.
In the existing implementation, object_property_get() must be used,
with with a visitor, implementing the 'type_str' callback,
receiving the child's canonical path.
In the new implementation, the child is returned directly.
For link properties, object_property_get_link() is used
to resolve the link.
Also, in the new implementation, object_property_get_child() is used
as a subroutine of object_resolve_abs_path(). This changes the way
object_resolve_abs_path() operates, moving away from directly peeking
the property's 'opaque' field to using object_property_get_link().
Thus, in the mew implementation link properties are resolved in the
same way,
as they are when an absolute path is resolved.
Errors relevant to the operation, QERR_OBJECT_PROPERTY_NOT_FOUND
and QERR_OBJECT_PROPERTY_INVALID_TYPE were added.
Also, in the new implementation, some common sense refactoring was done
in the file 'qom/object.c' in the code extracting child and link
properties.
Signed-off-by: Alexander Barabash alexander_barab...@mentor.com
diff --git a/include/qemu/object.h b/include/qemu/object.h
index ba2409d..001521d 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -711,6 +711,21 @@ int64_t object_property_get_int(Object *obj, const
char *name,
struct Error **errp);
/**
+ * object_property_get_child:
+ * @obj: the object
+ * @name: the name of the property
+ * @errp: returns an error if this function fails
+ *
+ * Returns: if this a child property, the value of the property, or NULL if
+ * an error occurs (including when the property value is not a child
property).
+ *
+ * Result's reference count does not change.
+ * Therefore, he caller is responsible for referencing the result.
+ */
+Object *object_property_get_child(Object *obj, const char *name,
+ struct Error **errp);
+
+/**
* object_property_set:
* @obj: the object
* @v: the visitor that will be used to write the property value.
This should
diff --git a/qerror.h b/qerror.h
index e26c635..45e4468 100644
--- a/qerror.h
+++ b/qerror.h
@@ -178,6 +178,12 @@ QError *qobject_to_qerror(const QObject *obj);
#define QERR_NOT_SUPPORTED \
{ 'class': 'NotSupported', 'data': {} }
+#define QERR_OBJECT_PROPERTY_NOT_FOUND \
+{ 'class': 'ObjectPropertyNotFound', 'data': { 'object': %s,
'property': %s } }
+
+#define QERR_OBJECT_PROPERTY_INVALID_TYPE \
+{ 'class': 'ObjectPropertyInvalidType', 'data': { 'object': %s,
'property': %s, 'expected_type': %s } }
+
#define QERR_OPEN_FILE_FAILED \
{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }
diff --git a/qom/object.c b/qom/object.c
index 2de6eaf..ddd19e1 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -297,12 +297,64 @@ static void object_property_del_all(Object *obj)
}
}
+/*
+ * To ensure correct format checking,
+ * this function should be used only via REPORT_OBJECT_ERROR() macro.
+ *
+ * The first argument after 'obj' should be of type 'const char *'.
+ * It is ignored, and replaced by the canonical path of 'obj'.
+ */
+static void report_object_error(Error **errp, const char *fmt, Object
*obj, ...)
+GCC_FMT_ATTR(2, 4);
+static void report_object_error(Error **errp, const char *fmt, Object
*obj, ...)
+{
+gchar *path;
+va_list ap;
+
+if (errp != NULL) {
+path = object_get_canonical_path(obj);
+va_start(ap, obj);
+va_arg(ap, const char *); /* Ignore the dummy string. */
+error_set(errp, fmt, path, ap);
+va_end(ap);
+g_free(path);
+}
+}
+#define REPORT_OBJECT_ERROR(errp, fmt, obj, ...) \
+do { \
+report_object_error(errp, fmt, obj, , ## __VA_ARGS__); \
+} while (0)
+
+#define CHILD_PROPERTY_TYPE_PREFIX child
+#define CHILD_PROPERTY_TYPE_SUFFIX
+#define LINK_PROPERTY_TYPE_PREFIX link
+#define LINK_PROPERTY_TYPE_SUFFIX
+
+static bool object_property_is_child(ObjectProperty *prop)
+{
+return (strstart(prop-type, CHILD_PROPERTY_TYPE_PREFIX, NULL) != 0);
+}
+
+static bool object_property_is_link(ObjectProperty *prop)
+{
+return (strstart(prop-type, LINK_PROPERTY_TYPE_PREFIX, NULL) != 0);
+}
+
+/* Go from LINK_PROPERTY_TYPE_PREFIX FOO LINK_PROPERTY_TYPE_SUFFIX to
FOO. */
+static gchar *link_type_to_type(const gchar *type)
+{
+return g_strndup(type[sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1],
+ strlen(type)
+ - (sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1)
+ - (sizeof(LINK_PROPERTY_TYPE_SUFFIX) - 1));
+}
+
static void object_property_del_child(Object *obj, Object *child,
Error **errp)
{
ObjectProperty *prop;
QTAILQ_FOREACH(prop, obj-properties, node) {
-if (!strstart(prop-type, child, NULL)) {
+if
Re: [Qemu-devel] [PATCH v2] Revised: Add object_property_get_child().
Am 19.02.2012 17:04, schrieb Alexander Barabash:
Add object_property_get_child().
Adding a direct accessor to a child property.
In the existing implementation, object_property_get() must be used,
with with a visitor, implementing the 'type_str' callback,
receiving the child's canonical path.
In the new implementation, the child is returned directly.
For link properties, object_property_get_link() is used
to resolve the link.
Also, in the new implementation, object_property_get_child() is used
as a subroutine of object_resolve_abs_path(). This changes the way
object_resolve_abs_path() operates, moving away from directly peeking
the property's 'opaque' field to using object_property_get_link().
Thus, in the mew implementation link properties are resolved in the
same way,
as they are when an absolute path is resolved.
Errors relevant to the operation, QERR_OBJECT_PROPERTY_NOT_FOUND
and QERR_OBJECT_PROPERTY_INVALID_TYPE were added.
Also, in the new implementation, some common sense refactoring was done
in the file 'qom/object.c' in the code extracting child and link
properties.
Signed-off-by: Alexander Barabash alexander_barab...@mentor.com
Please use git-send-email to submit your patches: The commit message is
unnecessarily indented and the first line is duplicated. Instead of
Revised: (which v2 already indicates) the subject should mention the
topic, here qom: .
http://wiki.qemu.org/Contribute/SubmitAPatch
Your patch is doing too many things at once. Please split it up into a
series of easily digestable and bisectable patches, e.g.:
* ..._PREFIX/SUFFIX introduction and use in _add_child/link,
* ..._is_child/_is_link introduction and use in
_property_del_child/_get_canonical_path/_resolve_partial_path,
* link_type_to_type() and use in _set_link_property,
* REPORT_OBJECT_ERROR(),
* object_property_get_child().
+/* Go from LINK_PROPERTY_TYPE_PREFIX FOO LINK_PROPERTY_TYPE_SUFFIX to
FOO. */
+static gchar *link_type_to_type(const gchar *type)
+{
+return g_strndup(type[sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1],
+ strlen(type)
+ - (sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1)
+ - (sizeof(LINK_PROPERTY_TYPE_SUFFIX) - 1));
Any reason not to use strlen()? I don't think this is a hot path, and
repeated sizeof() - 1 does not read so nice. The alternative would be to
hardcode the offsets.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH v2] Revised: Add object_property_get_child().
On 19 February 2012 17:14, Andreas Färber afaer...@suse.de wrote: Am 19.02.2012 17:04, schrieb Alexander Barabash: + return g_strndup(type[sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1], + strlen(type) + - (sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1) + - (sizeof(LINK_PROPERTY_TYPE_SUFFIX) - 1)); Any reason not to use strlen()? I don't think this is a hot path, and repeated sizeof() - 1 does not read so nice. gcc will optimise away strlen(constant string) at compile time anyway, so there really is no need to avoid it. -- PMM
