[Workman-devel] [PATCH] WorkmanAttribute: don't store format separately

[Daniel P. Berrange]

On Mon, Mar 04, 2013 at 05:27:04PM -0600, Josh Poimboeuf wrote:
> - The format string, or type, of a GVariant is inherent and can be
>   retrieved from the GVariant itself via g_variant_get_type(), so
>   there's no need to specify it separately.

Hmm, but what about if we have an attribute which does not currently
have any value set. ie the GVariant is NULL. We want to represent
the attribute and have its format for validation purposes still.

> - Beef up the error handling in workman_attribute_set_value() to ensure
>   the new type matches the old type.
> ---
>  workman/workman-attribute.c | 40 ++++++++++++++++++----------------------
>  workman/workman-attribute.h |  9 +++------
>  2 files changed, 21 insertions(+), 28 deletions(-)

> -void workman_attribute_set_value(WorkmanAttribute *attr,
> -                                 GVariant *value,
> -                                 GError **error)
> +gboolean
> +workman_attribute_set_value(WorkmanAttribute *attr,
> +                            GVariant *value,
> +                            GError **error)
>  {
> -    if (attr->priv->value)
> +    g_return_val_if_fail(value != NULL, FALSE);
> +
> +    if (attr->priv->value) {
> +        const GVariantType *old_type = g_variant_get_type(attr->priv->value);
> +        const GVariantType *new_type = g_variant_get_type(value);
> +
> +        g_return_val_if_fail(g_variant_type_equal(old_type, new_type),
> +                             FALSE);
> +
>          g_variant_unref(attr->priv->value);
> -    /* XXX validate new value against priv->format */
> +    }
> +
>      attr->priv->value = value;
> -    if (attr->priv->value)
> -        g_variant_ref(attr->priv->value);
> +    g_variant_ref(attr->priv->value);
> +
> +    return TRUE;

If there is no initial value set, this just allows the user to pass
in any old GVariant they like. eg they could be passing in a list of
strings, where we require an unsigned int, and we can't validate this.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|