Re: [PATCH 3/4] partitions/dos: add function to write partition table

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Michael,

On Mon, Oct 17, 2016 at 03:29:22PM +0200, Michael Grzeschik wrote:
> The function can be used to write an partition layout to the block device
> based on its cdev layout. Only cdevs with flag DEVFS_PARTITION_IN_PT set
> get written. The function also adds an static offset of 0x200000 to
> ensure the mbr and bootloader will not be overwritten.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  common/partitions/dos.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/disks.h         |  1 +
>  2 files changed, 72 insertions(+)
> 
> diff --git a/common/partitions/dos.c b/common/partitions/dos.c
> index 5f08e25..d7fa538 100644
> --- a/common/partitions/dos.c
> +++ b/common/partitions/dos.c
> @@ -256,6 +256,77 @@ static void dos_partition(void *buf, struct block_device *blk,
>  			&dsp->signature, "%08x", dsp);
>  }
>  
> +static inline void hdimage_setup_chs(unsigned int lba, unsigned char *chs)
> +{
> +	const unsigned int hpc = 255;
> +	const unsigned int spt = 63;
> +	unsigned int s, c;
> +
> +	chs[0] = (lba/spt)%hpc;

Please run checkpatch over this. There are some stylistic flaws like
missing whitespaces left and right of operators.

> +	c = (lba/(spt * hpc));
> +	s = (lba > 0) ?(lba%spt + 1) : 0;
> +	chs[1] = ((c & 0x300) >> 2) | (s & 0xff);
> +	chs[2] = (c & 0xff);
> +}
> +
> +int write_dos_partition_table(struct block_device *blk, struct list_head *cdevs)
> +{
> +	char part_table[6+4*sizeof(struct partition_entry)+2];
> +	struct cdev *cdev, *ct;
> +	void *buf;
> +	int ret;
> +	int n = 0;
> +	char *ptr;
> +
> +	/* prepare partition table entry */
> +	ptr = part_table;
> +	memset(ptr, 0, sizeof(part_table));
> +
> +	/* skip disk signature */
> +	ptr += 6;

It's even more important to skip this in the output buffer. This code
should not change the disk signature.

> +	list_for_each_entry_safe(cdev, ct, cdevs, devices_list) {

Why _safe? You do not remove entries, do you?

> +		if ((cdev->flags & DEVFS_IS_PARTITION) &&
> +			(cdev->flags & DEVFS_PARTITION_IN_PT)) {

In a multiline if clause the second line should either start directly
under the opening brace or indented with at least two more tabs than the
opening if(), but using the same indention level as the conditional code
makes it hard to read.

> +			struct partition_entry *entry;

Instead of the silent test below, do a:

			if (n == 4) {
				pr_warn("Only 4 partitions written to MBR\n");
				break;
			}

> +			entry = (struct partition_entry *)
> +				(ptr + n * sizeof(struct partition_entry));
> +
> +			/* add static offset to skip the mbr and bootloader */
> +			cdev->offset += 4096 * SECTOR_SIZE;
> +
> +			entry->type = 0x83;
> +			entry->partition_start = cdev->offset / SECTOR_SIZE;
> +			entry->partition_size = cdev->size / SECTOR_SIZE;

We should have a test if offset and/or size exceed the 32bit limit.

> +
> +			hdimage_setup_chs(entry->partition_start,
> +						entry->chs_begin);
> +			hdimage_setup_chs(entry->partition_start +
> +						entry->partition_size - 1,
> +						entry->chs_end);
> +			n++;
> +		}
> +		if (n == 4)
> +			break;
> +	}
> +
> +	ptr += 4 * sizeof(struct partition_entry);
> +	ptr[0] = 0x55;
> +	ptr[1] = 0xaa;
> +
> +	buf = read_mbr(blk);
> +	if (!buf)
> +		return -EIO;

You could move this to the top of the function and directly manipulate
the input buffer.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox