I like it... for the most part.

On Saturday, February 20, Giuseppe Magnotta wrote:
quoted text
> Index: mbr.S
> ===================================================================
> RCS file: /cvs/src/sys/arch/i386/stand/mbr/mbr.S,v
> retrieving revision 1.21
> diff -u -r1.21 mbr.S
> --- mbr.S	25 Jun 2007 14:10:17 -0000	1.21
> +++ mbr.S	20 Feb 2010 15:40:04 -0000
> @@ -35,6 +35,13 @@
>   * with the other copyrights as long as you retain my pseudonym and
>   * this copyright notice in the file.
>   */
> +/* Copyright (c) 2010 Giuseppe Magnotta <giuseppe.magnotta@gmail.com>
> + * last edited 20 Feb 2010
> + * Added the check for a single bootable partition.
> + * You may use this code or fragments thereof in a manner consistent
> + * with the other copyrights as long as you retain my pseudonym and
> + * this copyright notice in the file.
> + */

This can be achieved by simply adding your "Copyright (C)" line right
below the other copyright lines in the file.  The extra text is not
necessary, or contains comments that should be in the commit message.


quoted text
>  	.file	"mbr.S"
>  
> @@ -224,12 +231,26 @@
>  
>  drive_ok:
>  	/* Find the first active partition.
> -	 * Note: this should be the only active partition.  We currently
> -	 * don't check for that.
> +	 * Note: this should be the only active partition. We check for that.

This can be shorter.  "Find first and check for only active partition."
Thanks for updating comments to reflect reality.

quoted text
>  	 */
>  	movw	$pt, %si
>  
>  	movw	$NDOSPART, %cx
> +	xorw	%ax, %ax
> +	xorw	%bx, %bx
> +
> +test_pt:
> +	movb	(%si), %al
> +	addw	%ax, %bx
> +	addw	$PARTSZ, %si
> +	loop	test_pt
> +
> +	cmpw	$DOSACTIVE, %bx
> +	jne	no_part
> +
> +	movw	$pt, %si
> +	movw	$NDOSPART, %cx
> +

Nice way to do this.  When I wrote this I must have been out
to lunch, considering the simple way to do this.

quoted text
>  find_active:
>  	DBGMSG(CHAR_L)
>  	movb	(%si), %al
> @@ -533,7 +554,7 @@
>  efdmbr:	.asciz		"MBR on floppy or old BIOS\r\n"
>  eread:	.asciz		"\r\nRead error\r\n"
>  enoos:	.asciz		"No O/S\r\n"
> -enoboot: .ascii		"No active partition"	/* runs into crlf... */
> +enoboot: .ascii		"Error selecting bootable partition" /* runs in
> to crlf... */
>  crlf:	.asciz		"\r\n"

I wish there was a shorter way to indicate this.  Some MBRs I know simply
say "Corrupt Partition Table", which might be good enough and a bit shorter.


Are you ok with the modification to your copyright/comment statement above?

--Toby.