Re: [Qemu-block] [PATCH 1/2] qemu-img convert: ignore read errors

[Vladimir Sementsov-Ogievskiy]

09.04.2019 17:56, Andrey Shinkevich wrote:
> The 'qemu-img convert' new command option 'force read' with the key '-R'
> allows converting a damaged image to get all the available information
> in case of the read errors. The program reports read errors and continue
> the image conversion. The users should keep in their minds that the
> resulting image is inconsistent.
> 
> Suggested-by: Denis V. Lunev <address@hidden>
> Signed-off-by: Andrey Shinkevich <address@hidden>
> ---
>   qemu-img-cmds.hx |  4 ++--
>   qemu-img.c       | 18 ++++++++++++++++--
>   qemu-img.texi    |  2 +-
>   3 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 1526f32..09af9cb 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -44,9 +44,9 @@ STEXI
>   ETEXI
>   
>   DEF("convert", img_convert,
> -    "convert [--object objectdef] [--image-opts] [--target-image-opts] [-U] 
> [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] 
> [-B backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m 
> num_coroutines] [-W] filename [filename2 [...]] output_filename")
> +    "convert [--object objectdef] [--image-opts] [--target-image-opts] [-U] 
> [-R] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O 
> output_fmt] [-B backing_file] [-o options] [-l snapshot_param] [-S 
> sparse_size] [-m num_coroutines] [-W] filename [filename2 [...]] 
> output_filename")
>   STEXI
> address@hidden convert [--object @var{objectdef}] [--image-opts] 
> [--target-image-opts] [-U] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t 
> @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B 
> @var{backing_file}] [-o @var{options}] [-l @var{snapshot_param}] [-S 
> @var{sparse_size}] [-m @var{num_coroutines}] [-W] @var{filename} 
> address@hidden [...]] @var{output_filename}
> address@hidden convert [--object @var{objectdef}] [--image-opts] 
> [--target-image-opts] [-U] [-R] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t 
> @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B 
> @var{backing_file}] [-o @var{options}] [-l @var{snapshot_param}] [-S 
> @var{sparse_size}] [-m @var{num_coroutines}] [-W] @var{filename} 
> address@hidden [...]] @var{output_filename}
>   ETEXI
>   
>   DEF("create", img_create,
> diff --git a/qemu-img.c b/qemu-img.c
> index aa6f81f..494e880 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -175,6 +175,7 @@ static void QEMU_NORETURN help(void)
>              "Parameters to convert subcommand:\n"
>              "  '-m' specifies how many coroutines work in parallel during 
> the convert\n"
>              "       process (defaults to 8)\n"
> +           "  '-R' continue the image conversion in case of the sector read 
> error\n"
>              "  '-W' allow to write to the target out of order rather than 
> sequential\n"
>              "\n"
>              "Parameters to snapshot subcommand:\n"
> @@ -1579,6 +1580,7 @@ typedef struct ImgConvertState {
>       int64_t wait_sector_num[MAX_COROUTINES];
>       CoMutex lock;
>       int ret;
> +    bool force_read;
>   } ImgConvertState;
>   
>   static void convert_select_part(ImgConvertState *s, int64_t sector_num,
> @@ -1693,7 +1695,15 @@ static int coroutine_fn 
> convert_co_read(ImgConvertState *s, int64_t sector_num,
>                   blk, (sector_num - src_cur_offset) << BDRV_SECTOR_BITS,
>                   n << BDRV_SECTOR_BITS, &qiov, 0);
>           if (ret < 0) {
> -            return ret;
> +            if (!s->force_read) {
> +                return ret;
> +            }
> +            int64_t offset = (sector_num - src_cur_offset) << 
> BDRV_SECTOR_BITS;
> +            unsigned int bytes = n << BDRV_SECTOR_BITS;

declarations should be at start of block and better separated by one empty line.
On the other hand, if we add these variables, I think better to declare them in 
while
block and reuse for blk_co_preadv too.

with at least this fixed (but note, also, that I'm not good in cmd line option 
defining. Looks like we
have too many places where something should be added):
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> +            error_report("failed to read %d bytes at offset %" PRId64 ": %s",
> +                         bytes, offset, strerror(-ret));
> +            memset(buf, 0, n * BDRV_SECTOR_SIZE);

hmm, may be some non-zero pattern would be better.

> +            /* TODO: retry to read smaller chunks */
>           }
>   
>           sector_num += n;
> @@ -2018,6 +2028,7 @@ static int img_convert(int argc, char **argv)
>           .buf_sectors        = IO_BUF_SIZE / BDRV_SECTOR_SIZE,
>           .wr_in_order        = true,
>           .num_coroutines     = 8,
> +        .force_read         = false,

It is initialized by zero by default, as all other fields. ".copy_range = 
false" is
bad example here, I think.

>       };
>   
>       for(;;) {
> @@ -2029,7 +2040,7 @@ static int img_convert(int argc, char **argv)
>               {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
>               {0, 0, 0, 0}
>           };
> -        c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
> +        c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:RWU",
>                           long_options, NULL);
>           if (c == -1) {
>               break;
> @@ -2132,6 +2143,9 @@ static int img_convert(int argc, char **argv)
>           case 'U':
>               force_share = true;
>               break;
> +        case 'R':
> +            s.force_read = true;
> +            break;
>           case OPTION_OBJECT: {
>               QemuOpts *object_opts;
>               object_opts = qemu_opts_parse_noisily(&qemu_object_opts,
> diff --git a/qemu-img.texi b/qemu-img.texi
> index 3b6710a..94f7d80 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -325,7 +325,7 @@ Error on reading data
>   
>   @end table
>   
> address@hidden convert [--object @var{objectdef}] [--image-opts] 
> [--target-image-opts] [-U] [-C] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t 
> @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B 
> @var{backing_file}] [-o @var{options}] [-l @var{snapshot_param}] [-S 
> @var{sparse_size}] [-m @var{num_coroutines}] [-W] @var{filename} 
> address@hidden [...]] @var{output_filename}
> address@hidden convert [--object @var{objectdef}] [--image-opts] 
> [--target-image-opts] [-U] [-R] [-C] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t 
> @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B 
> @var{backing_file}] [-o @var{options}] [-l @var{snapshot_param}] [-S 
> @var{sparse_size}] [-m @var{num_coroutines}] [-W] @var{filename} 
> address@hidden [...]] @var{output_filename}
>   
>   Convert the disk image @var{filename} or a snapshot @var{snapshot_param}
>   to disk image @var{output_filename} using format @var{output_fmt}. It can 
> be optionally compressed (@code{-c}
> 


-- 
Best regards,
Vladimir