diff mbox

[1/2] Block: Cleanup vvfat.c to remove dead code.

Message ID 1460689374-9690-2-git-send-email-saxenap.ltc@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Prerna Saxena April 15, 2016, 3:02 a.m. UTC
Commit 43dc2a64 replaced assert() with abort(), but didnt remove statements
that followed these calls. So current code still has return values set after
a call to abort(). Such statements will never execute and need to be cleaned
up.

Signed-off-by: Prerna Saxena <saxenap.ltc@nutanix.com>
---
 block/vvfat.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

Comments

Stefan Hajnoczi April 27, 2016, 3:20 p.m. UTC | #1
On Fri, Apr 15, 2016 at 08:32:53AM +0530, Prerna Saxena wrote:
> Commit 43dc2a64 replaced assert() with abort(), but didnt remove statements
> that followed these calls. So current code still has return values set after
> a call to abort(). Such statements will never execute and need to be cleaned
> up.
> 
> Signed-off-by: Prerna Saxena <saxenap.ltc@nutanix.com>
> ---
>  block/vvfat.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 6b85314..ffe739b 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1747,8 +1747,7 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
>  	    schedule_new_file(s, g_strdup(path), cluster_num);
>  	else {
>              abort();
> -	    return 0;
> -	}
> +	    }

The following change breaks indentation:

  ^Ielse {
  ...
  -^I}
  +^I    }

Since the else statement only has a single tab character it's wrong to
have mixed tabs and spaces for the closing curly.

Please resend this patch with tab-only indentation.  The diff hunks
below sometimes introduce spaces.
Alex Bennée April 27, 2016, 4:07 p.m. UTC | #2
Prerna Saxena <saxenap.ltc@gmail.com> writes:

> Commit 43dc2a64 replaced assert() with abort(), but didnt remove statements
> that followed these calls. So current code still has return values set after
> a call to abort(). Such statements will never execute and need to be cleaned
> up.
>
> Signed-off-by: Prerna Saxena <saxenap.ltc@nutanix.com>
> ---
>  block/vvfat.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 6b85314..ffe739b 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1747,8 +1747,7 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
>  	    schedule_new_file(s, g_strdup(path), cluster_num);
>  	else {
>              abort();
> -	    return 0;
> -	}
> +	    }
>      }
>
>      while(1) {
> @@ -1768,7 +1767,6 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
>  			    * (cluster_num - mapping->begin)) {
>  			/* offset of this cluster in file chain has changed */
>                          abort();
> -			copy_it = 1;
>  		    } else if (offset == 0) {
>  			const char* basename = get_basename(mapping->path);
>
> @@ -1780,7 +1778,6 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
>  		    if (mapping->first_mapping_index != first_mapping_index
>  			    && mapping->info.file.offset > 0) {
>                          abort();
> -			copy_it = 1;
>  		    }
>
>  		    /* need to write out? */
> @@ -1946,8 +1943,6 @@ DLOG(fprintf(stderr, "check direntry %d:\n", i); print_direntry(direntries + i))
>  		}
>  	    } else
>                  abort(); /* cluster_count = 0; */
> -
> -	    ret += cluster_count;

I don't think you meant to do this. The ret += is not part of the else leg.

