From patchwork Sun Sep 21 14:58:59 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laszlo Ersek X-Patchwork-Id: 37662 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-we0-f198.google.com (mail-we0-f198.google.com [74.125.82.198]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 31BEF20063 for ; Sun, 21 Sep 2014 15:06:30 +0000 (UTC) Received: by mail-we0-f198.google.com with SMTP id t60sf890700wes.1 for ; Sun, 21 Sep 2014 08:06:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:message-id:date:from:user-agent :mime-version:to:references:in-reply-to:cc:subject:precedence :list-id:list-unsubscribe:list-archive:list-post:list-help :list-subscribe:sender:errors-to:x-original-sender :x-original-authentication-results:mailing-list:content-type; bh=vMgW3QwY9ZRBPwgIh0girksw6ZSYruLqUld+Cn8TsZA=; b=Amo6c75G3Zx9up+AppmKRxPz8khl8yjB2Q5BlwHIaRUXxk9k3OFkbd0V92EWcet6PL 6fWxxvIiUZEGj0sGiOW1iRHPY3DB/2BvLk2U1Y4o2iY6aV53o5X2MHTWrI9ieBM1ENof xqqLA6gq396tFOppHR6mzIZ6uF60HLI2uTgbJrU0EOI3QXyHHfwzW1Xr46zK7XL5ZJl9 54dLhmcdMGXt2ToHeGBLKFI+udkUS9yJFHkslA3z7zLM2MalweAXCfpnOCkJvmrcpRUJ 4kjs9J4ovxv0fZxDoDFmaaypvjKCtoxguk0kmZgJ21QttL4peRsviLrsA+He9sMiRLIz sc5Q== X-Gm-Message-State: ALoCoQk//oOUJcUM9Z0LlQZ9A7jRQcMf5RtoB1kJUVgngCuwzx0e2/xY9HzRQllVteXBnCWJ8HqU X-Received: by 10.194.201.198 with SMTP id kc6mr3653318wjc.0.1411311989383; Sun, 21 Sep 2014 08:06:29 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.43.103 with SMTP id v7ls490137lal.32.gmail; Sun, 21 Sep 2014 08:06:28 -0700 (PDT) X-Received: by 10.152.23.199 with SMTP id o7mr20096884laf.26.1411311988765; Sun, 21 Sep 2014 08:06:28 -0700 (PDT) Received: from mail-lb0-f171.google.com (mail-lb0-f171.google.com [209.85.217.171]) by mx.google.com with ESMTPS id cb3si11083686lad.16.2014.09.21.08.06.28 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Sun, 21 Sep 2014 08:06:28 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.171 as permitted sender) client-ip=209.85.217.171; Received: by mail-lb0-f171.google.com with SMTP id l4so5466270lbv.16 for ; Sun, 21 Sep 2014 08:06:28 -0700 (PDT) X-Received: by 10.152.204.231 with SMTP id lb7mr19775228lac.44.1411311988070; Sun, 21 Sep 2014 08:06:28 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.130.169 with SMTP id of9csp118973lbb; Sun, 21 Sep 2014 08:06:26 -0700 (PDT) X-Received: by 10.194.94.165 with SMTP id dd5mr14138195wjb.75.1411311986515; Sun, 21 Sep 2014 08:06:26 -0700 (PDT) Received: from mx3-phx2.redhat.com (mx3-phx2.redhat.com. [209.132.183.24]) by mx.google.com with ESMTPS id bh6si203948wib.51.2014.09.21.08.06.25 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Sun, 21 Sep 2014 08:06:26 -0700 (PDT) Received-SPF: pass (google.com: domain of libvir-list-bounces@redhat.com designates 209.132.183.24 as permitted sender) client-ip=209.132.183.24; Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by mx3-phx2.redhat.com (8.13.8/8.13.8) with ESMTP id s8LEx7Oh002631; Sun, 21 Sep 2014 10:59:08 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id s8LEx4X0007647; Sun, 21 Sep 2014 10:59:04 -0400 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-54.ams2.redhat.com [10.36.116.54]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s8LEx0oJ023686; Sun, 21 Sep 2014 10:59:01 -0400 Message-ID: <541EE7B3.2020509@redhat.com> Date: Sun, 21 Sep 2014 16:58:59 +0200 From: Laszlo Ersek User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: Cole Robinson , Michal Privoznik References: <541CCC33.7010007@redhat.com> In-Reply-To: <541CCC33.7010007@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-loop: libvir-list@redhat.com Cc: libvir-list@redhat.com, Giuseppe Scrivano , virt-tools-list@redhat.com Subject: Re: [libvirt] [PATCH virt-manager] details: Add UI for enabling UEFI via 'customize before install' X-BeenThere: libvir-list@redhat.com X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: , List-Help: , List-Subscribe: , Sender: libvir-list-bounces@redhat.com Errors-To: libvir-list-bounces@redhat.com X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: lersek@redhat.com X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.171 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 Hi Cole and Michal, I'm attaching three patches: On 09/20/14 02:37, Cole Robinson wrote: > On 09/17/2014 06:55 PM, Cole Robinson wrote: >> We expose a simple combobox with two entries: BIOS, and UEFI. The >> UEFI option is only selectable if 1) libvirt supports the necessary >> domcapabilities bits, 2) it detects that qemu supports the necessary >> command line options, and 3) libvirt detects a UEFI binary on the >> host that maps to a known template via qemu.conf >> >> If those conditions aren't met, we disable the UEFI option, and show >> a small warning icon with an explanatory tooltip. >> >> The option can only be changed via New VM->Customize Before Install. >> For existing x86 VMs, it's a readonly label. > > I've pushed this now, with follow up patches to handle a couple points > we discussed: > > - Remove the nvram deletion from delete.py, since it won't work in the > non-root case, and all uses of nvram should also be with a libvirt + > driver that provides UNDEFINE_NVRAM > > - Before using UNDEFINE_NVRAM, match the same condition that libvirt > uses: loader_ro is True and loader_type == "pflash" and bool(nvram) (1) unfortunately virt-manager commit 3feedb76 ("domain: Match UNDEFINE_NVRAM conditions with libvirt") is not correct (it doesn't work), and not what I had in mind: > diff --git a/virtManager/domain.py b/virtManager/domain.py > index 29f3861..abf3146 100644 > --- a/virtManager/domain.py > +++ b/virtManager/domain.py > @@ -1403,7 +1403,9 @@ class vmmDomain(vmmLibvirtObject): > flags |= getattr(libvirt, > "VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA", 0) > flags |= getattr(libvirt, "VIR_DOMAIN_UNDEFINE_MANAGED_SAVE", 0) > - if self.get_xmlobj().os.nvram: > + if (self.get_xmlobj().os.loader_ro is True and > + self.get_xmlobj().os.loader_type == "pflash" and > + self.get_xmlobj().os.nvram): > flags |= getattr(libvirt, "VIR_DOMAIN_UNDEFINE_NVRAM", 0) > try: > self._backend.undefineFlags(flags) Before virt-manager commit 3feedb76, the condition on which to set the flag was: self.get_xmlobj().os.nvram and it didn't work for all possible cases. After the patch, what you have is self.get_xmlobj().os.nvram *and* blah The commit only constrains (further tightens, further restricts) the original condition, which had been too restrictive to begin with. Again, the nvram element (or its text() child) can be completely absent from the domain XML -- libvirtd will generate the pathname of the varstore file on the fly, and it need not be part of the serialized XML file. So in the XML all you'll have is hvm /usr/share/OVMF/OVMF_CODE.fd or hvm /custom/OVMF_CODE.fd My suggestion was to *replace* the original condition with the (loader_ro && loader_type=="pflash") one. If you check my earlier message, I wrote *iff*, meaning "if and only if". Cole, can you please apply the first attached patch? (2) Unfortunately, even libvirtd needs to be modified, in addition. My patch for (1) *does* pass VIR_DOMAIN_UNDEFINE_NVRAM to libvirtd (I verified that with gdb), but libvirtd doesn't act upon it correctly. Namely, in the libvirtd source code, in qemuDomainUndefineFlags(), commit 273b6581 added: if (!virDomainObjIsActive(vm) && vm->def->os.loader && vm->def->os.loader->nvram && virFileExists(vm->def->os.loader->nvram)) { if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot delete inactive domain with nvram")); goto cleanup; } if (unlink(vm->def->os.loader->nvram) < 0) { virReportSystemError(errno, _("failed to remove nvram: %s"), vm->def->os.loader->nvram); goto cleanup; } } Here "vm->def->os.loader->nvram" evaluates to NULL: 6468 if (!virDomainObjIsActive(vm) && 6469 vm->def->os.loader && vm->def->os.loader->nvram && 6470 virFileExists(vm->def->os.loader->nvram)) { 6471 if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { 6472 virReportError(VIR_ERR_OPERATION_INVALID, "%s", (gdb) print vm->def->os.loader $1 = (virDomainLoaderDefPtr) 0x7ff59c00bf20 (gdb) print vm->def->os.loader->nvram $2 = 0x0 I thought that the auto-generation of the nvram pathname (internal to libvirtd, ie. not visible in the XML file) would occur on the undefine path as well. Apparently this is not the case. Indeed, the only call to qemuPrepareNVRAM() is from qemuProcessStart(). Michal, would it be possible generate the *pathname* of the separate varstore in qemuDomainUndefineFlags() too? Please see the second attached patch; it works for me. (This patch is best looked at with "git diff -b" (or "git show -b").) (3) I just realized that a domain's name can change during its lifetime. Renaming a domain becomes a problem when the varstore's pathname is deduced from the domain's name. For this reason, the auto-generation should use the domain's UUID (which never changes). Michal, what do you think of the 3rd attached patch? I can resubmit these as standalone patches / series as well (the first to virt-tools-list, and the last two to libvir-list), if that's necessary. Thanks, Laszlo --- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list >From 5fd4f9a9430c757aae8681ba7e9f65ec55bb7889 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Sun, 21 Sep 2014 16:07:22 +0200 Subject: [PATCH 2/2] qemu NVRAM: auto-generated pathnames should contain domain UUIDs The name of a domain is unstable in the sense that the domain can be renamed. Currently a rename will cause the domain to lose connection with its separate varstore file, if the pathname of the varstore was automatically generated. Hence generate such pathnames based on the domain's UUID, because that one never changes. Signed-off-by: Laszlo Ersek --- src/qemu/qemu_driver.c | 7 +++++-- src/qemu/qemu_process.c | 7 +++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 16cd3e3..9ea1b5b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6474,9 +6474,12 @@ qemuDomainUndefineFlags(virDomainPtr dom, loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH && loader->readonly == VIR_TRISTATE_SWITCH_ON) { if (!loader->nvram) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + if (virAsprintf(&loader->nvram, - "%s/lib/libvirt/qemu/nvram/%s_VARS.fd", - LOCALSTATEDIR, vm->def->name) < 0) + "%s/lib/libvirt/qemu/nvram/%s-VARS.fd", + LOCALSTATEDIR, + virUUIDFormat(vm->def->uuid, uuidstr)) < 0) goto cleanup; nvramPathnameGenerated = true; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 13614e9..57bde45 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3866,9 +3866,12 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg, /* Autogenerate nvram path if needed.*/ if (!loader->nvram) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + if (virAsprintf(&loader->nvram, - "%s/lib/libvirt/qemu/nvram/%s_VARS.fd", - LOCALSTATEDIR, def->name) < 0) + "%s/lib/libvirt/qemu/nvram/%s-VARS.fd", + LOCALSTATEDIR, + virUUIDFormat(def->uuid, uuidstr)) < 0) goto cleanup; generated = true; -- 1.8.3.1