Re: [Qemu-devel] [PATCH] net/filter: Fix the output information for command 'info network'

[Eric Blake]

On 01/25/2016 02:12 AM, zhanghailiang wrote:
> The properties of netfilter object could be changed by 'qom-set'
> command, but the output of 'info network' command is not updated,
> because it got the old information through nf->info_str, it will
> not be updated while we change the value of netfilter's property.
> 
> Here we split a helper function that could colletct the output

s/colletct/collect/

> information for filter, and also remove the useless member
> 'info_str' from struct NetFilterState.
> 
> Signed-off-by: zhanghailiang <address@hidden>
> Cc: Jason Wang <address@hidden>
> Cc: Eric Blake <address@hidden>
> Cc: Markus Armbruster <address@hidden>
> Cc: Yang Hongyang <address@hidden>
> ---
>  include/net/filter.h |  3 ++-
>  net/filter.c         | 47 ++++++++++++++++++++++++++---------------------
>  net/net.c            |  5 ++++-
>  3 files changed, 32 insertions(+), 23 deletions(-)
> 

> +++ b/include/net/filter.h
> @@ -55,7 +55,6 @@ struct NetFilterState {
>      char *netdev_id;
>      NetClientState *netdev;
>      NetFilterDirection direction;
> -    char info_str[256];

Fixed size buffer...

>      QTAILQ_ENTRY(NetFilterState) next;
>  };
>  
> @@ -74,4 +73,6 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>                                      int iovcnt,
>                                      void *opaque);
>  
> +void netfilter_print_info(NetFilterState *nf, char *output_str, int size);
> +
>  #endif /* QEMU_NET_FILTER_H */
> diff --git a/net/filter.c b/net/filter.c
> index 5d90f83..40254bd 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -128,6 +128,31 @@ static void netfilter_init(Object *obj)
>                               NULL);
>  }
>  
> +void netfilter_print_info(NetFilterState *nf, char *output_str, int size)
> +{
> +    char *str, *info;
> +    ObjectProperty *prop;
> +    ObjectPropertyIterator iter;
> +    StringOutputVisitor *ov;
> +
> +    /* generate info str */
> +    object_property_iter_init(&iter, OBJECT(nf));
> +    while ((prop = object_property_iter_next(&iter))) {
> +        if (!strcmp(prop->name, "type")) {
> +            continue;
> +        }
> +        ov = string_output_visitor_new(false);
> +        object_property_get(OBJECT(nf), string_output_get_visitor(ov),
> +                            prop->name, NULL);
> +        str = string_output_get_string(ov);
> +        string_output_visitor_cleanup(ov);
> +        info = g_strdup_printf(",%s=%s", prop->name, str);

...but variable-sized print collection.  Why not just rewrite the
function to malloc the entire string, and ditch the fixed-size buffer
and size parameters altogether, while at it?

> +        g_strlcat(output_str, info, size);
> +        g_free(str);
> +        g_free(info);
> +    }
> +}
> +

> +++ b/net/net.c
> @@ -1198,9 +1198,12 @@ void print_net_client(Monitor *mon, NetClientState *nc)
>      }
>      QTAILQ_FOREACH(nf, &nc->filters, next) {
>          char *path = object_get_canonical_path_component(OBJECT(nf));
> +        char info[256] = { 0 };
> +
> +        netfilter_print_info(nf, info, sizeof(info));
>          monitor_printf(mon, "  - %s: type=%s%s\n", path,
>                         object_get_typename(OBJECT(nf)),
> -                       nf->info_str);
> +                       info);

Or, instead of printing into a temporary string just to then replay that
string to monitor_printf(), why not have netfilter_print_info() directly
write to the monitor (that is, use monitor_printf() directly in the
helper function, instead of g_strlcat() into a temporary string).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature