diff mbox

[V7,09/14] block: move bdrv_snapshot_find() to block.c

Message ID 1361875228-15769-10-git-send-email-xiawenc@linux.vnet.ibm.com
State New
Headers show

Commit Message

Wayne Xia Feb. 26, 2013, 10:40 a.m. UTC 
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c               |   24 ++++++++++++++++++++++++
 include/block/block.h |    2 ++
 savevm.c              |   22 ----------------------
 3 files changed, 26 insertions(+), 22 deletions(-)

Comments

Markus Armbruster Feb. 27, 2013, 4:04 p.m. UTC | #1 
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:

> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c               |   24 ++++++++++++++++++++++++
>  include/block/block.h |    2 ++
>  savevm.c              |   22 ----------------------
>  3 files changed, 26 insertions(+), 22 deletions(-)

Perhaps savevm.c isn't the best home for snapshot stuff, but I don't
like it in block.c any better.  block.c is already unwieldy, and I'd
like us to try limiting it to core block stuff.

Kevin, Stefan, any ideas?
Kevin Wolf Feb. 27, 2013, 4:22 p.m. UTC | #2 
Am 27.02.2013 um 17:04 hat Markus Armbruster geschrieben:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 
> > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> >  block.c               |   24 ++++++++++++++++++++++++
> >  include/block/block.h |    2 ++
> >  savevm.c              |   22 ----------------------
> >  3 files changed, 26 insertions(+), 22 deletions(-)
> 
> Perhaps savevm.c isn't the best home for snapshot stuff, but I don't
> like it in block.c any better.  block.c is already unwieldy, and I'd
> like us to try limiting it to core block stuff.
> 
> Kevin, Stefan, any ideas?

Take some more snapshot related functions from block.c and move them to
block/snapshot.c?

Kevin
Wayne Xia March 1, 2013, 1:51 a.m. UTC | #3 
于 2013-2-28 0:22, Kevin Wolf 写道:
> Am 27.02.2013 um 17:04 hat Markus Armbruster geschrieben:
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>   block.c               |   24 ++++++++++++++++++++++++
>>>   include/block/block.h |    2 ++
>>>   savevm.c              |   22 ----------------------
>>>   3 files changed, 26 insertions(+), 22 deletions(-)
>>
>> Perhaps savevm.c isn't the best home for snapshot stuff, but I don't
>> like it in block.c any better.  block.c is already unwieldy, and I'd
>> like us to try limiting it to core block stuff.
>>
>> Kevin, Stefan, any ideas?
>
> Take some more snapshot related functions from block.c and move them to
> block/snapshot.c?
>
> Kevin
>
Hi, Stefan
   Do you also agree about adding new file block/snapshot.c?
Wayne Xia March 4, 2013, 3:20 a.m. UTC | #4 
于 2013-3-1 9:51, Wenchao Xia 写道:
> 于 2013-2-28 0:22, Kevin Wolf 写道:
>> Am 27.02.2013 um 17:04 hat Markus Armbruster geschrieben:
>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> ---
>>>>   block.c               |   24 ++++++++++++++++++++++++
>>>>   include/block/block.h |    2 ++
>>>>   savevm.c              |   22 ----------------------
>>>>   3 files changed, 26 insertions(+), 22 deletions(-)
>>>
>>> Perhaps savevm.c isn't the best home for snapshot stuff, but I don't
>>> like it in block.c any better.  block.c is already unwieldy, and I'd
>>> like us to try limiting it to core block stuff.
>>>
>>> Kevin, Stefan, any ideas?
>>
>> Take some more snapshot related functions from block.c and move them to
>> block/snapshot.c?
>>
>> Kevin
>>
> Hi, Stefan
>    Do you also agree about adding new file block/snapshot.c?
>
>
Hi,
   I am going to add include/block/snapshot.h and block/snapshot.c,
all internal snapshot function in block.c will be moved there.
external backing file related function will not be touched and
remain in block.c. This consume many effort and once done it will
be hard to revert back to form an incremental commit in this
serials, so before coding, please give your opinion if u don't agree
about it.
Stefan Hajnoczi March 4, 2013, 1:02 p.m. UTC | #5 
On Fri, Mar 01, 2013 at 09:51:53AM +0800, Wenchao Xia wrote: > 于 2013-2-28 0:22, Kevin Wolf 写道:
> >Am 27.02.2013 um 17:04 hat Markus Armbruster geschrieben:
> >>Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> >>
> >>>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>>Reviewed-by: Eric Blake <eblake@redhat.com>
> >>>---
> >>>  block.c               |   24 ++++++++++++++++++++++++
> >>>  include/block/block.h |    2 ++
> >>>  savevm.c              |   22 ----------------------
> >>>  3 files changed, 26 insertions(+), 22 deletions(-)
> >>
> >>Perhaps savevm.c isn't the best home for snapshot stuff, but I don't
> >>like it in block.c any better.  block.c is already unwieldy, and I'd
> >>like us to try limiting it to core block stuff.
> >>
> >>Kevin, Stefan, any ideas?
> >
> >Take some more snapshot related functions from block.c and move them to
> >block/snapshot.c?
> >
> >Kevin
> >
> Hi, Stefan
>   Do you also agree about adding new file block/snapshot.c?

Yes, I agree.

Stefan
Wayne Xia March 5, 2013, 7:11 a.m. UTC | #6 
于 2013-3-4 21:02, Stefan Hajnoczi 写道:
> On Fri, Mar 01, 2013 at 09:51:53AM +0800, Wenchao Xia wrote: > 于 2013-2-28 0:22, Kevin Wolf 写道:
>>> Am 27.02.2013 um 17:04 hat Markus Armbruster geschrieben:
>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>>>
>>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>> ---
>>>>>   block.c               |   24 ++++++++++++++++++++++++
>>>>>   include/block/block.h |    2 ++
>>>>>   savevm.c              |   22 ----------------------
>>>>>   3 files changed, 26 insertions(+), 22 deletions(-)
>>>>
>>>> Perhaps savevm.c isn't the best home for snapshot stuff, but I don't
>>>> like it in block.c any better.  block.c is already unwieldy, and I'd
>>>> like us to try limiting it to core block stuff.
>>>>
>>>> Kevin, Stefan, any ideas?
>>>
>>> Take some more snapshot related functions from block.c and move them to
>>> block/snapshot.c?
>>>
>>> Kevin
>>>
>> Hi, Stefan
>>    Do you also agree about adding new file block/snapshot.c?
>
> Yes, I agree.
>
> Stefan
>
great. Also there are some other functions such as image info
collecting, do you think new file block/misc.c is suitable
to store those functions there?
Stefan Hajnoczi March 5, 2013, 9:21 a.m. UTC | #7 
On Tue, Mar 05, 2013 at 03:11:48PM +0800, Wenchao Xia wrote:
> 于 2013-3-4 21:02, Stefan Hajnoczi 写道:
> >On Fri, Mar 01, 2013 at 09:51:53AM +0800, Wenchao Xia wrote: > 于 2013-2-28 0:22, Kevin Wolf 写道:
> >>>Am 27.02.2013 um 17:04 hat Markus Armbruster geschrieben:
> >>>>Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> >>>>
> >>>>>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>>>>Reviewed-by: Eric Blake <eblake@redhat.com>
> >>>>>---
> >>>>>  block.c               |   24 ++++++++++++++++++++++++
> >>>>>  include/block/block.h |    2 ++
> >>>>>  savevm.c              |   22 ----------------------
> >>>>>  3 files changed, 26 insertions(+), 22 deletions(-)
> >>>>
> >>>>Perhaps savevm.c isn't the best home for snapshot stuff, but I don't
> >>>>like it in block.c any better.  block.c is already unwieldy, and I'd
> >>>>like us to try limiting it to core block stuff.
> >>>>
> >>>>Kevin, Stefan, any ideas?
> >>>
> >>>Take some more snapshot related functions from block.c and move them to
> >>>block/snapshot.c?
> >>>
> >>>Kevin
> >>>
> >>Hi, Stefan
> >>   Do you also agree about adding new file block/snapshot.c?
> >
> >Yes, I agree.
> >
> >Stefan
> >
> great. Also there are some other functions such as image info
> collecting, do you think new file block/misc.c is suitable
> to store those functions there?

As discussed on IRC, it's fine by me.  If a better way to organize these
functions becomes clear in the future they can be moved.

