Message ID | 1401287125-355-10-git-send-email-armbru@redhat.com |
---|---|
State | New |
Headers | show |
On 28.05.2014 16:25, Markus Armbruster wrote: > Chiefly so I don't have to do the error checking in quadruplicate in > the next commit. Moreover, replacing the frequently updated > bs_sectors by an array assigned just once makes the code easier to > understand. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > qemu-img.c | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/qemu-img.c b/qemu-img.c > index 8d996ba..229c0c6 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1186,7 +1186,7 @@ static int img_convert(int argc, char **argv) > BlockDriver *drv, *proto_drv; > BlockDriverState **bs = NULL, *out_bs = NULL; > int64_t total_sectors, nb_sectors, sector_num, bs_offset; > - uint64_t bs_sectors; > + uint64_t *bs_sectors = NULL; > uint8_t * buf = NULL; > size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE; > const uint8_t *buf1; > @@ -1326,7 +1326,8 @@ static int img_convert(int argc, char **argv) > > qemu_progress_print(0, 100); > > - bs = g_malloc0(bs_n * sizeof(BlockDriverState *)); > + bs = g_new(BlockDriverState *, bs_n); I think this should rather be g_new0(), so the clean-up code after "out:" doesn't get confused about which BDS are valid and which are not. Other than that, this patch looks good to me. > + bs_sectors = g_new(uint64_t, bs_n); > > total_sectors = 0; > for (bs_i = 0; bs_i < bs_n; bs_i++) { > @@ -1340,8 +1341,8 @@ static int img_convert(int argc, char **argv) > ret = -1; > goto out; > } > - bdrv_get_geometry(bs[bs_i], &bs_sectors); > - total_sectors += bs_sectors; > + bdrv_get_geometry(bs[bs_i], &bs_sectors[bs_i]); > + total_sectors += bs_sectors[bs_i]; > } > > if (sn_opts) { > @@ -1465,7 +1466,6 @@ static int img_convert(int argc, char **argv) > > bs_i = 0; > bs_offset = 0; > - bdrv_get_geometry(bs[0], &bs_sectors); > > /* increase bufsectors from the default 4096 (2M) if opt_transfer_length > * or discard_alignment of the out_bs is greater. Limit to 32768 (16MB) > @@ -1532,19 +1532,19 @@ static int img_convert(int argc, char **argv) > buf2 = buf; > while (remainder > 0) { > int nlow; > - while (bs_num == bs_sectors) { > + while (bs_num == bs_sectors[bs_i]) { > + bs_offset += bs_sectors[bs_i]; > bs_i++; > assert (bs_i < bs_n); > - bs_offset += bs_sectors; > - bdrv_get_geometry(bs[bs_i], &bs_sectors); > bs_num = 0; > /* printf("changing part: sector_num=%" PRId64 ", " > "bs_i=%d, bs_offset=%" PRId64 ", bs_sectors=%" PRId64 > - "\n", sector_num, bs_i, bs_offset, bs_sectors); */ > + "\n", sector_num, bs_i, bs_offset, bs_sectors[bs_i]); */ > } > - assert (bs_num < bs_sectors); > + assert (bs_num < bs_sectors[bs_i]); > > - nlow = (remainder > bs_sectors - bs_num) ? bs_sectors - bs_num : remainder; > + nlow = remainder > bs_sectors[bs_i] - bs_num > + ? bs_sectors[bs_i] - bs_num : remainder; > > ret = bdrv_read(bs[bs_i], bs_num, buf2, nlow); > if (ret < 0) { > @@ -1605,14 +1605,13 @@ restart: > break; > } > > - while (sector_num - bs_offset >= bs_sectors) { > + while (sector_num - bs_offset >= bs_sectors[bs_i]) { > + bs_offset += bs_sectors[bs_i]; > bs_i ++; > assert (bs_i < bs_n); > - bs_offset += bs_sectors; > - bdrv_get_geometry(bs[bs_i], &bs_sectors); > /* printf("changing part: sector_num=%" PRId64 ", bs_i=%d, " > "bs_offset=%" PRId64 ", bs_sectors=%" PRId64 "\n", > - sector_num, bs_i, bs_offset, bs_sectors); */ > + sector_num, bs_i, bs_offset, bs_sectors[bs_i]); */ > } > > if ((out_baseimg || has_zero_init) && > @@ -1665,7 +1664,7 @@ restart: > } > } > > - n = MIN(n, bs_sectors - (sector_num - bs_offset)); > + n = MIN(n, bs_sectors[bs_i] - (sector_num - bs_offset)); > > sectors_read += n; > if (count_allocated_sectors) { > @@ -1723,6 +1722,7 @@ out: > } > g_free(bs); > } > + g_free(bs_sectors); > fail_getopt: > g_free(options); >
On 28.05.2014 16:25, Markus Armbruster wrote: > Chiefly so I don't have to do the error checking in quadruplicate in > the next commit. Moreover, replacing the frequently updated > bs_sectors by an array assigned just once makes the code easier to > understand. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > qemu-img.c | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com>
On 28.05.2014 23:26, Max Reitz wrote: > On 28.05.2014 16:25, Markus Armbruster wrote: >> Chiefly so I don't have to do the error checking in quadruplicate in >> the next commit. Moreover, replacing the frequently updated >> bs_sectors by an array assigned just once makes the code easier to >> understand. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> qemu-img.c | 32 ++++++++++++++++---------------- >> 1 file changed, 16 insertions(+), 16 deletions(-) > > Reviewed-by: Max Reitz <mreitz@redhat.com> Ups, sorry, this was meant for 10/10.
Max Reitz <mreitz@redhat.com> writes: > On 28.05.2014 16:25, Markus Armbruster wrote: >> Chiefly so I don't have to do the error checking in quadruplicate in >> the next commit. Moreover, replacing the frequently updated >> bs_sectors by an array assigned just once makes the code easier to >> understand. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> qemu-img.c | 32 ++++++++++++++++---------------- >> 1 file changed, 16 insertions(+), 16 deletions(-) >> >> diff --git a/qemu-img.c b/qemu-img.c >> index 8d996ba..229c0c6 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -1186,7 +1186,7 @@ static int img_convert(int argc, char **argv) >> BlockDriver *drv, *proto_drv; >> BlockDriverState **bs = NULL, *out_bs = NULL; >> int64_t total_sectors, nb_sectors, sector_num, bs_offset; >> - uint64_t bs_sectors; >> + uint64_t *bs_sectors = NULL; >> uint8_t * buf = NULL; >> size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE; >> const uint8_t *buf1; >> @@ -1326,7 +1326,8 @@ static int img_convert(int argc, char **argv) >> qemu_progress_print(0, 100); >> - bs = g_malloc0(bs_n * sizeof(BlockDriverState *)); >> + bs = g_new(BlockDriverState *, bs_n); > > I think this should rather be g_new0(), so the clean-up code after > "out:" doesn't get confused about which BDS are valid and which are > not. You're right. I'm afraid manually converting to g_new() & friends when I touch the line anyway is too error prone for me. I'll dust off the block parts of my "Use g_new() & friends where that makes obvious sense" series. > Other than that, this patch looks good to me. Thanks! May I add your R-by if all I change is fixing the g_new() to g_new0()?
On 30.05.2014 08:56, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > >> On 28.05.2014 16:25, Markus Armbruster wrote: >>> Chiefly so I don't have to do the error checking in quadruplicate in >>> the next commit. Moreover, replacing the frequently updated >>> bs_sectors by an array assigned just once makes the code easier to >>> understand. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> qemu-img.c | 32 ++++++++++++++++---------------- >>> 1 file changed, 16 insertions(+), 16 deletions(-) >>> >>> diff --git a/qemu-img.c b/qemu-img.c >>> index 8d996ba..229c0c6 100644 >>> --- a/qemu-img.c >>> +++ b/qemu-img.c >>> @@ -1186,7 +1186,7 @@ static int img_convert(int argc, char **argv) >>> BlockDriver *drv, *proto_drv; >>> BlockDriverState **bs = NULL, *out_bs = NULL; >>> int64_t total_sectors, nb_sectors, sector_num, bs_offset; >>> - uint64_t bs_sectors; >>> + uint64_t *bs_sectors = NULL; >>> uint8_t * buf = NULL; >>> size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE; >>> const uint8_t *buf1; >>> @@ -1326,7 +1326,8 @@ static int img_convert(int argc, char **argv) >>> qemu_progress_print(0, 100); >>> - bs = g_malloc0(bs_n * sizeof(BlockDriverState *)); >>> + bs = g_new(BlockDriverState *, bs_n); >> I think this should rather be g_new0(), so the clean-up code after >> "out:" doesn't get confused about which BDS are valid and which are >> not. > You're right. > > I'm afraid manually converting to g_new() & friends when I touch the > line anyway is too error prone for me. I'll dust off the block parts of > my "Use g_new() & friends where that makes obvious sense" series. > >> Other than that, this patch looks good to me. > Thanks! May I add your R-by if all I change is fixing the g_new() to > g_new0()? Yes, feel free to. :-) Max
diff --git a/qemu-img.c b/qemu-img.c index 8d996ba..229c0c6 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1186,7 +1186,7 @@ static int img_convert(int argc, char **argv) BlockDriver *drv, *proto_drv; BlockDriverState **bs = NULL, *out_bs = NULL; int64_t total_sectors, nb_sectors, sector_num, bs_offset; - uint64_t bs_sectors; + uint64_t *bs_sectors = NULL; uint8_t * buf = NULL; size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE; const uint8_t *buf1; @@ -1326,7 +1326,8 @@ static int img_convert(int argc, char **argv) qemu_progress_print(0, 100); - bs = g_malloc0(bs_n * sizeof(BlockDriverState *)); + bs = g_new(BlockDriverState *, bs_n); + bs_sectors = g_new(uint64_t, bs_n); total_sectors = 0; for (bs_i = 0; bs_i < bs_n; bs_i++) { @@ -1340,8 +1341,8 @@ static int img_convert(int argc, char **argv) ret = -1; goto out; } - bdrv_get_geometry(bs[bs_i], &bs_sectors); - total_sectors += bs_sectors; + bdrv_get_geometry(bs[bs_i], &bs_sectors[bs_i]); + total_sectors += bs_sectors[bs_i]; } if (sn_opts) { @@ -1465,7 +1466,6 @@ static int img_convert(int argc, char **argv) bs_i = 0; bs_offset = 0; - bdrv_get_geometry(bs[0], &bs_sectors); /* increase bufsectors from the default 4096 (2M) if opt_transfer_length * or discard_alignment of the out_bs is greater. Limit to 32768 (16MB) @@ -1532,19 +1532,19 @@ static int img_convert(int argc, char **argv) buf2 = buf; while (remainder > 0) { int nlow; - while (bs_num == bs_sectors) { + while (bs_num == bs_sectors[bs_i]) { + bs_offset += bs_sectors[bs_i]; bs_i++; assert (bs_i < bs_n); - bs_offset += bs_sectors; - bdrv_get_geometry(bs[bs_i], &bs_sectors); bs_num = 0; /* printf("changing part: sector_num=%" PRId64 ", " "bs_i=%d, bs_offset=%" PRId64 ", bs_sectors=%" PRId64 - "\n", sector_num, bs_i, bs_offset, bs_sectors); */ + "\n", sector_num, bs_i, bs_offset, bs_sectors[bs_i]); */ } - assert (bs_num < bs_sectors); + assert (bs_num < bs_sectors[bs_i]); - nlow = (remainder > bs_sectors - bs_num) ? bs_sectors - bs_num : remainder; + nlow = remainder > bs_sectors[bs_i] - bs_num + ? bs_sectors[bs_i] - bs_num : remainder; ret = bdrv_read(bs[bs_i], bs_num, buf2, nlow); if (ret < 0) { @@ -1605,14 +1605,13 @@ restart: break; } - while (sector_num - bs_offset >= bs_sectors) { + while (sector_num - bs_offset >= bs_sectors[bs_i]) { + bs_offset += bs_sectors[bs_i]; bs_i ++; assert (bs_i < bs_n); - bs_offset += bs_sectors; - bdrv_get_geometry(bs[bs_i], &bs_sectors); /* printf("changing part: sector_num=%" PRId64 ", bs_i=%d, " "bs_offset=%" PRId64 ", bs_sectors=%" PRId64 "\n", - sector_num, bs_i, bs_offset, bs_sectors); */ + sector_num, bs_i, bs_offset, bs_sectors[bs_i]); */ } if ((out_baseimg || has_zero_init) && @@ -1665,7 +1664,7 @@ restart: } } - n = MIN(n, bs_sectors - (sector_num - bs_offset)); + n = MIN(n, bs_sectors[bs_i] - (sector_num - bs_offset)); sectors_read += n; if (count_allocated_sectors) { @@ -1723,6 +1722,7 @@ out: } g_free(bs); } + g_free(bs_sectors); fail_getopt: g_free(options);
Chiefly so I don't have to do the error checking in quadruplicate in the next commit. Moreover, replacing the frequently updated bs_sectors by an array assigned just once makes the code easier to understand. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- qemu-img.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)