diff mbox

[04/15] sheepdog: Mark sd_snapshot_delete() lossage FIXME

Message ID 1488491046-2549-5-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster March 2, 2017, 9:43 p.m. UTC 
sd_snapshot_delete() should delete the snapshot whose ID matches
@snapshot_id and whose name matches @name.  But that's not what it
does.  If @snapshot_id is a valid ID, it deletes the snapshot with
that ID, else it deletes the snapshot with that name.  It doesn't use
@name at all.  Add suitable FIXME comments, so someone who actually
knows Sheepdog can fix it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/sheepdog.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Eric Blake March 2, 2017, 11:18 p.m. UTC | #1 
On 03/02/2017 03:43 PM, Markus Armbruster wrote:
> sd_snapshot_delete() should delete the snapshot whose ID matches
> @snapshot_id and whose name matches @name.  But that's not what it
> does.  If @snapshot_id is a valid ID, it deletes the snapshot with
> that ID, else it deletes the snapshot with that name.  It doesn't use
> @name at all.  Add suitable FIXME comments, so someone who actually
> knows Sheepdog can fix it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/sheepdog.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

If a sheepdog maintainer wants to rewrite this patch to avoid FIXME, I'm
fine with that. But in the meantime, this doesn't change behavior, so
it's safe for freeze.
Reviewed-by: Eric Blake <eblake@redhat.com>
diff mbox

Patch

diff --git a/block/sheepdog.c b/block/sheepdog.c
index abfaa95..5554f47 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2457,6 +2457,10 @@  static int sd_snapshot_delete(BlockDriverState *bs,
                               const char *name,
                               Error **errp)
 {
+    /*
+     * FIXME should delete the snapshot matching both @snapshot_id and
+     * @name, but @name not used here
+     */
     unsigned long snap_id = 0;
     char snap_tag[SD_MAX_VDI_TAG_LEN];
     int fd, ret;
@@ -2481,6 +2485,11 @@  static int sd_snapshot_delete(BlockDriverState *bs,
     pstrcpy(buf, SD_MAX_VDI_LEN, s->name);
     ret = qemu_strtoul(snapshot_id, NULL, 10, &snap_id);
     if (ret || snap_id > UINT32_MAX) {
+        /*
+         * FIXME Since qemu_strtoul() returns -EINVAL when
+         * @snapshot_id is null, @snapshot_id is mandatory.  Correct
+         * would be to require at least one of @snapshot_id and @name.
+         */
         error_setg(errp, "Invalid snapshot ID: %s",
                          snapshot_id ? snapshot_id : "<null>");
         return -EINVAL;
@@ -2489,6 +2498,7 @@  static int sd_snapshot_delete(BlockDriverState *bs,
     if (snap_id) {
         hdr.snapid = (uint32_t) snap_id;
     } else {
+        /* FIXME I suspect we should use @name here */
         pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id);
         pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag);
     }