On Tue, Oct 18, 2016 at 08:07:12AM +0200, Sascha Hauer wrote:
> 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@xxxxxxxxxxxxxx>
> > ---
> > 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.
Thanks. I forgot to do that. It was an badly formatet template I used
here for reference. Will fix it.
> > + 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?
No elements get changed in the iteration. I will change it.
> > + 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.
Will be changed.
>
> > + 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;
> }
>
Good thought. Will change.
> > + 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.
>
Good point. Will add in v2.
> > +
> > + 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.
Already prepared for v2.
Thanks,
Michael
--
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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
