Re: [PATCH v2 LIVEPATCH-BUILD-TOOLS] Fix patch creation with GCC 6.1+

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Dec 12, 2016 at 02:55:51PM +0000, Ross Lagerwall wrote:
> GCC 6.1+ fixed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=192 which
> means that .rodata.str1.[0-9]+ sections are now split by function.  We
> could probably be smarter about including just the sections we need, but
> for now, simply include the string sections for all functions as is done
> for previous versions of GCC.

Are there plans to push this to the upstream kpatch repo? Or did
they do it in some other way?
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Reported-by: M A Young <m.a.young@durham.ac.uk>
> ---
> 
> Changed in v2:
> * Clarified commit message.

Thanks. Two little nitpicks - feel free to either ignore them
or take them into account.

Either way:

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
>  create-diff-object.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/create-diff-object.c b/create-diff-object.c
> index 69bcd88..b0d1348 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -1184,6 +1184,43 @@ static void kpatch_process_special_sections(struct kpatch_elf *kelf)
>  	}
>  }
>  
> +/* Returns true if s is a string of only numbers with length > 0. */
> +static int isnumber(const char *s)

Could you use bool?

> +{
> +	do {
> +		if (!*s || !isdigit(*s))
> +			return 0;
> +	} while (*++s);
> +
> +	return 1;
> +}
> +
> +/*
> + * String sections are always included even if unchanged.
> + * The format is either:
> + * .rodata.<func>.str1.[0-9]+ (new in GCC 6.1.0)
> + * or .rodata.str1.[0-9]+ (older versions of GCC)
> + * For the new format we could be smarter and only include the needed
> + * strings sections.
> + */
> +static int should_include_str_section(const char *name)

Ditto here? bool?

> +{
> +	const char *s;
> +
> +	if (strncmp(name, ".rodata.", 8))
> +		return 0;
> +
> +	/* Check if name matches ".rodata.str1.[0-9]+" */
> +	if (!strncmp(name, ".rodata.str1.", 13))
> +		return isnumber(name + 13);

I would make an #define for the '13'
> +
> +	/* Check if name matches ".rodata.<func>.str1.[0-9]+" */
> +	s = strstr(name, ".str1.");
> +	if (!s)
> +		return 0;
> +	return isnumber(s + 6);
> +}
> +
>  static void kpatch_include_standard_elements(struct kpatch_elf *kelf)
>  {
>  	struct section *sec;
> @@ -1193,7 +1230,7 @@ static void kpatch_include_standard_elements(struct kpatch_elf *kelf)
>  		if (!strcmp(sec->name, ".shstrtab") ||
>  		    !strcmp(sec->name, ".strtab") ||
>  		    !strcmp(sec->name, ".symtab") ||
> -		    !strncmp(sec->name, ".rodata.str1.", 13)) {
> +		    should_include_str_section(sec->name)) {
>  			sec->include = 1;
>  			if (sec->secsym)
>  				sec->secsym->include = 1;
> -- 
> 2.7.4
> 

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