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

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

On 2016/1/26 4:49, Eric Blake wrote:
> 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 <zhang.zhanghailiang@huawei.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Eric Blake <eblake@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Cc: Yang Hongyang <hongyang.yang@easystack.cn>
>> ---
>>   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?
>

Yes, that is not elegant. It is inherited from the old codes.

>> +        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).
>

Good idea, i will fix it like that in next version, thanks.