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

[Kevin Wolf]

Am 21.11.2013 um 15:48 hat Stefan Hajnoczi geschrieben:
> 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));

In fact, I'm not sure if the file name should be included here at all.
The caller knows the file name because it passed it as a parameter, so
this is not interesting information.

Adding as much information as we can everywhere leads to error messages
that contain the same information multiple times. This is why I try to
only include information that the caller doesn't already have.

Kevin