Re: [Qemu-devel] [PATCH 2/6] util: introduce glib-helper.c

[Peter Maydell]

On 21 April 2017 at 16:10, Alexey <address@hidden> wrote:
> Hello, thank you for so  detailed comment,
>
> On Fri, Apr 21, 2017 at 11:27:55AM +0100, Peter Maydell wrote:

>> Can we have a proper doc comment format comment, please,
>> since this is now a function available to all of QEMU?
>>
>> > +gint g_int_cmp64(gconstpointer a, gconstpointer b,
>> > +        gpointer __attribute__((unused)) user_data);
>>
>> What is this actually for? Looking at the original uses
>> I can tell that this is a GCompareDataFunc function, but
>> the comment should tell me that.
> I looked at another functions comments in QEMU, I didn't find
> some common style, and decided keep it as is. Maybe I omitted some
> best practice here.

See include/qemu/bitops.h for an example of the comment style.
More important than just the style is that the comment
should clearly explain the purpose of the function in detail.

Certainly many of our existing functions are poorly documented,
but we're trying to raise the bar gradually here.

> yes, it was copy pasted,
> right now, after mingw build check I think to use intptr_t as a type
> for comparision in this function or even keep gpointer and merge these two
> functions into _direct_.
> I saw intptr_t is widely used in QEMU.
>
> The intent of this function was a comparator for case when client code
> want to keep integers in pointer field. xen_disk.c uses UINT32 so it
> wasn't a problem, but migration uses 64 address (kernel provides it in
> __u64, long long), so on 32 platform it's a problem.

Code which tries to put a genuinely 64 bit value into a pointer
is buggy and needs to be fixed. I'm not clear if that is the
case here, or if the ABI from the kernel guarantees that the
value is really a pointer type and fits in uintptr_t / gpointer.

I don't think we need more than one of these functions.

>> This is also missing the copyright line.
> Yes, maybe it was better for me to ask before send.
> I found in util files with reference to GNU GPL, version 2, like
> in this file, also I found that
>
>  * This library is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU Lesser General Public
>  * License as published by the Free Software Foundation; either
>  * version 2 of the License, or (at your option) any later version.
>
> So I just copied copyright reference from glib-compat.h.

Yes, that's the license statement, which is fine. What is
missing is the copyright line, which in glib-compat.h looks
like:
 Copyright IBM, Corp. 2013

For code you write, you want either your personal or (more likely)
a Samsung copyright line -- check with your company about what
their preferred form is.

thanks
-- PMM