| Message ID | 1279482150-19385-1-git-send-email-weil@mail.berlios.de |
|---|---|
| State | New |
| Headers | show |
Am 18.07.2010 21:42, schrieb Stefan Weil: > "No such file or directory" is a misleading error message > when a user tries to open a file with wrong permissions. > > Cc: Kevin Wolf <kwolf@redhat.com> > Signed-off-by: Stefan Weil <weil@mail.berlios.de> > --- > block.c | 12 ++++++++---- > 1 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/block.c b/block.c > index f837876..2f80540 100644 > --- a/block.c > +++ b/block.c > @@ -330,16 +330,20 @@ BlockDriver *bdrv_find_protocol(const char *filename) > return NULL; > } > > -static BlockDriver *find_image_format(const char *filename) > +static BlockDriver *find_image_format(const char *filename, int *error) Wouldn't it be a more natural interface to return an 0/-errno int and pass the BlockDriver* by reference? I think we already have some function that work this way in the block code, but I can't remember any that get an int *error. > { > int ret, score, score_max; > BlockDriver *drv1, *drv; > uint8_t buf[2048]; > BlockDriverState *bs; > > + *error = -ENOENT; Why -ENOENT is the default would be clearer if you moved it down next to the drv = NULL before the loop that searches for the driver. Apart from these minor nitpicks it looks good. Kevin
Am 19.07.2010 14:26, schrieb Kevin Wolf: > Am 18.07.2010 21:42, schrieb Stefan Weil: > >> "No such file or directory" is a misleading error message >> when a user tries to open a file with wrong permissions. >> >> Cc: Kevin Wolf<kwolf@redhat.com> >> Signed-off-by: Stefan Weil<weil@mail.berlios.de> >> --- >> block.c | 12 ++++++++---- >> 1 files changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/block.c b/block.c >> index f837876..2f80540 100644 >> --- a/block.c >> +++ b/block.c >> @@ -330,16 +330,20 @@ BlockDriver *bdrv_find_protocol(const char *filename) >> return NULL; >> } >> >> -static BlockDriver *find_image_format(const char *filename) >> +static BlockDriver *find_image_format(const char *filename, int *error) >> > Wouldn't it be a more natural interface to return an 0/-errno int and > pass the BlockDriver* by reference? I think we already have some > function that work this way in the block code, but I can't remember any > that get an int *error. > ... nor did I find a function which takes a BlockDriver**. But if you prefer it like that, I can send a new version of the patch. > >> { >> int ret, score, score_max; >> BlockDriver *drv1, *drv; >> uint8_t buf[2048]; >> BlockDriverState *bs; >> >> + *error = -ENOENT; >> > Why -ENOENT is the default would be clearer if you moved it down next to > the drv = NULL before the loop that searches for the driver. > > What about the "return bdrv_find_format" lines? They need a default value, too. And I did not want to change too much because I cannot run a complete test for all cases. So setting *error at the beginning should be the safest modification. > Apart from these minor nitpicks it looks good. > > Kevin > > Thanks. Stefan
Am 19.07.2010 22:55, schrieb Stefan Weil: > Am 19.07.2010 14:26, schrieb Kevin Wolf: >> Am 18.07.2010 21:42, schrieb Stefan Weil: >> >>> "No such file or directory" is a misleading error message >>> when a user tries to open a file with wrong permissions. >>> >>> Cc: Kevin Wolf<kwolf@redhat.com> >>> Signed-off-by: Stefan Weil<weil@mail.berlios.de> >>> --- >>> block.c | 12 ++++++++---- >>> 1 files changed, 8 insertions(+), 4 deletions(-) >>> >>> diff --git a/block.c b/block.c >>> index f837876..2f80540 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -330,16 +330,20 @@ BlockDriver *bdrv_find_protocol(const char *filename) >>> return NULL; >>> } >>> >>> -static BlockDriver *find_image_format(const char *filename) >>> +static BlockDriver *find_image_format(const char *filename, int *error) >>> >> Wouldn't it be a more natural interface to return an 0/-errno int and >> pass the BlockDriver* by reference? I think we already have some >> function that work this way in the block code, but I can't remember any >> that get an int *error. >> > > ... nor did I find a function which takes a BlockDriver**. > But if you prefer it like that, I can send a new version of the patch. Yes, I would prefer it like that. If you don't mind, please send a new version. >> >>> { >>> int ret, score, score_max; >>> BlockDriver *drv1, *drv; >>> uint8_t buf[2048]; >>> BlockDriverState *bs; >>> >>> + *error = -ENOENT; >>> >> Why -ENOENT is the default would be clearer if you moved it down next to >> the drv = NULL before the loop that searches for the driver. >> >> > > What about the "return bdrv_find_format" lines? > They need a default value, too. > > And I did not want to change too much because I cannot > run a complete test for all cases. > > So setting *error at the beginning should be the safest > modification. You mean the return bdrv_find_format("raw")? Shouldn't ever fail unless someone was crazy enough to compile raw out, but you're right. Better leave it as it is. Kevin
diff --git a/block.c b/block.c index f837876..2f80540 100644 --- a/block.c +++ b/block.c @@ -330,16 +330,20 @@ BlockDriver *bdrv_find_protocol(const char *filename) return NULL; } -static BlockDriver *find_image_format(const char *filename) +static BlockDriver *find_image_format(const char *filename, int *error) { int ret, score, score_max; BlockDriver *drv1, *drv; uint8_t buf[2048]; BlockDriverState *bs; + *error = -ENOENT; + ret = bdrv_file_open(&bs, filename, 0); - if (ret < 0) + if (ret < 0) { + *error = ret; return NULL; + } /* Return the raw BlockDriver * to scsi-generic devices or empty drives */ if (bs->sg || !bdrv_is_inserted(bs)) { @@ -350,6 +354,7 @@ static BlockDriver *find_image_format(const char *filename) ret = bdrv_pread(bs, 0, buf, sizeof(buf)); bdrv_delete(bs); if (ret < 0) { + *error = ret; return NULL; } @@ -571,12 +576,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, /* Find the right image format driver */ if (!drv) { - drv = find_image_format(filename); + drv = find_image_format(filename, &ret); probed = 1; } if (!drv) { - ret = -ENOENT; goto unlink_and_fail; }
"No such file or directory" is a misleading error message when a user tries to open a file with wrong permissions. Cc: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Stefan Weil <weil@mail.berlios.de> --- block.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-)