Re: sizeof(struct ...)

[Andy Whitcroft <apw@shadowen.org>]

René Scharfe wrote:
> Oh no, whitespace damage!  The major downside of getting a new
> machine is that Thunderbird needs to be beaten into submission
> again. :-(  Here's the patch a second time, hopefully intact.
> 
> René
> 

This structure represents an on-disk/on-the-wire thing, should we not be
specifying it in some architecture neutral way?  You are going to get
the length right in the case of tail padding but not in the face of any
other padding internally.

You see packing attributes applied to similar things in the kernel.
Perhaps they are relevant here?
Is there not some kind of attribute thing we can apply to this structure
to prevent the padding?  You see that in the kernel from time to time.

struct foo {
} __attribute__((packed));

-apw

> 
>  archive-zip.c |   24 ++++++++++++++++++------
>  1 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/archive-zip.c b/archive-zip.c
> index ae5572a..4caaec4 100644
> --- a/archive-zip.c
> +++ b/archive-zip.c
> @@ -35,6 +35,7 @@ struct zip_local_header {
>  	unsigned char size[4];
>  	unsigned char filename_length[2];
>  	unsigned char extra_length[2];
> +	unsigned char _end[0];
>  };
>  
>  struct zip_dir_header {
> @@ -55,6 +56,7 @@ struct zip_dir_header {
>  	unsigned char attr1[2];
>  	unsigned char attr2[4];
>  	unsigned char offset[4];
> +	unsigned char _end[0];
>  };
>  
>  struct zip_dir_trailer {
> @@ -66,8 +68,18 @@ struct zip_dir_trailer {
>  	unsigned char size[4];
>  	unsigned char offset[4];
>  	unsigned char comment_length[2];
> +	unsigned char _end[0];
>  };
>  
> +/*
> + * On ARM, padding is added at the end of the struct, so a simple
> + * sizeof(struct ...) reports two bytes more than the payload size
> + * we're interested in.
> + */
> +#define ZIP_LOCAL_HEADER_SIZE	offsetof(struct zip_local_header, _end)
> +#define ZIP_DIR_HEADER_SIZE	offsetof(struct zip_dir_header, _end)
> +#define ZIP_DIR_TRAILER_SIZE	offsetof(struct zip_dir_trailer, _end)
> +
>  static void copy_le16(unsigned char *dest, unsigned int n)
>  {
>  	dest[0] = 0xff & n;
> @@ -211,7 +223,7 @@ static int write_zip_entry(const unsigne
>  	}
>  
>  	/* make sure we have enough free space in the dictionary */
> -	direntsize = sizeof(struct zip_dir_header) + pathlen;
> +	direntsize = ZIP_DIR_HEADER_SIZE + pathlen;
>  	while (zip_dir_size < zip_dir_offset + direntsize) {
>  		zip_dir_size += ZIP_DIRECTORY_MIN_SIZE;
>  		zip_dir = xrealloc(zip_dir, zip_dir_size);
> @@ -234,8 +246,8 @@ static int write_zip_entry(const unsigne
>  	copy_le16(dirent.attr1, 0);
>  	copy_le32(dirent.attr2, attr2);
>  	copy_le32(dirent.offset, zip_offset);
> -	memcpy(zip_dir + zip_dir_offset, &dirent, sizeof(struct zip_dir_header));
> -	zip_dir_offset += sizeof(struct zip_dir_header);
> +	memcpy(zip_dir + zip_dir_offset, &dirent, ZIP_DIR_HEADER_SIZE);
> +	zip_dir_offset += ZIP_DIR_HEADER_SIZE;
>  	memcpy(zip_dir + zip_dir_offset, path, pathlen);
>  	zip_dir_offset += pathlen;
>  	zip_dir_entries++;
> @@ -251,8 +263,8 @@ static int write_zip_entry(const unsigne
>  	copy_le32(header.size, uncompressed_size);
>  	copy_le16(header.filename_length, pathlen);
>  	copy_le16(header.extra_length, 0);
> -	write_or_die(1, &header, sizeof(struct zip_local_header));
> -	zip_offset += sizeof(struct zip_local_header);
> +	write_or_die(1, &header, ZIP_LOCAL_HEADER_SIZE);
> +	zip_offset += ZIP_LOCAL_HEADER_SIZE;
>  	write_or_die(1, path, pathlen);
>  	zip_offset += pathlen;
>  	if (compressed_size > 0) {
> @@ -282,7 +294,7 @@ static void write_zip_trailer(const unsi
>  	copy_le16(trailer.comment_length, sha1 ? 40 : 0);
>  
>  	write_or_die(1, zip_dir, zip_dir_offset);
> -	write_or_die(1, &trailer, sizeof(struct zip_dir_trailer));
> +	write_or_die(1, &trailer, ZIP_DIR_TRAILER_SIZE);
>  	if (sha1)
>  		write_or_die(1, sha1_to_hex(sha1), 40);
>  }
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html