>  	}
>
>  	cluster_num = modified_fat_get(s, cluster_num);
> @@ -2578,10 +2573,6 @@ static int handle_commits(BDRVVVFATState* s)
>      for (i = 0; !fail && i < s->commits.next; i++) {
>  	commit_t* commit = array_get(&(s->commits), i);
>  	switch(commit->action) {
> -	case ACTION_RENAME: case ACTION_MKDIR:
> -            abort();
> -	    fail = -2;
> -	    break;
>  	case ACTION_WRITEOUT: {
>  #ifndef NDEBUG
>              /* these variables are only used by assert() below */
> @@ -2639,6 +2630,8 @@ static int handle_commits(BDRVVVFATState* s)
>
>  	    break;
>  	}
> +    case ACTION_RENAME:
> +    case ACTION_MKDIR:
>  	default:
>              abort();
>  	}
> @@ -2729,7 +2722,6 @@ static int do_commit(BDRVVVFATState* s)
>      if (ret) {
>  	fprintf(stderr, "Error handling renames (%d)\n", ret);
>          abort();
> -	return ret;
>      }
>
>      /* copy FAT (with bdrv_read) */
> @@ -2740,21 +2732,18 @@ static int do_commit(BDRVVVFATState* s)
>      if (ret) {
>  	fprintf(stderr, "Fatal: error while committing (%d)\n", ret);
>          abort();
> -	return ret;
>      }
>
>      ret = handle_commits(s);
>      if (ret) {
>  	fprintf(stderr, "Error handling commits (%d)\n", ret);
>          abort();
> -	return ret;
>      }
>
>      ret = handle_deletes(s);
>      if (ret) {
>  	fprintf(stderr, "Error deleting\n");
>          abort();
> -	return ret;
>      }
>
>      if (s->qcow->drv->bdrv_make_empty) {


--
Alex Bennée
diff mbox

Patch

diff --git a/block/vvfat.c b/block/vvfat.c
index 6b85314..ffe739b 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1747,8 +1747,7 @@  static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
 	    schedule_new_file(s, g_strdup(path), cluster_num);
 	else {
             abort();
-	    return 0;
-	}
+	    }
     }
 
     while(1) {
@@ -1768,7 +1767,6 @@  static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
 			    * (cluster_num - mapping->begin)) {
 			/* offset of this cluster in file chain has changed */
                         abort();
-			copy_it = 1;
 		    } else if (offset == 0) {
 			const char* basename = get_basename(mapping->path);
 
@@ -1780,7 +1778,6 @@  static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
 		    if (mapping->first_mapping_index != first_mapping_index
 			    && mapping->info.file.offset > 0) {
                         abort();
-			copy_it = 1;
 		    }
 
 		    /* need to write out? */
@@ -1946,8 +1943,6 @@  DLOG(fprintf(stderr, "check direntry %d:\n", i); print_direntry(direntries + i))
 		}
 	    } else
                 abort(); /* cluster_count = 0; */
-
-	    ret += cluster_count;
 	}
 
 	cluster_num = modified_fat_get(s, cluster_num);
@@ -2578,10 +2573,6 @@  static int handle_commits(BDRVVVFATState* s)
     for (i = 0; !fail && i < s->commits.next; i++) {
 	commit_t* commit = array_get(&(s->commits), i);
 	switch(commit->action) {
-	case ACTION_RENAME: case ACTION_MKDIR:
-            abort();
-	    fail = -2;
-	    break;
 	case ACTION_WRITEOUT: {
 #ifndef NDEBUG
             /* these variables are only used by assert() below */
@@ -2639,6 +2630,8 @@  static int handle_commits(BDRVVVFATState* s)
 
 	    break;
 	}
+    case ACTION_RENAME:
+    case ACTION_MKDIR:
 	default:
             abort();
 	}
@@ -2729,7 +2722,6 @@  static int do_commit(BDRVVVFATState* s)
     if (ret) {
 	fprintf(stderr, "Error handling renames (%d)\n", ret);
         abort();
-	return ret;
     }
 
     /* copy FAT (with bdrv_read) */
@@ -2740,21 +2732,18 @@  static int do_commit(BDRVVVFATState* s)
     if (ret) {
 	fprintf(stderr, "Fatal: error while committing (%d)\n", ret);
         abort();
-	return ret;
     }
 
     ret = handle_commits(s);
     if (ret) {
 	fprintf(stderr, "Error handling commits (%d)\n", ret);
         abort();
-	return ret;
     }
 
     ret = handle_deletes(s);
     if (ret) {
 	fprintf(stderr, "Error deleting\n");
         abort();
-	return ret;
     }
 
     if (s->qcow->drv->bdrv_make_empty) {