1. 25 2月, 2019 2 次提交
    • D
      block/snapshot: remove bdrv_snapshot_delete_by_id_or_name · 8c04093c
      Daniel Henrique Barboza 提交于
      After the previous patch, the only instance of this function left
      is inside qemu-img.c.
      
      qemu-img is using it inside the 'img_snapshot' function to delete
      snapshots in the SNAPSHOT_DELETE case, based on a "snapshot_name"
      string that refers to the tag, not ID, of the QEMUSnapshotInfo struct.
      This can be verified by checking the SNAPSHOT_CREATE case that
      comes shortly before SNAPSHOT_DELETE. In that case, the same
      "snapshot_name" variable is being strcpy to the 'name' field
      of the QEMUSnapshotInfo struct sn:
      
      pstrcpy(sn.name, sizeof(sn.name), snapshot_name);
      
      Based on that, it is unlikely that "snapshot_name" might contain
      an "id" in SNAPSHOT_DELETE.
      
      This patch changes SNAPSHOT_DELETE to use snapshot_find() and
      snapshot_delete() instead of bdrv_snapshot_delete_by_id_or_name.
      After that, there is no instances left of bdrv_snapshot_delete_by_id_or_name
      in the code, so it is safe to remove it entirely.
      Suggested-by: NMurilo Opsfelder Araujo <muriloo@linux.ibm.com>
      Signed-off-by: NDaniel Henrique Barboza <danielhb413@gmail.com>
      Signed-off-by: NKevin Wolf <kwolf@redhat.com>
      8c04093c
    • D
      block/snapshot.c: eliminate use of ID input in snapshot operations · 6ca08045
      Daniel Henrique Barboza 提交于
      At this moment, QEMU attempts to create/load/delete snapshots
      by using either an ID (id_str) or a name. The problem is that the code
      isn't consistent of whether the entered argument is an ID or a name,
      causing unexpected behaviors.
      
      For example, when creating snapshots via savevm <arg>, what happens is that
      "arg" is treated as both name and id_str. In a guest without snapshots, create
      a single snapshot via savevm:
      
      (qemu) savevm 0
      (qemu) info snapshots
      List of snapshots present on all disks:
      ID        TAG                 VM SIZE                DATE       VM CLOCK
      --        0                      741M 2018-07-31 13:39:56   00:41:25.313
      
      A snapshot with name "0" is created. ID is hidden from the user, but the
      ID is a non-zero integer that starts at "1". Thus, this snapshot has
      id_str=1, TAG="0". Creating a second snapshot with arg = 1, the first one
      is deleted:
      
      (qemu) savevm 1
      (qemu) info snapshots
      List of snapshots present on all disks:
      ID        TAG                 VM SIZE                DATE       VM CLOCK
      --        1                      741M 2018-07-31 13:42:14   00:41:55.252
      
      What happened?
      
      - when creating the second snapshot, a verification is done inside
      bdrv_all_delete_snapshot to delete any existing snapshots that matches an
      string argument. Here, the code calls bdrv_all_delete_snapshot("1", ...);
      
      - bdrv_all_delete_snapshot calls bdrv_snapshot_find(..., "1") for each
      BlockDriverState of the guest. And this is where things goes tilting:
      bdrv_snapshot_find does a search by both id_str and name. It finds
      out that there is a snapshot that has id_str = 1, stores a reference
      to the snapshot in the sn_info pointer and then returns match found;
      
      - since a match was found, a call to bdrv_snapshot_delete_by_id_or_name() is
      made. This function ignores the pointer written by bdrv_snapshot_find. Instead,
      it deletes the snapshot using bdrv_snapshot_delete() calling it first with
      id_str = 1. If it fails to delete, then it calls it again with name = 1.
      
      - after all that, QEMU creates the new snapshot, that has id_str = 1 and
      name = 1. The user is left wondering that happened with the first snapshot
      created. Similar bugs can be triggered when using loadvm and delvm.
      
      Before contemplating discarding the use of ID input in these operations,
      I've searched the code of what would be the implications. My findings
      are:
      
      - the RBD and Sheepdog drivers don't care. Both uses the 'name' field as
      key in their logic, making id_str = name when appropriate.
      replay-snapshot.c does not make any special use of id_str;
      
      - qcow2 uses id_str as an unique identifier but it is automatically
      calculated, not being influenced by user input. Other than that, there are
      no distinguish operations made only with id_str;
      
      - in blockdev.c, the delete operation uses a match of both id_str AND
      name. Given that id_str is either a copy of 'name' or auto-generated,
      we're fine here.
      
      This gives motivation to not consider ID as a valid user input in HMP
      commands - sticking with 'name' input only is more consistent. To
      accomplish that, the following changes were made in this patch:
      
      - bdrv_snapshot_find() does not match for id_str anymore, only 'name'. The
      function is called in save_snapshot(), load_snapshot(), bdrv_all_delete_snapshot()
      and bdrv_all_find_snapshot(). This change makes the search function more
      predictable and does not change the behavior of any underlying code that uses
      these affected functions, which are related to HMP (which is fine) and the
      main loop inside vl.c (which doesn't care about it anyways);
      
      - bdrv_all_delete_snapshot() does not call bdrv_snapshot_delete_by_id_or_name
      anymore. Instead, it uses the pointer returned by bdrv_snapshot_find to
      erase the snapshot with the exact match of id_str an name. This function
      is called in save_snapshot and hmp_delvm, thus this change  produces the
      intended effect;
      
      - documentation changes to reflect the new behavior. I consider this to
      be an API fix instead of an API change - the user was already creating
      snapshots using 'name', but now he/she will also enjoy a consistent
      behavior.
      
      Ideally we would get rid of the id_str field entirely, but this would have
      repercussions on existing snapshots. Another day perhaps.
      Signed-off-by: NDaniel Henrique Barboza <danielhb413@gmail.com>
      Acked-by: NDr. David Alan Gilbert <dgilbert@redhat.com>
      Signed-off-by: NKevin Wolf <kwolf@redhat.com>
      6ca08045
  2. 15 8月, 2018 1 次提交
  3. 15 6月, 2018 1 次提交
    • M
      block: Add block-specific QDict header · 609f45ea
      Max Reitz 提交于
      There are numerous QDict functions that have been introduced for and are
      used only by the block layer.  Move their declarations into an own
      header file to reflect that.
      
      While qdict_extract_subqdict() is in fact used outside of the block
      layer (in util/qemu-config.c), it is still a function related very
      closely to how the block layer works with nested QDicts, namely by
      sometimes flattening them.  Therefore, its declaration is put into this
      header as well and util/qemu-config.c includes it with a comment stating
      exactly which function it needs.
      Suggested-by: NMarkus Armbruster <armbru@redhat.com>
      Signed-off-by: NMax Reitz <mreitz@redhat.com>
      Message-Id: <20180509165530.29561-7-mreitz@redhat.com>
      [Copyright note tweaked, superfluous includes dropped]
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      Reviewed-by: NKevin Wolf <kwolf@redhat.com>
      Signed-off-by: NKevin Wolf <kwolf@redhat.com>
      609f45ea
  4. 04 5月, 2018 1 次提交
  5. 09 2月, 2018 2 次提交
  6. 21 11月, 2017 3 次提交
  7. 18 11月, 2017 1 次提交
    • M
      block: Make bdrv_next() keep strong references · 5e003f17
      Max Reitz 提交于
      On one hand, it is a good idea for bdrv_next() to return a strong
      reference because ideally nearly every pointer should be refcounted.
      This fixes intermittent failure of iotest 194.
      
      On the other, it is absolutely necessary for bdrv_next() itself to keep
      a strong reference to both the BB (in its first phase) and the BDS (at
      least in the second phase) because when called the next time, it will
      dereference those objects to get a link to the next one.  Therefore, it
      needs these objects to stay around until then.  Just storing the pointer
      to the next in the iterator is not really viable because that pointer
      might become invalid as well.
      
      Both arguments taken together means we should probably just invoke
      bdrv_ref() and blk_ref() in bdrv_next().  This means we have to assert
      that bdrv_next() is always called from the main loop, but that was
      probably necessary already before this patch and judging from the
      callers, it also looks to actually be the case.
      
      Keeping these strong references means however that callers need to give
      them up if they decide to abort the iteration early.  They can do so
      through the new bdrv_next_cleanup() function.
      Suggested-by: NKevin Wolf <kwolf@redhat.com>
      Signed-off-by: NMax Reitz <mreitz@redhat.com>
      Message-id: 20171110172545.32609-1-mreitz@redhat.com
      Reviewed-by: NStefan Hajnoczi <stefanha@redhat.com>
      Signed-off-by: NMax Reitz <mreitz@redhat.com>
      5e003f17
  8. 15 11月, 2017 1 次提交
  9. 09 5月, 2017 1 次提交
  10. 11 4月, 2017 1 次提交
    • D
      block: pass the right options for BlockDriver.bdrv_open() · 7a9e5119
      Dong Jia Shi 提交于
      raw_open() expects the caller always passing in the right actual
      @options parameter. But when trying to applying snapshot on a RBD
      image, bdrv_snapshot_goto() calls raw_open() (by calling the
      bdrv_open callback on the BlockDriver) with a NULL @options, and
      that will result in a Segmentation fault.
      
      For the other non-raw format drivers, it also makes sense to passing
      in the actual options, althought they don't trigger the problem so
      far.
      
      Let's prepare a @options by adding the "file" key-value pair to a
      copy of the actual options that were given for the node (i.e.
      bs->options), and pass it to the callback.
      
      BlockDriver.bdrv_open() expects bs->file to be NULL and just
      overwrites it with the result from bdrv_open_child(). That means we
      should actually make sure it's NULL because otherwise the child BDS
      will have a reference count that is 1 too high. So we unconditionally
      invoke bdrv_unref_child() before calling BlockDriver.bdrv_open(), and
      we wrap everything in bdrv_ref()/bdrv_unref() so the BDS isn't
      deleted in the meantime.
      Suggested-by: NMax Reitz <mreitz@redhat.com>
      Signed-off-by: NDong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
      Message-id: 20170405091909.36357-2-bjsdjshi@linux.vnet.ibm.com
      Signed-off-by: NMax Reitz <mreitz@redhat.com>
      7a9e5119
  11. 20 6月, 2016 1 次提交
  12. 08 6月, 2016 1 次提交
  13. 26 5月, 2016 1 次提交
    • K
      block: Fix bdrv_next() memory leak · 88be7b4b
      Kevin Wolf 提交于
      The bdrv_next() users all leaked the BdrvNextIterator after completing
      the iteration. Simply changing bdrv_next() to free the iterator before
      returning NULL at the end of list doesn't work because some callers exit
      the loop before looking at all BDSes.
      
      This patch moves the BdrvNextIterator from the heap to the stack of
      the caller and switches to a bdrv_first()/bdrv_next() interface for
      initialising the iterator.
      Signed-off-by: NKevin Wolf <kwolf@redhat.com>
      Reviewed-by: NFam Zheng <famz@redhat.com>
      88be7b4b
  14. 19 5月, 2016 1 次提交
  15. 23 3月, 2016 1 次提交
    • M
      include/qemu/osdep.h: Don't include qapi/error.h · da34e65c
      Markus Armbruster 提交于
      Commit 57cb38b3 included qapi/error.h into qemu/osdep.h to get the
      Error typedef.  Since then, we've moved to include qemu/osdep.h
      everywhere.  Its file comment explains: "To avoid getting into
      possible circular include dependencies, this file should not include
      any other QEMU headers, with the exceptions of config-host.h,
      compiler.h, os-posix.h and os-win32.h, all of which are doing a
      similar job to this file and are under similar constraints."
      qapi/error.h doesn't do a similar job, and it doesn't adhere to
      similar constraints: it includes qapi-types.h.  That's in excess of
      100KiB of crap most .c files don't actually need.
      
      Add the typedef to qemu/typedefs.h, and include that instead of
      qapi/error.h.  Include qapi/error.h in .c files that need it and don't
      get it now.  Include qapi-types.h in qom/object.h for uint16List.
      
      Update scripts/clean-includes accordingly.  Update it further to match
      reality: replace config.h by config-target.h, add sysemu/os-posix.h,
      sysemu/os-win32.h.  Update the list of includes in the qemu/osdep.h
      comment quoted above similarly.
      
      This reduces the number of objects depending on qapi/error.h from "all
      of them" to less than a third.  Unfortunately, the number depending on
      qapi-types.h shrinks only a little.  More work is needed for that one.
      Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
      [Fix compilation without the spice devel packages. - Paolo]
      Signed-off-by: NPaolo Bonzini <pbonzini@redhat.com>
      da34e65c
  16. 20 1月, 2016 1 次提交
  17. 18 12月, 2015 1 次提交
  18. 19 11月, 2015 7 次提交
  19. 16 10月, 2015 1 次提交
  20. 07 7月, 2015 1 次提交
  21. 23 6月, 2015 2 次提交
  22. 28 4月, 2015 1 次提交
  23. 03 11月, 2014 1 次提交
  24. 18 2月, 2014 1 次提交
  25. 04 12月, 2013 2 次提交
  26. 12 9月, 2013 3 次提交