Re: [Qemu-devel] [PATCH V8 2/4] block: Add check infinite loop in bdrv_img_create()

[Stefan Hajnoczi]

On Fri, Nov 15, 2013 at 01:37:21AM -0500, Xu Wang wrote:
> @@ -4627,15 +4627,17 @@ void bdrv_img_create(const char *filename, const char 
> *fmt,
>      }
>  
>      backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
> +    backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
>      if (backing_file && backing_file->value.s) {
> -        if (!strcmp(filename, backing_file->value.s)) {
> -            error_setg(errp, "Error: Trying to create an image with the "
> -                             "same filename as the backing file");
> +        if (!bdrv_backing_chain_okay(backing_file->value.s, 
> +                                     backing_fmt->value.s, filename,

This assumes backing_fmt != NULL.

> +                                     &local_err)) {
> +            error_setg(errp, "bdrv_img_create: Image %s create failed. %s",
> +                       filename, error_get_pretty(local_err));

This error message is not consistent with other error_setg() usage in
QEMU.  The function name is normally not included.  It also helps to
quote the filename (in case it has spaces):

error_setg(errp, "Failed to create image '%s': %s",
           filename, error_get_pretty(local_err));

>              goto out;
>          }
>      }
>  
> -    backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
>      if (backing_fmt && backing_fmt->value.s) {

Here we check first before dereferencing backing_fmt, so the assumption
is that backing_fmt may be NULL.