Re: [LTP] [PATCH] [PATCH v5] Migrating the libhugetlbfs/testcases/truncate_above_4GB.c test

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi Pavithra,

The following is just a coding style/conventions review, not a logic
review. I will leave that for people who worked on huge files.

---------

The subject line should follow LTP conventions:

  hugemmap: Add test for hugepage truncation above 4GB

> Changes in v5:
> - Modified code to use FOURGIG instead of truncate_point.

Same issue as noted on your previous patch: the commit body should
explain *why* the test is being added — what kernel bug it guards
against, what it verifies. Changelog entries belong below the ---
separator in the patch email, not in the commit body.

> + *[Descripiton]

Missing space after *, "Descripiton" is misspelled, and [Description]
is a deprecated label in LTP doc comments. Drop the label entirely —
the existing description text below it is fine on its own:

  /*\
   * At one stage, a misconversion of hugetlb_vmtruncate_list to a
   * ...

> +static void sigbus_handler_fail(int signum, siginfo_t *si, void *uc)
> +{
> +	siglongjmp(sig_escape, 17);
> +}
> +
> +static void sigbus_handler_pass(int signum, siginfo_t *si, void *uc)

All three parameters are unused in both handlers, producing a compiler
warning for 'uc'. Mark them with LTP_ATTRIBUTE_UNUSED:

  static void sigbus_handler_fail(int signum LTP_ATTRIBUTE_UNUSED,
                                  siginfo_t *si LTP_ATTRIBUTE_UNUSED,
                                  void *uc LTP_ATTRIBUTE_UNUSED)

Apply the same to sigbus_handler_pass.

> +	static long long buggy_offset;
> +	...
> +	buggy_offset = FOURGIG / (hpage_size / page_size);
> +	buggy_offset = (long long)PALIGN(buggy_offset, hpage_size);

buggy_offset is recomputed from global statics, make it public instead.

> +cleanup:
> +	SAFE_MUNMAP(q, 3*hpage_size);
> +	SAFE_MUNMAP(p, hpage_size);

p == q + 2*hpage_size, which falls within [q, q + 3*hpage_size). The
first SAFE_MUNMAP already covers p's region, so the second call is a
redundant double-unmap.

Regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp