From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.1 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 83780C433ED for ; Mon, 3 May 2021 09:47:32 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C4C30610A5 for ; Mon, 3 May 2021 09:47:31 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C4C30610A5 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:46104 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ldVAo-000778-NG for qemu-devel@archiver.kernel.org; Mon, 03 May 2021 05:47:30 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:38788) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ldV8h-00069B-MR for qemu-devel@nongnu.org; Mon, 03 May 2021 05:45:20 -0400 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:46255) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ldV8d-0007NM-Ck for qemu-devel@nongnu.org; Mon, 03 May 2021 05:45:19 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1620035112; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=2y1fmYzLLad+sLUpjfTNG4f+vPZYbS5fiZD/yikpHis=; b=MsLVfw8k+GG0DZhZuK0lagWKGM9KF5GR24I0j/t66sWmNwql8Bo6LBZRChTtId4HJY51lv zzfEyLHCMv6GEDmq10By5ZKUIj0Sxa1Vtp00IobAvId1bBrinLzIck0XjkBC4HIAUBFVsc XwL9Irm/6mMvnAys0hE80ykMyzav2/E= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-21-Vj5GMMwVMxC0fgrG1QEbhw-1; Mon, 03 May 2021 05:45:11 -0400 X-MC-Unique: Vj5GMMwVMxC0fgrG1QEbhw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 522C3801106; Mon, 3 May 2021 09:45:10 +0000 (UTC) Received: from dresden.str.redhat.com (ovpn-114-107.ams2.redhat.com [10.36.114.107]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1F40A19D7D; Mon, 3 May 2021 09:45:08 +0000 (UTC) Subject: Re: [PULL 37/64] block/snapshot: Fix fallback To: Kevin Wolf , Peter Maydell References: <20200907110936.261684-1-kwolf@redhat.com> <20200907110936.261684-38-kwolf@redhat.com> From: Max Reitz Message-ID: <1b1f6fe6-1a0e-61f8-1197-d26667a3d6fc@redhat.com> Date: Mon, 3 May 2021 11:45:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=mreitz@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Received-SPF: pass client-ip=216.205.24.124; envelope-from=mreitz@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -34 X-Spam_score: -3.5 X-Spam_bar: --- X-Spam_report: (-3.5 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.697, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, NICE_REPLY_A=-0.001, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H4=-0.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: QEMU Developers , Qemu-block Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On 03.05.21 11:40, Kevin Wolf wrote: > Am 01.05.2021 um 00:30 hat Peter Maydell geschrieben: >> On Mon, 7 Sept 2020 at 12:11, Kevin Wolf wrote: >>> >>> From: Max Reitz >>> >>> If the top node's driver does not provide snapshot functionality and we >>> want to fall back to a node down the chain, we need to snapshot all >>> non-COW children. For simplicity's sake, just do not fall back if there >>> is more than one such child. Furthermore, we really only can fall back >>> to bs->file and bs->backing, because bdrv_snapshot_goto() has to modify >>> the child link (notably, set it to NULL). >>> >>> Suggested-by: Vladimir Sementsov-Ogievskiy >>> Signed-off-by: Max Reitz >>> Reviewed-by: Andrey Shinkevich >>> Reviewed-by: Kevin Wolf >> >> Hi; Coverity thinks it's found a problem with this code >> (CID 1452774): > > Cc: Max as the patch author Yes, I’m writing a patch to add a comment. >>> @@ -205,39 +258,46 @@ int bdrv_snapshot_goto(BlockDriverState *bs, >>> return ret; >>> } >>> >>> - if (bs->file) { >>> - BlockDriverState *file; >>> - QDict *options = qdict_clone_shallow(bs->options); >>> + fallback_ptr = bdrv_snapshot_fallback_ptr(bs); >>> + if (fallback_ptr) { >>> + QDict *options; >>> QDict *file_options; >>> Error *local_err = NULL; >>> + BlockDriverState *fallback_bs = (*fallback_ptr)->bs; >>> + char *subqdict_prefix = g_strdup_printf("%s.", (*fallback_ptr)->name); >>> + >>> + options = qdict_clone_shallow(bs->options); >>> >>> - file = bs->file->bs; >>> /* Prevent it from getting deleted when detached from bs */ >>> - bdrv_ref(file); >>> + bdrv_ref(fallback_bs); >>> >>> - qdict_extract_subqdict(options, &file_options, "file."); >>> + qdict_extract_subqdict(options, &file_options, subqdict_prefix); >>> qobject_unref(file_options); >>> - qdict_put_str(options, "file", bdrv_get_node_name(file)); >>> + g_free(subqdict_prefix); >>> + >>> + qdict_put_str(options, (*fallback_ptr)->name, >>> + bdrv_get_node_name(fallback_bs)); >>> >>> if (drv->bdrv_close) { >>> drv->bdrv_close(bs); >>> } >>> - bdrv_unref_child(bs, bs->file); >>> - bs->file = NULL; >>> >>> - ret = bdrv_snapshot_goto(file, snapshot_id, errp); >>> + bdrv_unref_child(bs, *fallback_ptr); >>> + *fallback_ptr = NULL; >> >> Here we set *fallback_ptr to NULL... >> >>> + >>> + ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp); >>> open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err); >>> qobject_unref(options); >>> if (open_ret < 0) { >>> - bdrv_unref(file); >>> + bdrv_unref(fallback_bs); >>> bs->drv = NULL; >>> /* A bdrv_snapshot_goto() error takes precedence */ >>> error_propagate(errp, local_err); >>> return ret < 0 ? ret : open_ret; >>> } >>> >>> - assert(bs->file->bs == file); >>> - bdrv_unref(file); >>> + assert(fallback_bs == (*fallback_ptr)->bs); >> >> ...but here we dereference *fallback_ptr, and Coverity doesn't see >> anything that it recognizes as being able to change it. >> >>> + bdrv_unref(fallback_bs); >>> return ret; >>> } >> >> False positive, or real issue? (If a false positive, a comment >> explaining what's going on wouldn't go amiss -- as a human reader >> I'm kind of confused about whether there's some kind of hidden >> magic going on here.) > > I think it's a false positive because drv->bdrv_open() is supposed to > give it a non-NULL value again. Not sure if we can make the assumption > in every case without checking it, but it feels reasonable to require > that drv->bdrv_open() would return failure otherwise. Max? Yes. I think it’s sensible to add an *fallback_ptr non-NULL check to the assert condition (i.e., assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs); ), because the intention of the condition is already to verify that .bdrv_open() has opened the right node. So we might say what’s missing is to also assert that it has opened any node at all, but if we’re fine with asserting that it has opened the right node (which we did since 7a9e51198c24), we should definitely be fine with asserting that it has opened any node at all. Max