| Message ID | 1399996972-23429-15-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 05/13/2014 10:02 AM, Markus Armbruster wrote: > Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/sheepdog.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > @@ -1568,13 +1565,15 @@ static int sd_prealloc(const char *filename) > */ > ret = bdrv_pread(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE); > if (ret < 0) { > - goto out; > + goto out_setg; > } > ret = bdrv_pwrite(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE); > if (ret < 0) { > - goto out; > + goto out_setg; > } > } > +out_setg: > + error_setg_errno(errp, -ret, "Can't pre-allocate"); This unconditionally sets errp even when the for loop completes normally. Are you sure you want this control flow? Maybe you are missing a 'goto out' prior to the 'out_setg' label?
Eric Blake <eblake@redhat.com> writes: > On 05/13/2014 10:02 AM, Markus Armbruster wrote: >> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block/sheepdog.c | 21 ++++++++++++--------- >> 1 file changed, 12 insertions(+), 9 deletions(-) >> >> @@ -1568,13 +1565,15 @@ static int sd_prealloc(const char *filename) >> */ >> ret = bdrv_pread(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE); >> if (ret < 0) { >> - goto out; >> + goto out_setg; >> } >> ret = bdrv_pwrite(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE); >> if (ret < 0) { >> - goto out; >> + goto out_setg; >> } >> } >> +out_setg: >> + error_setg_errno(errp, -ret, "Can't pre-allocate"); > > This unconditionally sets errp even when the for loop completes > normally. Are you sure you want this control flow? Maybe you are > missing a 'goto out' prior to the 'out_setg' label? You're right, will fix, thanks!
Markus Armbruster <armbru@redhat.com> writes: > Eric Blake <eblake@redhat.com> writes: > >> On 05/13/2014 10:02 AM, Markus Armbruster wrote: >>> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> block/sheepdog.c | 21 ++++++++++++--------- >>> 1 file changed, 12 insertions(+), 9 deletions(-) >>> >>> @@ -1568,13 +1565,15 @@ static int sd_prealloc(const char *filename) >>> */ >>> ret = bdrv_pread(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE); >>> if (ret < 0) { >>> - goto out; >>> + goto out_setg; >>> } >>> ret = bdrv_pwrite(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE); >>> if (ret < 0) { >>> - goto out; >>> + goto out_setg; >>> } >>> } >>> +out_setg: >>> + error_setg_errno(errp, -ret, "Can't pre-allocate"); >> >> This unconditionally sets errp even when the for loop completes >> normally. Are you sure you want this control flow? Maybe you are >> missing a 'goto out' prior to the 'out_setg' label? > > You're right, will fix, thanks! I went over the whole series again looking for similar mistakes, and found a few in PATCH 04 and 17. Respin coming.
diff --git a/block/sheepdog.c b/block/sheepdog.c index b932d68..a8385bb 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -1537,27 +1537,24 @@ static int do_sd_create(BDRVSheepdogState *s, uint32_t *vdi_id, int snapshot) return 0; } -static int sd_prealloc(const char *filename) +static int sd_prealloc(const char *filename, Error **errp) { BlockDriverState *bs = NULL; uint32_t idx, max_idx; int64_t vdi_size; void *buf = g_malloc0(SD_DATA_OBJ_SIZE); - Error *local_err = NULL; int ret; ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL, - NULL, &local_err); + NULL, errp); if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); goto out; } vdi_size = bdrv_getlength(bs); if (vdi_size < 0) { ret = vdi_size; - goto out; + goto out_setg; } max_idx = DIV_ROUND_UP(vdi_size, SD_DATA_OBJ_SIZE); @@ -1568,13 +1565,15 @@ static int sd_prealloc(const char *filename) */ ret = bdrv_pread(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE); if (ret < 0) { - goto out; + goto out_setg; } ret = bdrv_pwrite(bs, idx * SD_DATA_OBJ_SIZE, buf, SD_DATA_OBJ_SIZE); if (ret < 0) { - goto out; + goto out_setg; } } +out_setg: + error_setg_errno(errp, -ret, "Can't pre-allocate"); out: if (bs) { bdrv_unref(bs); @@ -1734,7 +1733,11 @@ static int sd_create(const char *filename, QEMUOptionParameter *options, goto out; } - ret = sd_prealloc(filename); + ret = sd_prealloc(filename, &local_err); + if (ret < 0) { + qerror_report_err(local_err); + error_free(local_err); + } out: g_free(s); return ret;
Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block/sheepdog.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)