Re: [OpenBIOS] Patch for auto partition selection

[Programmingkid]

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++)
> {
>
> 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)
+			{
    			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 
+				{
    				/* 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
+	else 
+	{
    	SEEK( bs * parnum );
    	READ( &par, sizeof(par) );

Patch for auto partition selection

[Blue Swirl]

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) );