From patchwork Thu Sep 6 11:11:05 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Henrique Barboza X-Patchwork-Id: 10590457 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id CF944921 for ; Thu, 6 Sep 2018 11:13:04 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C25FC287D4 for ; Thu, 6 Sep 2018 11:13:04 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B67B728898; Thu, 6 Sep 2018 11:13:04 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.8 required=2.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,FREEMAIL_FROM,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 01B8E287D4 for ; Thu, 6 Sep 2018 11:13:03 +0000 (UTC) Received: from localhost ([::1]:60818 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fxsDf-0008PC-6O for patchwork-qemu-devel@patchwork.kernel.org; Thu, 06 Sep 2018 07:13:03 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53423) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fxsCF-0007F3-HD for qemu-devel@nongnu.org; Thu, 06 Sep 2018 07:11:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fxsCB-0001nf-HB for qemu-devel@nongnu.org; Thu, 06 Sep 2018 07:11:33 -0400 Received: from mail-qt0-x22a.google.com ([2607:f8b0:400d:c0d::22a]:38485) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fxsCB-0001md-A9 for qemu-devel@nongnu.org; Thu, 06 Sep 2018 07:11:31 -0400 Received: by mail-qt0-x22a.google.com with SMTP id x7-v6so11708929qtk.5 for ; Thu, 06 Sep 2018 04:11:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=TiBSSKeSdoCfXwE5rt8NI5+zqvFidn2NKqyrgCn7tqo=; b=dmK8JYBxzhTMu0g0HeKnq/T6jMoiCcFjXmJgZZov1DY9B+HwRXQhC0GVhMcydm3gEM 2LRMk3OBrKa+35U83kixUimUH3kMpt3Z/B8gqqYTvuicgJwX5wfgw8kTJB26ERwsYQFR bMOuohDuesVTSyQqdm7hcRaSIpRiHAqAETNUcdTHz7HzBvpFcjJuW9swgj6ndwELW1dO Rtf9P4wy+UPHCuYpcUdvshn+VvGJg8opSjZ3rXOJmIAMHtYxHSgaupIyfN62tn28GHXW 9JaOE1eByNxCuAxBdu48i1FtqDkPfBJGCvFJhWSh2DUWk0tJddBuiu132sBX+Ofk48ow n6fg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=TiBSSKeSdoCfXwE5rt8NI5+zqvFidn2NKqyrgCn7tqo=; b=kHLyy5ZjnSF4DCE1zAD17+9jp5j6hj80nZZMHlgs1gQdGIySpfPENsfo0repGTOpH6 t3Wd/ue09y5S0ZsWUgSVUM9dOWRQnjLJWMcPq8U9x369MiBibM7PnwPpYjeP44KGwttC REcw8x0rUUSo1HKefpvbnwy/GpWmcBzFA3AnViJCMT8W9nqnuBWSplGZ+hmBH1wiB5kQ GbjCd5HxskNZjnbvg7D6pIh5aehwnq2eY7iylsWVXRiZN1vnD8rQL2tZmzSVL+juXb6z 7vVIPvL3lAKkJgT+mZuENZg5vGChX6BKwwVJcFYfoa7o3wvnEST/WDc6jjG9B8QAZ91b sMZg== X-Gm-Message-State: APzg51AU5l1DpR7uoWcAKTwpqteMDdSgZHvzQOvD0mpNZF9nZ10lYObl qtkQtLfqWnEjOxfqdcf6HgA6G/2Gyfg= X-Google-Smtp-Source: ANB0VdYM5duPwbJ3kXYO6OO44f4kVe5/Ldd34Q/IxJTFavUhiW7UH1T9s05KChttYzB+IcjN3sJHNw== X-Received: by 2002:ac8:35d9:: with SMTP id l25-v6mr1630233qtb.84.1536232289951; Thu, 06 Sep 2018 04:11:29 -0700 (PDT) Received: from localhost.localdomain ([2804:431:f701:4c04:a888:3548:cd61:60d5]) by smtp.gmail.com with ESMTPSA id x26-v6sm3180169qth.15.2018.09.06.04.11.27 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 06 Sep 2018 04:11:29 -0700 (PDT) From: Daniel Henrique Barboza To: qemu-devel@nongnu.org Date: Thu, 6 Sep 2018 08:11:05 -0300 Message-Id: <20180906111107.30684-2-danielhb413@gmail.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20180906111107.30684-1-danielhb413@gmail.com> References: <20180906111107.30684-1-danielhb413@gmail.com> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:400d:c0d::22a Subject: [Qemu-devel] [PATCH v2 1/3] block/snapshot.c: eliminate use of ID input in snapshot operations X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kwolf@redhat.com, Daniel Henrique Barboza , armbru@redhat.com, dgilbert@redhat.com, muriloo@linux.ibm.com, mreitz@redhat.com Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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 , 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. Libvirt does not care about this change either, as far as I've tested. 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: Daniel Henrique Barboza --- block/snapshot.c | 5 +++-- hmp-commands.hx | 24 ++++++++++++------------ 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/block/snapshot.c b/block/snapshot.c index 3218a542df..e371d2243d 100644 --- a/block/snapshot.c +++ b/block/snapshot.c @@ -63,7 +63,7 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, } for (i = 0; i < nb_sns; i++) { sn = &sn_tab[i]; - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { + if (!strcmp(sn->name, name)) { *sn_info = *sn; ret = 0; break; @@ -448,7 +448,8 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs, aio_context_acquire(ctx); if (bdrv_can_snapshot(bs) && bdrv_snapshot_find(bs, snapshot, name) >= 0) { - ret = bdrv_snapshot_delete_by_id_or_name(bs, name, err); + ret = bdrv_snapshot_delete(bs, snapshot->id_str, + snapshot->name, err); } aio_context_release(ctx); if (ret < 0) { diff --git a/hmp-commands.hx b/hmp-commands.hx index db0c681f74..5b4b8fbf2d 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -350,49 +350,49 @@ ETEXI { .name = "savevm", .args_type = "name:s?", - .params = "[tag|id]", - .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", + .params = "tag", + .help = "save a VM snapshot. If no tag is provided, a new snapshot is created", .cmd = hmp_savevm, }, STEXI -@item savevm [@var{tag}|@var{id}] +@item savevm @var{tag} @findex savevm Create a snapshot of the whole virtual machine. If @var{tag} is provided, it is used as human readable identifier. If there is already -a snapshot with the same tag or ID, it is replaced. More info at +a snapshot with the same tag, it is replaced. More info at @ref{vm_snapshots}. ETEXI { .name = "loadvm", .args_type = "name:s", - .params = "tag|id", - .help = "restore a VM snapshot from its tag or id", + .params = "tag", + .help = "restore a VM snapshot from its tag", .cmd = hmp_loadvm, .command_completion = loadvm_completion, }, STEXI -@item loadvm @var{tag}|@var{id} +@item loadvm @var{tag} @findex loadvm Set the whole virtual machine to the snapshot identified by the tag -@var{tag} or the unique snapshot ID @var{id}. +@var{tag}. ETEXI { .name = "delvm", .args_type = "name:s", - .params = "tag|id", - .help = "delete a VM snapshot from its tag or id", + .params = "tag", + .help = "delete a VM snapshot from its tag", .cmd = hmp_delvm, .command_completion = delvm_completion, }, STEXI -@item delvm @var{tag}|@var{id} +@item delvm @var{tag} @findex delvm -Delete the snapshot identified by @var{tag} or @var{id}. +Delete the snapshot identified by @var{tag}. ETEXI { From patchwork Thu Sep 6 11:11:06 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Henrique Barboza X-Patchwork-Id: 10590461 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 88056921 for ; Thu, 6 Sep 2018 11:14:53 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7B9852A6DD for ; Thu, 6 Sep 2018 11:14:53 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 6F6452A6FC; Thu, 6 Sep 2018 11:14:53 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.8 required=2.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,FREEMAIL_FROM,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id F0E102A6DD for ; Thu, 6 Sep 2018 11:14:52 +0000 (UTC) Received: from localhost ([::1]:60827 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fxsFQ-0001AN-AJ for patchwork-qemu-devel@patchwork.kernel.org; Thu, 06 Sep 2018 07:14:52 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53442) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fxsCJ-0007HZ-4s for qemu-devel@nongnu.org; Thu, 06 Sep 2018 07:11:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fxsCF-0001rX-Qs for qemu-devel@nongnu.org; Thu, 06 Sep 2018 07:11:39 -0400 Received: from mail-qt0-x241.google.com ([2607:f8b0:400d:c0d::241]:38529) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fxsCD-0001o1-Fl for qemu-devel@nongnu.org; Thu, 06 Sep 2018 07:11:34 -0400 Received: by mail-qt0-x241.google.com with SMTP id x7-v6so11709033qtk.5 for ; Thu, 06 Sep 2018 04:11:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=g4n3zGh+uAzd6bvFbTVTuVJpoFRTP3Y0DDwIsdwvgzY=; b=DJYj+VOpkdqAoXMZL+GsofH/RxQ0V4v0407UbofTJRAxC/xcJ5/RxiCzyEYEHLkmII TrywYkgxQnWniOb57WLcCH1S5/fXCAkyS6i8YdJ9wQNI1XlMDEK6I3IGTWLzg8f2eyd2 nJbIw88RBhvMM6Vu24Y//cXCyrhWyKEB7ZtUcGGbBjucltf81Z3wgi6mOCuYrka+0MV7 gziW644ZSktlaYDBO090qOsk7DBzV6X0cpVXWQIjtxSj2nJErSak8T4BOIIi+fmJEVvh nJ8tjSlHBlENJJjQNVAKkn1w/mi93WpTv2zOMqG05XdPry+YpoMgwJVOcw5UvOr6ngvs zTZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=g4n3zGh+uAzd6bvFbTVTuVJpoFRTP3Y0DDwIsdwvgzY=; b=kLA8TlBRauRNz7A7NXg6FmqWzrYDMlQzWjD3lWTgtw2AprXjysDbve0xF8/Y+Dw64W GzeR2/6v/y4hLjO7bNjFF5UZbs28yZFco4+wc5QxAZs88MMhuSJ8I9a+U83zVNTOHici sWi8tqTtuMBDdpuMVHGtp1igFzqM0JP0tgr+AFMjIxLJZbQvvEatRypzv8mYhwmk5uTt eMIL5lJO6X9WFY7z5CUDPa7yXXI1MPNKPzUn5udd3iHXkxp93tv/wiCe/QnN3YsOGuoA TucuEpf5t2tJncI3VDiUdToJZFXSlcHyA2N1XzPakNe0fTyV0qv6ba4D5aKV7xobRk5B ergQ== X-Gm-Message-State: APzg51CLs/TUVRUqJkqFB+UrjxIzAbBFMAjMK3MXOT1G3j/9mpgBSjNL ySquIZ3VQ3IfNQ5flwjkRumeuLqW7zU= X-Google-Smtp-Source: ANB0Vdah/oC68p0aiAqsjeZIRi3me+O2xYJNgYULWCrPni0RcHPfE8WqrpZv9A6LFep19c54LykpQg== X-Received: by 2002:ac8:39e1:: with SMTP id v88-v6mr1542778qte.273.1536232292143; Thu, 06 Sep 2018 04:11:32 -0700 (PDT) Received: from localhost.localdomain ([2804:431:f701:4c04:a888:3548:cd61:60d5]) by smtp.gmail.com with ESMTPSA id x26-v6sm3180169qth.15.2018.09.06.04.11.30 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 06 Sep 2018 04:11:31 -0700 (PDT) From: Daniel Henrique Barboza To: qemu-devel@nongnu.org Date: Thu, 6 Sep 2018 08:11:06 -0300 Message-Id: <20180906111107.30684-3-danielhb413@gmail.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20180906111107.30684-1-danielhb413@gmail.com> References: <20180906111107.30684-1-danielhb413@gmail.com> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:400d:c0d::241 Subject: [Qemu-devel] [PATCH v2 2/3] block/snapshot: remove bdrv_snapshot_delete_by_id_or_name X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kwolf@redhat.com, Daniel Henrique Barboza , armbru@redhat.com, dgilbert@redhat.com, muriloo@linux.ibm.com, mreitz@redhat.com Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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: Murilo Opsfelder Araujo Signed-off-by: Daniel Henrique Barboza --- block/snapshot.c | 20 -------------------- include/block/snapshot.h | 3 --- qemu-img.c | 15 +++++++++++---- 3 files changed, 11 insertions(+), 27 deletions(-) diff --git a/block/snapshot.c b/block/snapshot.c index e371d2243d..f2f48f926a 100644 --- a/block/snapshot.c +++ b/block/snapshot.c @@ -301,26 +301,6 @@ int bdrv_snapshot_delete(BlockDriverState *bs, return ret; } -int bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs, - const char *id_or_name, - Error **errp) -{ - int ret; - Error *local_err = NULL; - - ret = bdrv_snapshot_delete(bs, id_or_name, NULL, &local_err); - if (ret == -ENOENT || ret == -EINVAL) { - error_free(local_err); - local_err = NULL; - ret = bdrv_snapshot_delete(bs, NULL, id_or_name, &local_err); - } - - if (ret < 0) { - error_propagate(errp, local_err); - } - return ret; -} - int bdrv_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_info) { diff --git a/include/block/snapshot.h b/include/block/snapshot.h index f73d1094af..b5d5084a12 100644 --- a/include/block/snapshot.h +++ b/include/block/snapshot.h @@ -61,9 +61,6 @@ int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id, const char *name, Error **errp); -int bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs, - const char *id_or_name, - Error **errp); int bdrv_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_info); int bdrv_snapshot_load_tmp(BlockDriverState *bs, diff --git a/qemu-img.c b/qemu-img.c index b12f4cd19b..62bb35f1f0 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -3121,11 +3121,18 @@ static int img_snapshot(int argc, char **argv) break; case SNAPSHOT_DELETE: - bdrv_snapshot_delete_by_id_or_name(bs, snapshot_name, &err); - if (err) { - error_reportf_err(err, "Could not delete snapshot '%s': ", - snapshot_name); + ret = bdrv_snapshot_find(bs, &sn, snapshot_name); + if (ret < 0) { + error_report("Could not delete snapshot '%s': snapshot not " + "found", snapshot_name); ret = 1; + } else { + ret = bdrv_snapshot_delete(bs, sn.id_str, sn.name, &err); + if (ret < 0) { + error_reportf_err(err, "Could not delete snapshot '%s': ", + snapshot_name); + ret = 1; + } } break; } From patchwork Thu Sep 6 11:11:07 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Henrique Barboza X-Patchwork-Id: 10590459 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 6A3A614E0 for ; Thu, 6 Sep 2018 11:13:12 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5A948287D4 for ; Thu, 6 Sep 2018 11:13:12 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4DFD228898; Thu, 6 Sep 2018 11:13:12 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.8 required=2.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,FREEMAIL_FROM,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id E6949287EF for ; Thu, 6 Sep 2018 11:13:11 +0000 (UTC) Received: from localhost ([::1]:60819 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fxsDn-00005Y-1Z for patchwork-qemu-devel@patchwork.kernel.org; Thu, 06 Sep 2018 07:13:11 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53457) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fxsCN-0007Ko-AU for qemu-devel@nongnu.org; Thu, 06 Sep 2018 07:11:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fxsCJ-0001tN-5Y for qemu-devel@nongnu.org; Thu, 06 Sep 2018 07:11:40 -0400 Received: from mail-qt0-x243.google.com ([2607:f8b0:400d:c0d::243]:44871) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fxsCF-0001pz-Qf for qemu-devel@nongnu.org; Thu, 06 Sep 2018 07:11:37 -0400 Received: by mail-qt0-x243.google.com with SMTP id k38-v6so11674602qtk.11 for ; Thu, 06 Sep 2018 04:11:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=bT2r8V74033xk+DGVr172Gu8HP1ZinVaO+w5cMZosb0=; b=p6fl9eVh48qvZpt6W95MoIpEkgFv2053JZQRLFKWIRJK9FXwkug5CJDJ3vS9617Scf NvuqKvzb0U4+qGExXOS28cw+xELbb0vVbrzfwZQvWcGSsdkBUKP8rj2eNc8APCK9Ikin +xHzuvMVOHwYFuujkAFu60UeCo1x+ULO1wADwDH8p9aGtdhGyOq67iEWuJchi9pjlfM1 68uA1bEvORS3J3Y8+WTPd8DHRN5gdsheB5AD02/RF63d8Bm9NLP489SYFboZVLRu5uwV 1h5CESpmTfLF2RGxi2ifp697wxl3SFaas2Z1Hnx7nG3ECSHm1DnO6n28EUZgMy0Ck9n7 JTjw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=bT2r8V74033xk+DGVr172Gu8HP1ZinVaO+w5cMZosb0=; b=FjLoYeY9GdQ82caC0xL5tmau/nu1jZ9g9xbYhfuWTyrAsDirWT/Op8LfLS0olAJwKl iGYQ661H5EA9yfkKWOkmxC/RGP1IyY2uWWUCY9WGJGCwT2XhyZTc7GsIKgfFpHare+HQ KmlTAnHtAHr3lVkc73wgzAjr95+JbgTh9l2TesATqlj3HIDKOy+zMyahPjmtVu+NeoAs c+ar4BCTcxgFQWVkNvErimkIkjEK02uHEmu3joFbpxSJNYU4T7DEtpy0mfx5F7AuMwZ/ pcZz58VPZ4UaSCaVUpxuaGwMyGYy40l04l+1oFpdS+Nem4NMMgAW4ZVrwkv47L1A2slP LdKw== X-Gm-Message-State: APzg51DD6cmWX3bf+oFYVTp9FAfNqRJG/Buyj0/iaNrH6x2qjzv9Y8rj m2TgbGOOMr1ivFzKuNk3aJxByZMyqZA= X-Google-Smtp-Source: ANB0VdYgOhRlVcA4bR/jXTaw8SFMzQOtaOKsSL9xd4juzBkZ1fVHFK3x9uUlZDmJ4SiTFppjynBOOw== X-Received: by 2002:a0c:f643:: with SMTP id s3-v6mr1545231qvm.131.1536232294675; Thu, 06 Sep 2018 04:11:34 -0700 (PDT) Received: from localhost.localdomain ([2804:431:f701:4c04:a888:3548:cd61:60d5]) by smtp.gmail.com with ESMTPSA id x26-v6sm3180169qth.15.2018.09.06.04.11.32 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 06 Sep 2018 04:11:34 -0700 (PDT) From: Daniel Henrique Barboza To: qemu-devel@nongnu.org Date: Thu, 6 Sep 2018 08:11:07 -0300 Message-Id: <20180906111107.30684-4-danielhb413@gmail.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20180906111107.30684-1-danielhb413@gmail.com> References: <20180906111107.30684-1-danielhb413@gmail.com> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:400d:c0d::243 Subject: [Qemu-devel] [PATCH v2 3/3] qcow2-snapshot: remove redundant find_snapshot_by_id_and_name call X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kwolf@redhat.com, Daniel Henrique Barboza , armbru@redhat.com, dgilbert@redhat.com, muriloo@linux.ibm.com, mreitz@redhat.com Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP In qcow2_snapshot_create there is the following code block: /* Generate an ID */ find_new_snapshot_id(bs, sn_info->id_str, sizeof(sn_info->id_str)); /* Check that the ID is unique */ if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) { return -EEXIST; } find_new_snapshot_id cycles through all snapshots, getting the id_str as an unsigned long int, calculating the max id_max value of all the existing id_strs and writing in the id_str pointer id_max + 1: for(i = 0; i < s->nb_snapshots; i++) { sn = s->snapshots + i; id = strtoul(sn->id_str, NULL, 10); if (id > id_max) id_max = id; } snprintf(id_str, id_str_size, "%lu", id_max + 1); Here, sn_info->id_str will have the unique value id_max + 1. Right after that, find_snapshot_by_id_and_name is called with id = sn_info->id_str and name = NULL. This will cause the function to execute the following: } else if (id) { for (i = 0; i < s->nb_snapshots; i++) { if (!strcmp(s->snapshots[i].id_str, id)) { return i; } } } In short, we're searching the existing snapshots to see if sn_info->id_str matches any existing id, right after we set in the previous line a sn_info->id_str value that is already unique. The first code block goes way back to commit 585f8587ad, a 2006 commit from Fabrice Bellard that simply says "new qcow2 disk image format". No more info is provided about this logic in any subsequent commits that moved this code block around. I can't say about the original design, but the current logic is redundant. bdrv_snapshot_create is called in aio_context lock, forbidding any concurrent call to accidentally create a new snapshot between the find_new_snapshot_id and find_snapshot_by_id_and_name calls. What we're ending up doing is to cycle through the snapshots two times for no viable reason. This patch eliminates the redundancy by removing the 'id is unique' check that calls find_snapshot_by_id_and_name. Signed-off-by: Daniel Henrique Barboza --- block/qcow2-snapshot.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index bb6a5b7516..20e8472191 100644 --- a/block/qcow2-snapshot.c +++ b/block/qcow2-snapshot.c @@ -358,11 +358,6 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) /* Generate an ID */ find_new_snapshot_id(bs, sn_info->id_str, sizeof(sn_info->id_str)); - /* Check that the ID is unique */ - if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) { - return -EEXIST; - } - /* Populate sn with passed data */ sn->id_str = g_strdup(sn_info->id_str); sn->name = g_strdup(sn_info->name);