| Message ID | 1361875228-15769-5-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 02/26/2013 03:40 AM, Wenchao Xia wrote: > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> In v6, I complained that these functions didn't match the namespace expected in block.c. You replied: >> This patch is just for making review easier, those two functions will >> be deleted later so they do not have much meaning, renaming may bring >> confusion to reviewer. This information needs to be in the commit message, so that someone that happens on this commit during a git bisect will understand that several patches later gets rid of the problems. In other words, any commit that temporarily violates coding standards with plans to fix things up later should document that fact, rather than making reviewers chase down conversations in the mailing list. Personally, I think that renaming during code motion is not that confusing; but as I'm not the maintainer of this code, you might want to get Kevin or Stefan's opinion before spending your time to add churn that they don't find necessary. > --- > block.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++ > include/block/block.h | 4 ++ > qemu-img.c | 81 ------------------------------------------------- > 3 files changed, 85 insertions(+), 81 deletions(-) I have reviewed that this is a straight code motion patch, so if you have to respin the series for other reasons, and all that changes on this patch is adding a proper commit message, then I'm okay if you add: Reviewed-by: Eric Blake <eblake@redhat.com>
于 2013-2-26 23:59, Eric Blake 写道: > On 02/26/2013 03:40 AM, Wenchao Xia wrote: >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > > In v6, I complained that these functions didn't match the namespace > expected in block.c. You replied: > >>> This patch is just for making review easier, those two functions will >>> be deleted later so they do not have much meaning, renaming may bring >>> confusion to reviewer. > > This information needs to be in the commit message, so that someone that > happens on this commit during a git bisect will understand that several > patches later gets rid of the problems. In other words, any commit that > temporarily violates coding standards with plans to fix things up later > should document that fact, rather than making reviewers chase down > conversations in the mailing list. > > Personally, I think that renaming during code motion is not that > confusing; but as I'm not the maintainer of this code, you might want to > get Kevin or Stefan's opinion before spending your time to add churn > that they don't find necessary. > OK, I'll add it to the commit message about the patch. >> --- >> block.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++ >> include/block/block.h | 4 ++ >> qemu-img.c | 81 ------------------------------------------------- >> 3 files changed, 85 insertions(+), 81 deletions(-) > > I have reviewed that this is a straight code motion patch, so if you > have to respin the series for other reasons, and all that changes on > this patch is adding a proper commit message, then I'm okay if you add: > Reviewed-by: Eric Blake <eblake@redhat.com> >
On Wed, Feb 27, 2013 at 10:32:13AM +0800, Wenchao Xia wrote: > 于 2013-2-26 23:59, Eric Blake 写道: > >On 02/26/2013 03:40 AM, Wenchao Xia wrote: > >>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > > > >In v6, I complained that these functions didn't match the namespace > >expected in block.c. You replied: > > > >>> This patch is just for making review easier, those two functions will > >>>be deleted later so they do not have much meaning, renaming may bring > >>>confusion to reviewer. > > > >This information needs to be in the commit message, so that someone that > >happens on this commit during a git bisect will understand that several > >patches later gets rid of the problems. In other words, any commit that > >temporarily violates coding standards with plans to fix things up later > >should document that fact, rather than making reviewers chase down > >conversations in the mailing list. > > > >Personally, I think that renaming during code motion is not that > >confusing; but as I'm not the maintainer of this code, you might want to > >get Kevin or Stefan's opinion before spending your time to add churn > >that they don't find necessary. > > > OK, I'll add it to the commit message about the patch. I would go ahead and rename to "bdrv_..." in this patch. I was about to leave a comment asking you to rename it when I saw Eric's reply. Stefan
于 2013-2-28 23:24, Stefan Hajnoczi 写道: > On Wed, Feb 27, 2013 at 10:32:13AM +0800, Wenchao Xia wrote: >> 于 2013-2-26 23:59, Eric Blake 写道: >>> On 02/26/2013 03:40 AM, Wenchao Xia wrote: >>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >>> >>> In v6, I complained that these functions didn't match the namespace >>> expected in block.c. You replied: >>> >>>>> This patch is just for making review easier, those two functions will >>>>> be deleted later so they do not have much meaning, renaming may bring >>>>> confusion to reviewer. >>> >>> This information needs to be in the commit message, so that someone that >>> happens on this commit during a git bisect will understand that several >>> patches later gets rid of the problems. In other words, any commit that >>> temporarily violates coding standards with plans to fix things up later >>> should document that fact, rather than making reviewers chase down >>> conversations in the mailing list. >>> >>> Personally, I think that renaming during code motion is not that >>> confusing; but as I'm not the maintainer of this code, you might want to >>> get Kevin or Stefan's opinion before spending your time to add churn >>> that they don't find necessary. >>> >> OK, I'll add it to the commit message about the patch. > > I would go ahead and rename to "bdrv_..." in this patch. I was about to > leave a comment asking you to rename it when I saw Eric's reply. > > Stefan > OK, it will be renamed.
diff --git a/block.c b/block.c index 0ed8190..368b37c 100644 --- a/block.c +++ b/block.c @@ -2813,6 +2813,87 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, return 0; } +void collect_snapshots(BlockDriverState *bs , ImageInfo *info) +{ + int i, sn_count; + QEMUSnapshotInfo *sn_tab = NULL; + SnapshotInfoList *info_list, *cur_item = NULL; + sn_count = bdrv_snapshot_list(bs, &sn_tab); + + for (i = 0; i < sn_count; i++) { + info->has_snapshots = true; + info_list = g_new0(SnapshotInfoList, 1); + + info_list->value = g_new0(SnapshotInfo, 1); + info_list->value->id = g_strdup(sn_tab[i].id_str); + info_list->value->name = g_strdup(sn_tab[i].name); + info_list->value->vm_state_size = sn_tab[i].vm_state_size; + info_list->value->date_sec = sn_tab[i].date_sec; + info_list->value->date_nsec = sn_tab[i].date_nsec; + info_list->value->vm_clock_sec = sn_tab[i].vm_clock_nsec / 1000000000; + info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000; + + /* XXX: waiting for the qapi to support qemu-queue.h types */ + if (!cur_item) { + info->snapshots = cur_item = info_list; + } else { + cur_item->next = info_list; + cur_item = info_list; + } + + } + + g_free(sn_tab); +} + +void collect_image_info(BlockDriverState *bs, + ImageInfo *info, + const char *filename) +{ + uint64_t total_sectors; + char backing_filename[1024]; + char backing_filename2[1024]; + BlockDriverInfo bdi; + + bdrv_get_geometry(bs, &total_sectors); + + info->filename = g_strdup(filename); + info->format = g_strdup(bdrv_get_format_name(bs)); + info->virtual_size = total_sectors * 512; + info->actual_size = bdrv_get_allocated_file_size(bs); + info->has_actual_size = info->actual_size >= 0; + if (bdrv_is_encrypted(bs)) { + info->encrypted = true; + info->has_encrypted = true; + } + if (bdrv_get_info(bs, &bdi) >= 0) { + if (bdi.cluster_size != 0) { + info->cluster_size = bdi.cluster_size; + info->has_cluster_size = true; + } + info->dirty_flag = bdi.is_dirty; + info->has_dirty_flag = true; + } + bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename)); + if (backing_filename[0] != '\0') { + info->backing_filename = g_strdup(backing_filename); + info->has_backing_filename = true; + bdrv_get_full_backing_filename(bs, backing_filename2, + sizeof(backing_filename2)); + + if (strcmp(backing_filename, backing_filename2) != 0) { + info->full_backing_filename = + g_strdup(backing_filename2); + info->has_full_backing_filename = true; + } + + if (bs->backing_format[0]) { + info->backing_filename_format = g_strdup(bs->backing_format); + info->has_backing_filename_format = true; + } + } +} + BlockInfo *bdrv_query_info(BlockDriverState *bs) { BlockInfo *info = g_malloc0(sizeof(*info)); diff --git a/include/block/block.h b/include/block/block.h index 2dabb87..e6d915c 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -450,4 +450,8 @@ int bdrv_debug_breakpoint(BlockDriverState *bs, const char *event, int bdrv_debug_resume(BlockDriverState *bs, const char *tag); bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag); +void collect_snapshots(BlockDriverState *bs , ImageInfo *info); +void collect_image_info(BlockDriverState *bs, + ImageInfo *info, + const char *filename); #endif diff --git a/qemu-img.c b/qemu-img.c index 9dab48f..b650d17 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1134,39 +1134,6 @@ static void dump_json_image_info_list(ImageInfoList *list) QDECREF(str); } -static void collect_snapshots(BlockDriverState *bs , ImageInfo *info) -{ - int i, sn_count; - QEMUSnapshotInfo *sn_tab = NULL; - SnapshotInfoList *info_list, *cur_item = NULL; - sn_count = bdrv_snapshot_list(bs, &sn_tab); - - for (i = 0; i < sn_count; i++) { - info->has_snapshots = true; - info_list = g_new0(SnapshotInfoList, 1); - - info_list->value = g_new0(SnapshotInfo, 1); - info_list->value->id = g_strdup(sn_tab[i].id_str); - info_list->value->name = g_strdup(sn_tab[i].name); - info_list->value->vm_state_size = sn_tab[i].vm_state_size; - info_list->value->date_sec = sn_tab[i].date_sec; - info_list->value->date_nsec = sn_tab[i].date_nsec; - info_list->value->vm_clock_sec = sn_tab[i].vm_clock_nsec / 1000000000; - info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000; - - /* XXX: waiting for the qapi to support qemu-queue.h types */ - if (!cur_item) { - info->snapshots = cur_item = info_list; - } else { - cur_item->next = info_list; - cur_item = info_list; - } - - } - - g_free(sn_tab); -} - static void dump_json_image_info(ImageInfo *info) { Error *errp = NULL; @@ -1184,54 +1151,6 @@ static void dump_json_image_info(ImageInfo *info) QDECREF(str); } -static void collect_image_info(BlockDriverState *bs, - ImageInfo *info, - const char *filename) -{ - uint64_t total_sectors; - char backing_filename[1024]; - char backing_filename2[1024]; - BlockDriverInfo bdi; - - bdrv_get_geometry(bs, &total_sectors); - - info->filename = g_strdup(filename); - info->format = g_strdup(bdrv_get_format_name(bs)); - info->virtual_size = total_sectors * 512; - info->actual_size = bdrv_get_allocated_file_size(bs); - info->has_actual_size = info->actual_size >= 0; - if (bdrv_is_encrypted(bs)) { - info->encrypted = true; - info->has_encrypted = true; - } - if (bdrv_get_info(bs, &bdi) >= 0) { - if (bdi.cluster_size != 0) { - info->cluster_size = bdi.cluster_size; - info->has_cluster_size = true; - } - info->dirty_flag = bdi.is_dirty; - info->has_dirty_flag = true; - } - bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename)); - if (backing_filename[0] != '\0') { - info->backing_filename = g_strdup(backing_filename); - info->has_backing_filename = true; - bdrv_get_full_backing_filename(bs, backing_filename2, - sizeof(backing_filename2)); - - if (strcmp(backing_filename, backing_filename2) != 0) { - info->full_backing_filename = - g_strdup(backing_filename2); - info->has_full_backing_filename = true; - } - - if (bs->backing_format[0]) { - info->backing_filename_format = g_strdup(bs->backing_format); - info->has_backing_filename_format = true; - } - } -} - static void dump_human_image_info(ImageInfo *info) { char size_buf[128], dsize_buf[128];
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- block.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++ include/block/block.h | 4 ++ qemu-img.c | 81 ------------------------------------------------- 3 files changed, 85 insertions(+), 81 deletions(-)