Stefan
Kevin Wolf March 5, 2013, 10:08 a.m. UTC | #8 
Am 05.03.2013 um 10:21 hat Stefan Hajnoczi geschrieben:
> On Tue, Mar 05, 2013 at 03:11:48PM +0800, Wenchao Xia wrote:
> > 于 2013-3-4 21:02, Stefan Hajnoczi 写道:
> > >On Fri, Mar 01, 2013 at 09:51:53AM +0800, Wenchao Xia wrote: > 于 2013-2-28 0:22, Kevin Wolf 写道:
> > >>Hi, Stefan
> > >>   Do you also agree about adding new file block/snapshot.c?
> > >
> > >Yes, I agree.
> > >
> > >Stefan
> > >
> > great. Also there are some other functions such as image info
> > collecting, do you think new file block/misc.c is suitable
> > to store those functions there?
> 
> As discussed on IRC, it's fine by me.  If a better way to organize these
> functions becomes clear in the future they can be moved.

As also discussed on IRC, I'm not excited by having a block/misc.c. We
already have something for "everything block related that doesn't fit
elsewhere" and it's block.c. I couldn't tell if a function belong into
block.c or block/misc.c.

I suggested having a file that concentrates all function related to
QAPI, the monitor and JSON (including the qemu-img JSON output) and call
it something like block/qapi.c (I'm open for better suggestions). This
would at least make it clear if a function should be in there or not.

Kevin
Paolo Bonzini March 5, 2013, 10:52 a.m. UTC | #9 
Il 05/03/2013 11:08, Kevin Wolf ha scritto:
>> > 
>> > As discussed on IRC, it's fine by me.  If a better way to organize these
>> > functions becomes clear in the future they can be moved.
> As also discussed on IRC, I'm not excited by having a block/misc.c. We
> already have something for "everything block related that doesn't fit
> elsewhere" and it's block.c. I couldn't tell if a function belong into
> block.c or block/misc.c.
> 
> I suggested having a file that concentrates all function related to
> QAPI, the monitor and JSON (including the qemu-img JSON output) and call
> it something like block/qapi.c (I'm open for better suggestions). This
> would at least make it clear if a function should be in there or not.

I agree with Kevin.

Paolo
diff mbox

Patch

diff --git a/block.c b/block.c
index 3976167..a97c7ec 100644
--- a/block.c
+++ b/block.c
@@ -3421,6 +3421,30 @@  int bdrv_snapshot_load_tmp(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                       const char *name)
+{
+    QEMUSnapshotInfo *sn_tab, *sn;
+    int nb_sns, i, ret;
+
+    ret = -ENOENT;
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    if (nb_sns < 0) {
+        return ret;
+    }
+
+    for (i = 0; i < nb_sns; i++) {
+        sn = &sn_tab[i];
+        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
+            *sn_info = *sn;
+            ret = 0;
+            break;
+        }
+    }
+    g_free(sn_tab);
+    return ret;
+}
+
 /* backing_file can either be relative, or absolute, or a protocol.  If it is
  * relative, it must be relative to the chain.  So, passing in bs->filename
  * from a BDS as backing_file should not be done, as that may be relative to
diff --git a/include/block/block.h b/include/block/block.h
index a820184..d2b8bd1 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -343,6 +343,8 @@  int bdrv_snapshot_list(BlockDriverState *bs,
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
                            const char *snapshot_name);
 char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                       const char *name);
 
 char *get_human_readable_size(char *buf, int buf_size, int64_t size);
 int path_is_absolute(const char *path);
diff --git a/savevm.c b/savevm.c
index a8a53ef..f73ac32 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2060,28 +2060,6 @@  out:
     return ret;
 }
 
-static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
-                              const char *name)
-{
-    QEMUSnapshotInfo *sn_tab, *sn;
-    int nb_sns, i, ret;
-
-    ret = -ENOENT;
-    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
-    if (nb_sns < 0)
-        return ret;
-    for(i = 0; i < nb_sns; i++) {
-        sn = &sn_tab[i];
-        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
-            *sn_info = *sn;
-            ret = 0;
-            break;
-        }
-    }
-    g_free(sn_tab);
-    return ret;
-}
-
 /*
  * Deletes snapshots of a given name in all opened images.
  */