Re: [Qemu-devel] [PATCH 1/2] Block: Cleanup vvfat.c to remove dead code.

["Alex Bennée" <alex.bennee@linaro.org>]

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