Re: [Xen-devel] [RFC PATCH 4/8] gcov: add new interface and 3.4 and 4.7 format support

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.10.16 at 13:43, <wei.liu2@xxxxxxxxxx> wrote:
> On Fri, Oct 07, 2016 at 12:10:25PM +0100, Wei Liu wrote:
>> On Fri, Oct 07, 2016 at 05:06:42AM -0600, Jan Beulich wrote:
>> > >>> On 06.10.16 at 16:37, <wei.liu2@xxxxxxxxxx> wrote:
>> > > A new sysctl interface for passing gcov data back to userspace. The new
>> > > interface uses a customised record file format. The new sysctl reuses
>> > > original sysctl number but renames the op to gcov_op.
>> > > 
>> > > Both gcc 3.4 and 4.7 format are supported. The code is rewritten so that
>> > > a new format can be easily added in the future.  Version specific code
>> > > is grouped into different files. The format one needs to use can be
>> > > picked via Kconfig. The default format is 4.7 format.
>> > > 
>> > > Userspace programs to handle extracted data will come in a later patch.
>> > > 
>> > > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
>> > 
>> > Looks reasonable, but wants some cosmetics done: Constification
>> > seems to b possible in a number of places, labels should be
>> > indented by at least one blank, and pointless initializers would be
>> > nice to see eliminated. Also, unless at least the removal patch is
>> > still meant to go in for 4.8, you'll need to bump
>> > XEN_SYSCTL_INTERFACE_VERSION in the patch here.
>> > 
>> 
>> Regarding cosmetic issues, I will try to fix as many as possible.
>> 
>> The removal patch won't go in 4.8 because though the implementation has
>> a lot of limitations it might still be useful to some people. I will
>> bump XEN_SYSCTL_INTERFACE_VERSION here in this patch.
>> 
> 
> I go through most (if not all) initialisers, actually they are necessary:
> 
> 1. those accumulative counters need to be initialised to 0.

In e.g.

+    uint32_t total_size = 0;
+    struct gcov_info *info = NULL;
+
+    /* Magic number XCOV */
+    total_size += sizeof(uint32_t);

I can't see why the sizeof(uint32_t) can't be the initializer right away,
or the initializer be dropped and = used in place of += .

> 2. gcov_info_next expects NULL for first invocation.

Oh, I've indeed overlooked this requirement.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel