Re: [OpenBIOS] Patch for auto partition selection

[Programmingkid]

On Dec 4, 2012, at 1:48 PM, Blue Swirl wrote:
> On Tue, Dec 4, 2012 at 1:25 AM, Programmingkid
>  wrote:
> > On Dec 3, 2012, at 3:13 PM, Mark Cave-Ayland wrote:
> >
> > On 30/11/12 20:14, Programmingkid wrote:
> >
> > What do you mean when you say something is whitespace damaged?
> >
> > It's a general term for formatting changes that don't affect the patch, e.g.
> >
> > for (i = 0; i <= 10; i++) {
> >
> > to
> >
> > for (i = 0; i <= 10; i++)
> > {
>
> Please take a look at this example for non-wanted style changes. Don't
> just ignore what others are telling you.
> > It means that the patch includes non-functional changes which is generally
> > frowned upon.
> >
> > What section do I not need? Could you give me the first and last line of
> > this section? I need the actual text since the line number will probably not
> > help if our files are different.
> >
> > See below.
> >
> > } else {
> >
> > /* Another partition was explicitly requested */
> >
> > }
> >
> > // if no partition and no file was selected - example: "dir cd:,"
> >
> > else if (parnum == -1)
> >
> > {
> >  // search for the first partition of type Apple_HFS or Apple_HFSX
> >
> >  for(parnum = 1; parnum<= __be32_to_cpu(par.pmMapBlkCnt); parnum++)
> >
> >  {
> >
> >     SEEK( bs * parnum );
> >
> >     READ(&par, sizeof(par) );
> >
> >     DPRINTF("found partition type: %s with status %x\n",
> >
> > par.pmPartType, __be32_to_cpu(par.pmPartStatus));
> >     if(strcmp(par.pmPartType, "Apple_HFS") == 0 ||
> >
> > strcmp(par.pmPartType, "Apple_HFSX") == 0)
> >     {
> >
> >           offs = (long long)__be32_to_cpu(par.pmPyPartStart) * bs;
> >
> >           size = (long long)__be32_to_cpu(par.pmPartBlkCnt) * bs;
> >
> >           goto found;
> >
> >           break;
> >
> >     }
> >
> >  }
> >
> > }
> >
> > // Another partition was explicitly requested
> >
> > else
> >
> > {
> >
> > ATB,
> >
> > Mark.
> >
> > Ok this patch removes the unneeded code and remotes the whitespace damage.
> >
> > signed-off-by: John Arbuckle
> > Index: trunk/openbios-devel/packages/mac-parts.c
> >
> > --- trunk/openbios-devel/packages/mac-parts.c (revision 1075)
> > +++ trunk/openbios-devel/packages/mac-parts.c (working copy)
> > @@ -164,16 +164,12 @@
> >  for (parnum = 1; parnum <= __be32_to_cpu(par.pmMapBlkCnt); parnum++) {
> >  SEEK( bs * parnum );
> >  READ( &par, sizeof(par) );
> > if( __be16_to_cpu(par.pmSig) != DESC_PART_SIGNATURE ||
> >                        !__be32_to_cpu(par.pmPartBlkCnt) )
> > break;
> >
> > DPRINTF("found partition type: %s with status %x\n", par.pmPartType,
> > __be32_to_cpu(par.pmPartStatus));
> > /* If we have a valid, allocated and readable partition... */
> > if( (__be32_to_cpu(par.pmPartStatus) & kPartitionAUXIsValid) &&
> > (__be32_to_cpu(par.pmPartStatus) & kPartitionAUXIsAllocated) &&
> > (__be32_to_cpu(par.pmPartStatus) & kPartitionAUXIsReadable) ) {
> > // If an Apple_HFS or Apple_HFSX partition was found
> > if(strcmp(par.pmPartType, "Apple_HFS") == 0 || strcmp(par.pmPartType,
> >
> > "Apple_HFSX") == 0)
> > {
>
> Here's one case where you have changed the brace style.
> > offs = (long long)__be32_to_cpu(par.pmPyPartStart) * bs;
> >  size = (long long)__be32_to_cpu(par.pmPartBlkCnt) * bs;
> >
> > @@ -181,9 +177,11 @@
> >  if (want_bootcode && (__be32_to_cpu(par.pmPartStatus) &
> > kPartitionAUXIsBootValid)) {
> >  offs += (long long)__be32_to_cpu(par.pmLgBootStart) * bs;
> >  size = (long long)__be32_to_cpu(par.pmBootSize);
> >
> > goto found;
> > } else {
> > }
> > else
> > {
>
> Another, don't change '} else {'.
> > /* Otherwise we were passed a filename and path. So let's
> >    choose the first partition with a valid filesystem */
> >  DPUSH( offs );
> > @@ -196,9 +194,11 @@
> >  }
> >  }
> >  }
> > } else {
> > /* Another partition was explicitly requested */
> > }
> > // Another partition was explicitly requested
>
> And here you changed the comment style to C99. Don't do that.
> > else
> > {
>
> Another unwanted change.
>
> Please take a more care to follow the style, consistency is important
> in software projects. Ignoring review comments will lead to ignored
> patches.
> > SEEK( bs * parnum );
> >  READ( &par, sizeof(par) );

Here is an updated patch with the above issues fixed.
Signed-off-by : John Arbuckle

Index: trunk/openbios-devel/packages/mac-parts.c
===================================================================
--- trunk/openbios-devel/packages/mac-parts.c	(revision 1075)
+++ trunk/openbios-devel/packages/mac-parts.c	(working copy)
@@ -164,16 +164,10 @@
    	for (parnum = 1; parnum <= __be32_to_cpu(par.pmMapBlkCnt); parnum++) {
    		SEEK( bs * parnum );
    		READ( &par, sizeof(par) );
-			if( __be16_to_cpu(par.pmSig) != DESC_PART_SIGNATURE ||
-                            !__be32_to_cpu(par.pmPartBlkCnt) )
-				break;
-
    		DPRINTF("found partition type: %s with status %x\n", par.pmPartType, __be32_to_cpu(par.pmPartStatus));

-			/* If we have a valid, allocated and readable partition... */
-			if( (__be32_to_cpu(par.pmPartStatus) & kPartitionAUXIsValid) &&
-			(__be32_to_cpu(par.pmPartStatus) & kPartitionAUXIsAllocated) &&
-			(__be32_to_cpu(par.pmPartStatus) & kPartitionAUXIsReadable) ) {
+			/* If an Apple_HFS or Apple_HFSX partition was found */
+			if(strcmp(par.pmPartType, "Apple_HFS") == 0 || strcmp(par.pmPartType, "Apple_HFSX") == 0) {
    			offs = (long long)__be32_to_cpu(par.pmPyPartStart) * bs;
    			size = (long long)__be32_to_cpu(par.pmPartBlkCnt) * bs;

@@ -196,9 +190,7 @@
    			}
    		}
    	}
-
-	} else {
-		/* Another partition was explicitly requested */
+	} else {	/* Another partition was explicitly requested */
    	SEEK( bs * parnum );
    	READ( &par, sizeof(par) );

Patch for auto partition selection

[Mark Cave-Ayland]

On 04/12/12 19:29, Programmingkid wrote:
> Here is an updated patch with the above issues fixed.
> Signed-off-by : John Arbuckle <[e-mail]
> >

This is much, much improved. Can you now understand how much easier this 
is to test and review the patch below, compared to your original submission?
> Index: trunk/openbios-devel/packages/mac-parts.c
>
> --- trunk/openbios-devel/packages/mac-parts.c(revision 1075)
> +++ trunk/openbios-devel/packages/mac-parts.c(working copy)
> @@ -164,16 +164,10 @@
> for (parnum = 1; parnum <= __be32_to_cpu(par.pmMapBlkCnt); parnum++) {
> SEEK( bs * parnum );
> READ( &par, sizeof(par) );
> -if( __be16_to_cpu(par.pmSig) != DESC_PART_SIGNATURE ||
> !__be32_to_cpu(par.pmPartBlkCnt) )
>
> -break;

Interesting. So the above looks like a bug - I'd expect that break to be 
a continue so that invalid entries are just skipped, rather than 
terminating the entire loop. Does your patch still work if you leave the 
above if statement in? If not, does it then work if you swap the above 
"break" for a "continue"?
> DPRINTF("found partition type: %s with status %x\n", par.pmPartType,
> __be32_to_cpu(par.pmPartStatus));
>
> -/* If we have a valid, allocated and readable partition... */
> -if( (__be32_to_cpu(par.pmPartStatus) & kPartitionAUXIsValid) &&
> -(__be32_to_cpu(par.pmPartStatus) & kPartitionAUXIsAllocated) &&
> -(__be32_to_cpu(par.pmPartStatus) & kPartitionAUXIsReadable) ) {
> +/* If an Apple_HFS or Apple_HFSX partition was found */
> +if(strcmp(par.pmPartType, "Apple_HFS") == 0 || strcmp(par.pmPartType,
> "Apple_HFSX") == 0) {
> offs = (long long)__be32_to_cpu(par.pmPyPartStart) * bs;
> size = (long long)__be32_to_cpu(par.pmPartBlkCnt) * bs;
>
> @@ -196,9 +190,7 @@
> }
> }
> }
>
> -} else {
> -/* Another partition was explicitly requested */
> +} else {/* Another partition was explicitly requested */
> SEEK( bs * parnum );
> READ( &par, sizeof(par) );

I'd like to wait back from the results of Amadeusz's testing before 
committing a revised version of this - I can see from what has been 
shown already that I need to make some additional changes to mac-parts.c 
and then subsequently rebase the patch on that. It may be that at this 
point the changes become so minimal I can just commit it myself.

ATB,

Mark.