From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47036) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VEeVX-0000wa-Sx for qemu-devel@nongnu.org; Wed, 28 Aug 2013 08:06:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VEeVR-0000CR-T1 for qemu-devel@nongnu.org; Wed, 28 Aug 2013 08:05:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:27837) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VEeVR-0000CK-Ku for qemu-devel@nongnu.org; Wed, 28 Aug 2013 08:05:49 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7SC5mTN018259 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 28 Aug 2013 08:05:49 -0400 Message-ID: <521DE799.1080607@redhat.com> Date: Wed, 28 Aug 2013 14:05:45 +0200 From: Max Reitz MIME-Version: 1.0 References: <1377677336-15804-1-git-send-email-mreitz@redhat.com> <1377677336-15804-3-git-send-email-mreitz@redhat.com> <20130828110620.GE2743@dhcp-200-207.str.redhat.com> <521DE171.30209@redhat.com> <20130828114826.GG2743@dhcp-200-207.str.redhat.com> In-Reply-To: <20130828114826.GG2743@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/3] qcow2: Implement bdrv_amend_options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi Hi, Am 28.08.2013 13:48, schrieb Kevin Wolf: > Am 28.08.2013 um 13:39 hat Max Reitz geschrieben: >>>> + /* clear incompatible features */ >>>> + if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) { >>>> + BdrvCheckResult result; >>>> + ret = qcow2_check(bs, &result, BDRV_FIX_ERRORS); >>>> + if (ret < 0) { >>>> + return ret; >>>> + } >>> This is unnecessary: The image could be opened, so we know that it was >>> clean when we started. We also know that we haven't crashed yet, so if we >>> flush all in-memory data, we'll have a consistent on-disk state again. >>> >>> qcow2_mark_clean() already calls bdrv_flush(bs), so it does everything >>> that is needed in this respect. >> So that flag should always be already cleared at this point anyway? > The qcow2_mark_clean() call is on the next line (which you removed from > the quote), so not at this point but one line later. But yeah, it's done > by other code. Yes, I was referring to other code (which cleans the image right at opening). >>>> + } else if (!strcmp(options[i].name, "encryption")) { >>>> + if (options[i].value.n != !!s->crypt_method) { >>>> + fprintf(stderr, "Changing the encryption flag is not " >>>> + "supported.\n"); >>>> + return -ENOTSUP; >>>> + } >>>> + } else if (!strcmp(options[i].name, "cluster_size")) { >>>> + if (options[i].value.n && (options[i].value.n != s->cluster_size)) { >>>> + fprintf(stderr, "Changing the cluster size is not " >>>> + "supported.\n"); >>>> + return -ENOTSUP; >>>> + } >>>> + } else if (!strcmp(options[i].name, "lazy_refcounts")) { >>>> + /* TODO: detect whether this flag was indeed explicitly given */ >>>> + lazy_refcounts = options[i].value.n; >>> I can see two ways to achieve this: >>> >>> 1. Add a new field 'bool assigned' to QEMUOptionParameter, which would >>> be cleared before parsing an option string and set for each option in >>> set_option_parameter() >>> >>> 2. Get the QemuOpts conversion series in and add a function that tells >>> whether a given option was specified or not. >>> >>> The same TODO should actually apply to encryption and cluster_size as >>> well, shouldn't it? >> Kind of; however, a cluster_size of 0 seems invalid to me, therefore >> it is pretty easy to detect that option not being given. > Depends on whether you think that 'qemu-img amend -o cluster_size=0' is > a valid way of saying that you don't want to change the cluster size. I > would prefer to error out. No, I just missed the default for that option not being zero. Sorry, my fault. Max