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
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 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
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 Barabash
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
