Re: [PATCH] ntfs: Cleanup string initializations (char[] instead of char *)

[Anton Altaparmakov]

Hi,

NAK.

As Al Viro said in reply to one of your other patches, if you look at the assembler output you will see that the 'char te[] = "string"' generates dreadful code whilst the 'char *te = "string"' generates much better code thus if anything we should change all occurrences of 'char (.*)\[\] =' to "char *\1" instead...

If you think about it logically it actually makes sense, too.  In the 'char te[] = "string"' case we are telling the compiler to take a constant string "string" which will be stored both in the binary and once loaded in memory, then make a copy of it into the array "te" and that is exactly what the compiler does (if the string is short enough the compiler actually copies immediate values instead of storing the string elsewhere but that is still worse than just pointing to constant memory).  Whilst in the 'char *te = "string"' case we are telling the compiler to take a constant string (as previous case) and then assign the address of that string to the pointer variable "te".  Thus no copying takes place.  Thus one should never use the 'char te[] = "string"' form UNLESS you want to then mess about with the contents of the string in which case modifying "te[]" will work as it is a local copy but modifying "*te" will cause a crash or be ignored depending on whether you are programming in the kernel or user space because you are trying to modify a read-only memory segment...

Best regards,

	Anton

On 17 May 2014, at 13:25, Manuel Schölling <manuel.schoelling@xxxxxx> wrote:

> Initializations like 'char *foo = "bar"' will create two variables: a static
> string and a pointer (foo) to that static string. Instead 'char foo[] = "bar"'
> will allocate only a declare a single variable and will end up in shorter
> assembly (according to Jeff Garzik on the KernelJanitor's TODO list).
> 
> Signed-off-by: Manuel Schölling <manuel.schoelling@xxxxxx>
> ---
> fs/ntfs/inode.c |    2 +-
> fs/ntfs/super.c |    2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index f47af5e..3975798 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -2368,7 +2368,7 @@ int ntfs_truncate(struct inode *vi)
> 	ntfs_attr_search_ctx *ctx;
> 	MFT_RECORD *m;
> 	ATTR_RECORD *a;
> -	const char *te = "  Leaving file length out of sync with i_size.";
> +	const char te[] = "  Leaving file length out of sync with i_size.";
> 	int err, mp_size, size_change, alloc_change;
> 	u32 attr_len;
> 
> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
> index 9de2491..eb2c195 100644
> --- a/fs/ntfs/super.c
> +++ b/fs/ntfs/super.c
> @@ -676,7 +676,7 @@ not_ntfs:
> static struct buffer_head *read_ntfs_boot_sector(struct super_block *sb,
> 		const int silent)
> {
> -	const char *read_err_str = "Unable to read %s boot sector.";
> +	const char read_err_str[] = "Unable to read %s boot sector.";
> 	struct buffer_head *bh_primary, *bh_backup;
> 	sector_t nr_blocks = NTFS_SB(sb)->nr_blocks;
> 
> -- 
> 1.7.10.4

-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
University of Cambridge Information Services, Roger Needham Building
7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/