Re: [PATCH v2 01/23] MAINTAINERS: Add Connor Kuehl as reviewer for AMD SEV
On 8/30/21 9:18 AM, Philippe Mathieu-Daudé wrote: >> >> +AMD Secure Encrypted Virtualization (SEV) >> +R: Connor Kuehl > > Is this patch still valid? Thank you for championing it, but due to recent changes, no, it is no longer valid. Thank you, Connor
Re: [PATCH v3 0/2] x86/sev: Measured Linux SEV guest with kernel/initrd/cmdline
On Fri Jul 30, 2021 at 1:02 PM CDT, Dov Murik wrote: > > > > Awesome! Unfortunately, it's looking like we'll have to wait[1] for QEMU to > > thaw before this series goes in. > > > > Thanks for explaining this. Do I need to do anything after 6.1 is > released? Ping? Rebase and re-send? Rebase and re-send. I think your patches already have the Reviewed-by tags in the patch descriptions, but if that's not the case, make sure you add them for the re-send so it's obvious that the patches have already been reviewed. Thank you, Connor
Re: [PATCH v3 0/2] x86/sev: Measured Linux SEV guest with kernel/initrd/cmdline
On Thu Jul 29, 2021 at 2:31 PM CDT, Dov Murik wrote: > The OVMF companion series has been reviewed by the new OVMF maintainer > and merged to edk2 master branch as of edk2 commit 514b3aa08ece [1]. > > [1] https://github.com/tianocore/edk2/commit/514b3aa08ece Awesome! Unfortunately, it's looking like we'll have to wait[1] for QEMU to thaw before this series goes in. Connor [1] https://wiki.qemu.org/Planning/6.1
Re: [RFC PATCH v2 11/44] i386/tdx: Implement user specified tsc frequency
On 7/7/21 7:54 PM, isaku.yamah...@gmail.com wrote:
From: Xiaoyao Li
Reuse -cpu,tsc-frequency= to get user wanted tsc frequency and pass it
to KVM_TDX_INIT_VM.
Besides, sanity check the tsc frequency to be in the legal range and
legal granularity (required by SEAM module).
Signed-off-by: Xiaoyao Li
Signed-off-by: Isaku Yamahata
---
[..]
+if (env->tsc_khz && (env->tsc_khz < TDX1_MIN_TSC_FREQUENCY_KHZ ||
+ env->tsc_khz > TDX1_MAX_TSC_FREQUENCY_KHZ)) {
+error_report("Invalid TSC %ld KHz, must specify cpu_frequecy between [%d,
%d] kHz\n",
s/frequecy/frequency
+ env->tsc_khz, TDX1_MIN_TSC_FREQUENCY_KHZ,
+ TDX1_MAX_TSC_FREQUENCY_KHZ);
+exit(1);
+}
+
+if (env->tsc_khz % (25 * 1000)) {
+error_report("Invalid TSC %ld KHz, it must be multiple of 25MHz\n",
env->tsc_khz);
Should this be 25KHz instead of 25MHz?
Re: [RFC PATCH v2 12/44] target/i386/tdx: Finalize the TD's measurement when machine is done
On 7/7/21 7:54 PM, isaku.yamah...@gmail.com wrote: From: Xiaoyao Li Invoke KVM_TDX_FINALIZEMR to finalize the TD's measurement and make the TD vCPUs runnable once machine initialization is complete. Signed-off-by: Xiaoyao Li Signed-off-by: Isaku Yamahata --- target/i386/kvm/kvm.c | 7 +++ target/i386/kvm/tdx.c | 21 + target/i386/kvm/tdx.h | 3 +++ 3 files changed, 31 insertions(+) diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index be0b96b120..5742fa4806 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -53,6 +53,7 @@ #include "migration/blocker.h" #include "exec/memattrs.h" #include "trace.h" +#include "tdx.h" //#define DEBUG_KVM @@ -2246,6 +2247,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s) return ret; } This is probably a good place in the series to update the comment preceding the sev_kvm_init call since TDX is now here and otherwise the comment seems untimely. Reviewed-by: Connor Kuehl
Re: [RFC PATCH v2 06/44] hw/i386: Introduce kvm-type for TDX guest
On 7/7/21 7:54 PM, isaku.yamah...@gmail.com wrote: From: Xiaoyao Li Introduce a machine property, kvm-type, to allow the user to create a Trusted Domain eXtensions (TDX) VM, a.k.a. a Trusted Domain (TD), e.g.: # $QEMU \ -machine ...,kvm-type=tdx \ ... Only two types are supported: "legacy" and "tdx", with "legacy" being the default. Signed-off-by: Xiaoyao Li Co-developed-by: Sean Christopherson Signed-off-by: Sean Christopherson Signed-off-by: Isaku Yamahata I am not a QEMU command line expert, so my mental model of this may be wrong, but: This seems to have a very broad scope on the command line and I am wondering if it's possible to associate it with a TDX command line object specifically to narrow its scope. I.e., is it possible to express this on the command line when launching something that is _not_ meant to be powered by TDX, such as an SEV guest? If it doesn't make sense to express that command line argument in a situation like that, perhaps it could be constrained only to the TDX-specific commandline objects.
Re: [RFC PATCH v2 09/44] target/i386: kvm: don't synchronize guest tsc for TD guest
On 7/7/21 7:54 PM, isaku.yamah...@gmail.com wrote: From: Isaku Yamahata Make kvm_synchronize_all_tsc() nop for TD-guest. s/nop/noop TDX module specification, 9.11.1 TSC Virtualization This appears in 9.12.1 of the latest revision as of this writing. https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-module-1eas-v0.85.039.pdf "Virtual TSC values are consistent among all the TD;s VCPUs at the s/TD;s/TDs level suppored by the CPU". s/suppored/supported There is no need for qemu to synchronize tsc and VMM can't access to guest TSC. Actually do_kvm_synchronize_tsc() hits assert due to failure to write to guest tsc. qemu/target/i386/kvm.c:235: kvm_get_tsc: Assertion `ret == 1' failed. Signed-off-by: Isaku Yamahata Reviewed-by: Connor Kuehl
Re: [RFC PATCH v2 32/44] tdx: add kvm_tdx_enabled() accessor for later use
On 7/7/21 7:55 PM, isaku.yamah...@gmail.com wrote:
From: Isaku Yamahata
Signed-off-by: Isaku Yamahata
---
include/sysemu/tdx.h | 1 +
target/i386/kvm/kvm.c | 5 +
2 files changed, 6 insertions(+)
diff --git a/include/sysemu/tdx.h b/include/sysemu/tdx.h
index 70eb01348f..f3eced10f9 100644
--- a/include/sysemu/tdx.h
+++ b/include/sysemu/tdx.h
@@ -6,6 +6,7 @@
#include "hw/i386/pc.h"
bool kvm_has_tdx(KVMState *s);
+bool kvm_tdx_enabled(void);
int tdx_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
#endif
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index af6b5f350e..76c3ea9fac 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -152,6 +152,11 @@ int kvm_set_vm_type(MachineState *ms, int kvm_type)
return -ENOTSUP;
}
+bool kvm_tdx_enabled(void)
+{
+return vm_type == KVM_X86_TDX_VM;
+}
+
Is this the whole story? Does this guarantee that the VM QEMU is
responsible to bring up is a successfully initialized TD?
From my reading of the series as it unfolded, this looks like the
function proves that KVM can support TDs and that the user requested
a TDX kvm-type, not that we have a fully-formed TD.
Is it possible to associate this with a more verifiable metric that
the TD has been or will be created successfully? I.e., once the VM
has successfully called the TDX INIT ioctl or has finalized setup?
My question mainly comes from a later patch in the series, where the
"query-tdx-capabilities" and "query-tdx" QMP commands are added.
Forgive me if I am misinterpreting the semantics of each of these
commands:
"query-tdx-capabilities" sounds like it answers the question of
"can it run a TD?"
and "query-tdx" sounds like it answers the question of "is it a TD?"
Is the assumption with "query-tdx" that anything that's gone wrong
with developing a TD will have resulted in the QEMU process exiting
and therefore if we get to a point where we can run "query-tdx" then
we know the TD was successfully formed?
Re: [RFC PATCH v2 34/44] target/i386/tdx: set reboot action to shutdown when tdx
On 7/7/21 7:55 PM, isaku.yamah...@gmail.com wrote: From: Isaku Yamahata In TDX CPU state is also protected, thus vcpu state can't be reset by VMM. It assumes -action reboot=shutdown instead of silently ignoring vcpu reset. TDX module spec version 344425-002US doesn't support vcpu reset by VMM. VM needs to be destroyed and created again to emulate REBOOT_ACTION_RESET. For simplicity, put its responsibility to management system like libvirt because it's difficult for the current qemu implementation to destroy and re-create KVM VM resources with keeping other resources. If management system wants reboot behavior for its users, it needs to - set reboot_action to REBOOT_ACTION_SHUTDOWN, - set shutdown_action to SHUTDOWN_ACTION_PAUSE optionally and, - subscribe VM state change and on reboot, (destroy qemu if SHUTDOWN_ACTION_PAUSE and) start new qemu. Signed-off-by: Isaku Yamahata --- target/i386/kvm/tdx.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c index 1316d95209..0621317b0a 100644 --- a/target/i386/kvm/tdx.c +++ b/target/i386/kvm/tdx.c @@ -25,6 +25,7 @@ #include "qapi/qapi-types-misc-target.h" #include "standard-headers/asm-x86/kvm_para.h" #include "sysemu/sysemu.h" +#include "sysemu/runstate-action.h" #include "sysemu/kvm.h" #include "sysemu/kvm_int.h" #include "sysemu/tdx.h" @@ -363,6 +364,19 @@ static void tdx_guest_init(Object *obj) qemu_mutex_init(>lock); +/* + * TDX module spec version 344425-002US doesn't support reset of vcpu by + * VMM. VM needs to be destroyed and created again to emulate + * REBOOT_ACTION_RESET. For simplicity, put its responsibility to + * management system like libvirt. + * + * Management system should + * - set reboot_action to REBOOT_ACTION_SHUTDOWN + * - set shutdown_action to SHUTDOWN_ACTION_PAUSE + * - subscribe VM state and on reboot, destroy qemu and start new qemu + */ +reboot_action = REBOOT_ACTION_SHUTDOWN; + tdx->debug = false; object_property_add_bool(obj, "debug", tdx_guest_get_debug, tdx_guest_set_debug); I think the same effect could be accomplished with modifying kvm_arch_cpu_check_are_resettable.
Re: [RFC PATCH v2 04/44] vl: Introduce machine_init_done_late notifier
On 7/7/21 7:54 PM, isaku.yamah...@gmail.com wrote: From: Isaku Yamahata Introduce a new notifier, machine_init_done_late, that is notified after machine_init_done. This will be used by TDX to generate the HOB for its virtual firmware, which needs to be done after all guest memory has been added, i.e. after machine_init_done notifiers have run. Some code registers memory by machine_init_done(). Signed-off-by: Isaku Yamahata --- hw/core/machine.c | 26 ++ include/sysemu/sysemu.h | 2 ++ 2 files changed, 28 insertions(+) diff --git a/hw/core/machine.c b/hw/core/machine.c index ffc076ae84..66c39cf72a 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -1278,6 +1278,31 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify) notifier_remove(notify); } +static NotifierList machine_init_done_late_notifiers = +NOTIFIER_LIST_INITIALIZER(machine_init_done_late_notifiers); I think a comment here describing the difference between machine_init_done and machine_init_done_late would go a long way for other developers so they don't have to hunt through the git log. Connor
Re: [RFC PATCH v2 01/44] target/i386: Expose x86_cpu_get_supported_feature_word() for TDX
On 7/7/21 7:54 PM, isaku.yamah...@gmail.com wrote: From: Sean Christopherson Expose x86_cpu_get_supported_feature_word() outside of cpu.c so that it can be used by TDX to setup the VM-wide CPUID configuration. Signed-off-by: Sean Christopherson Signed-off-by: Isaku Yamahata Reviewed-by: Connor Kuehl
Re: [RFC PATCH v2 02/44] kvm: Switch KVM_CAP_READONLY_MEM to a per-VM ioctl()
On 7/7/21 7:54 PM, isaku.yamah...@gmail.com wrote: From: Isaku Yamahata Switch to making a VM ioctl() call for KVM_CAP_READONLY_MEM, which may be conditional on VM type in recent versions of KVM, e.g. when TDX is supported. kvm_vm_check_extension() has fallback from kvm_vm_ioctl() to kvm_check_extension(). fallback from VM ioctl to System ioctl for compatibility for old kernel. Signed-off-by: Isaku Yamahata --- accel/kvm/kvm-all.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index e5b10dd129..fdbe24bf59 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -2531,7 +2531,7 @@ static int kvm_init(MachineState *ms) } kvm_readonly_mem_allowed = -(kvm_check_extension(s, KVM_CAP_READONLY_MEM) > 0); +(kvm_vm_check_extension(s, KVM_CAP_READONLY_MEM) > 0); kvm_eventfds_allowed = (kvm_check_extension(s, KVM_CAP_IOEVENTFD) > 0); Reviewed-by: Connor Kuehl
Re: [PATCH] docs: convert writing-qmp-commands.txt to writing-qmp-commands.rst
On 7/21/21 11:50 AM, John Snow wrote: This does about the bare minimum, converting section headers to ReST ones and adding an indent for code blocks. Signed-off-by: John Snow --- Looks like ReST! The generated document also looks good to me. Reviewed-by: Connor Kuehl
Re: [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled
On 7/9/21 3:55 PM, Brijesh Singh wrote:
> The KVM_SEV_SNP_LAUNCH_UPDATE command is used for encrypting the bios
> image used for booting the SEV-SNP guest.
>
> Signed-off-by: Brijesh Singh
> ---
> target/i386/sev.c| 33 -
> target/i386/trace-events | 1 +
> 2 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 259408a8f1..41dcb084d1 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -883,6 +883,30 @@ out:
> return ret;
> }
>
> +static int
> +sev_snp_launch_update(SevGuestState *sev, uint8_t *addr, uint64_t len, int
> type)
> +{
> +int ret, fw_error;
> +struct kvm_sev_snp_launch_update update = {};
> +
> +if (!addr || !len) {
> +return 1;
Should this be a -1? It looks like the caller checks if this function
returns < 0, but doesn't check for res == 1.
Alternatively, invoking error_report might provide more useful
information that the preconditions to this function were violated.
Connor
Re: [PATCH v3 0/2] x86/sev: Measured Linux SEV guest with kernel/initrd/cmdline
On 7/8/21 10:03 AM, Philippe Mathieu-Daudé wrote: > On 7/8/21 6:41 PM, Connor Kuehl wrote: >> Hi Paolo, >> >> Please consider this series[1] for inclusion into your next pull request. >> >> Just a note that this series has a companion series that is getting >> upstreamed into OVMF[2] > > Shouldn't we get the OVMF part merged first? The approach taken in the OVMF series doesn't seem very controversial, so I don't anticipate any breaking changes with the current state of those patches as far as QEMU is concerned. However, I'm fine with erring on the side of caution. Connor
Re: [PATCH v3 0/2] x86/sev: Measured Linux SEV guest with kernel/initrd/cmdline
Hi Paolo, Please consider this series[1] for inclusion into your next pull request. Just a note that this series has a companion series that is getting upstreamed into OVMF[2] [1] Patchwork link, if convenient: https://patchwork.kernel.org/project/qemu-devel/cover/20210624102040.2015280-1-dovmu...@linux.ibm.com/ [2] https://bugzilla.tianocore.org/show_bug.cgi?id=3457#c6 Thank you, Connor On 6/24/21 3:20 AM, Dov Murik wrote: > Currently booting with -kernel/-initrd/-append is not supported in SEV > confidential guests, because the content of these blobs is not measured > and therefore not trusted by the SEV guest. > > However, in some cases the kernel, initrd, and cmdline are not secret > but should not be modified by the host. In such a case, we want to > verify inside the trusted VM that the kernel, initrd, and cmdline are > indeed the ones expected by the Guest Owner, and only if that is the > case go on and boot them up (removing the need for grub inside OVMF in > that mode). > > To support that, OVMF adds a special area for hashes of > kernel/initrd/cmdline; that area is expected to be filled by QEMU and > encrypted as part of the initial SEV guest launch. This in turn makes > the hashes part of the PSP measured content, and OVMF can trust these > inputs if they match the hashes. > > This series adds an SEV function to generate the table of hashes for > OVMF and encrypt it (patch 1/2), and calls this function if SEV is > enabled when the kernel/initrd/cmdline are prepared (patch 2/2). > > Corresponding OVMF support was submitted to edk2-devel [1] (patch series > "Measured SEV boot with kernel/initrd/cmdline"); it's still under > review. > > [1] https://edk2.groups.io/g/devel/topic/patch_v1_0_8_measured_sev/83074450 > > --- > > v3 changes: > - initrd hash is now mandatory; if no -initrd is passed, calculate the >hash of the empty buffer. This is now aligned with the OVMF >behaviour which verifies the empty initrd (correctly). > - make SevHashTable entries fixed: 3 entries for cmdline, initrd, and kernel. > - in sev_add_kernel_loader_hashes: first calculate all the hashes, only then >fill-in the hashes table in the guest's memory. > - Use g_assert_not_reached in sev-stub.c. > - Use QEMU_PACKED attribute for structs. > - Use QemuUUID type for guids. > - in sev_add_kernel_loader_hashes: use ARRAY_SIZE(iov) instead of literal 2. > > v2: > https://lore.kernel.org/qemu-devel/20210621190553.1763020-1-dovmu...@linux.ibm.com/ > v2 changes: > - Extract main functionality to sev.c (with empty stub in sev-stub.c) > - Use sev_enabled() instead of machine->cgs->ready to detect SEV guest > - Coding style changes > > v1: > https://lore.kernel.org/qemu-devel/20210525065931.1628554-1-dovmu...@linux.ibm.com/ > > Dov Murik (2): > sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux > boot > x86/sev: generate SEV kernel loader hashes in x86_load_linux > > target/i386/sev_i386.h | 12 > hw/i386/x86.c | 25 +++- > target/i386/sev-stub.c | 5 ++ > target/i386/sev.c | 137 + > 4 files changed, 178 insertions(+), 1 deletion(-) > > > base-commit: b22726abdfa54592d6ad88f65b0297c0e8b363e2 >
Re: [PATCH] Fix libpmem configuration option
On Wed Jul 7, 2021 at 12:51 AM PDT, Miroslav Rezanina wrote: > For some reason, libpmem option setting was set to work in an opposite > way (--enable-libpmem disabled it and vice versa). Fixing this so > configuration works properly. > > Signed-off-by: Miroslav Rezanina > --- > configure | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/configure b/configure > index 7994bdee92..ffa93cc5fd 100755 > --- a/configure > +++ b/configure > @@ -1501,9 +1501,9 @@ for opt do > ;; > --disable-debug-mutex) debug_mutex=no > ;; > - --enable-libpmem) libpmem=disabled > + --enable-libpmem) libpmem="enabled" > ;; > - --disable-libpmem) libpmem=enabled > + --disable-libpmem) libpmem="disabled" > ;; > --enable-xkbcommon) xkbcommon="enabled" > ;; > -- > 2.27.0 Good catch. Reviewed-by: Connor Kuehl or Tested-by: Connor Kuehl
Re: [PATCH 0/2] numa: Parse initiator= attribute before cpus= attribute
On Wed Jul 7, 2021 at 6:40 AM PDT, Michal Privoznik wrote: > See 2/2 for explanation. The first patch is just cosmetics. > > Michal Privoznik (2): > numa: Report expected initiator > numa: Parse initiator= attribute before cpus= attribute > > hw/core/machine.c | 3 ++- > hw/core/numa.c | 45 +++-- > 2 files changed, 25 insertions(+), 23 deletions(-) > > -- > 2.31.1 For the series: Reviewed-by: Connor Kuehl
Re: [PATCH] block/rbd: fix type of task->complete
On 7/7/21 11:04 AM, Peter Lieven wrote:
> task->complete is a bool not an integer.
>
> Signed-off-by: Peter Lieven
> ---
> block/rbd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index 01a7b94d62..dcf82b15b8 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -1066,7 +1066,7 @@ static int qemu_rbd_resize(BlockDriverState *bs,
> uint64_t size)
> static void qemu_rbd_finish_bh(void *opaque)
> {
> RBDTask *task = opaque;
> -task->complete = 1;
> +task->complete = true;
> aio_co_wake(task->co);
> }
>
>
Hi Peter,
What tree/QEMU git sha does this apply to? I am having trouble
finding definitions for RBDTask and qemu_rbd_finish_bh; and the
patch won't apply to my few-minutes-old clone of upstream.
Connor
Re: [PATCH 0/2] Remove deprecated qemu-img backing file without format
On 5/3/21 2:35 PM, Eric Blake wrote: > We've gone enough release cycles without noticeable pushback on our > intentions, so time to make it harder to create images that can form a > security hole due to a need for format probing rather than an explicit > format. > > Eric Blake (2): > qcow2: Prohibit backing file changes in 'qemu-img amend' > qemu-img: Require -F with -b backing image > > docs/system/deprecated.rst | 32 --- > docs/system/removed-features.rst | 31 ++ > block.c | 37 ++-- > block/qcow2.c| 13 --- > qemu-img.c | 6 -- > tests/qemu-iotests/061 | 3 +++ > tests/qemu-iotests/061.out | 3 ++- > tests/qemu-iotests/082.out | 6 -- > tests/qemu-iotests/114 | 18 > tests/qemu-iotests/114.out | 11 -- > tests/qemu-iotests/301 | 4 +--- > tests/qemu-iotests/301.out | 16 ++ > 12 files changed, 75 insertions(+), 105 deletions(-) > For the series + the squash attached to patch 2: Reviewed-by: Connor Kuehl
Re: Contributions: Adding New Devices
On 6/30/21 7:01 AM, Federico Vaga wrote: > Hello, > > I can't find this information on the website, so here I am. > > I developed a QEMU device that virtualises a PCI card that we widely use at > CERN. > But this card is only used at CERN. > > Clearly, having CERN specific devices in QEMU does not help much the qemu > community, hence I maintain an internal QEMU fork. > > But, I was wondering what is the QEMU policy about contributions that are > known to be > used only by a handful of people (one organization in this case)? Are they > accepted? Your first instinct is correct that it's unlikely that the community will be able to maintain a device if it's really so niche as to only be used at your organization. However, if you do decide to try to upstream it, it could only help your chances if you or some of your colleagues agreed to maintain it for the QEMU community. This mainly involves adding an entry to the MAINTAINERS file where, if accepted, the expectation is that you'll be reachable within reason to review patches, make pull requests, help discuss bugs in the subsystem, etc. Sorry I don't have a concrete "yes" or "no" for you; but I'd recommend giving it a shot if you have the time. Connor
Re: [PATCH v3 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot
On 6/24/21 3:20 AM, Dov Murik wrote: > Add the sev_add_kernel_loader_hashes function to calculate the hashes of > the kernel/initrd/cmdline and fill a designated OVMF encrypted hash > table area. For this to work, OVMF must support an encrypted area to > place the data which is advertised via a special GUID in the OVMF reset > table. > > The hashes of each of the files is calculated (or the string in the case > of the cmdline with trailing '\0' included). Each entry in the hashes > table is GUID identified and since they're passed through the > sev_encrypt_flash interface, the hashes will be accumulated by the PSP > measurement (SEV_LAUNCH_MEASURE). > > Co-developed-by: James Bottomley > Signed-off-by: James Bottomley > Signed-off-by: Dov Murik > --- Reviewed-by: Connor Kuehl
Re: [PATCH v2 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot
On 6/21/21 2:05 PM, Dov Murik wrote:
> +static void fill_sev_hash_table_entry(SevHashTableEntry *e, const uint8_t
> *guid,
> + const uint8_t *hash, size_t hash_len)
> +{
> +memcpy(e->guid, guid, sizeof(e->guid));
> +e->len = sizeof(*e);
> +memcpy(e->hash, hash, hash_len);
Should this memcpy be constrained to MIN(sizeof(e->hash), hash_len)? Or
perhaps an assert statement since I see below that this function's
caller sets this to HASH_SIZE which is currently == sizeof(e->hash).
Actually, the assert statement would be easier to debug if the input
to this function is ever unexpected, especially since it avoids an
outcome where the input is silently truncated; which is a pitfall that
that the memcpy clamping would fall into.
Connor
Re: [PATCH v2 2/2] x86/sev: generate SEV kernel loader hashes in x86_load_linux
On 6/21/21 2:05 PM, Dov Murik wrote:
> If SEV is enabled and a kernel is passed via -kernel, pass the hashes of
> kernel/initrd/cmdline in an encrypted guest page to OVMF for SEV
> measured boot.
>
> Co-developed-by: James Bottomley
> Signed-off-by: James Bottomley
> Signed-off-by: Dov Murik
> ---
> hw/i386/x86.c | 25 -
> 1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index ed796fe6ba..5c46463d9f 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -45,6 +45,7 @@
> #include "hw/i386/fw_cfg.h"
> #include "hw/intc/i8259.h"
> #include "hw/rtc/mc146818rtc.h"
> +#include "target/i386/sev_i386.h"
>
> #include "hw/acpi/cpu_hotplug.h"
> #include "hw/irq.h"
> @@ -778,6 +779,7 @@ void x86_load_linux(X86MachineState *x86ms,
> const char *initrd_filename = machine->initrd_filename;
> const char *dtb_filename = machine->dtb;
> const char *kernel_cmdline = machine->kernel_cmdline;
> +KernelLoaderContext kernel_loader_context = {};
>
> /* Align to 16 bytes as a paranoia measure */
> cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
> @@ -924,6 +926,8 @@ void x86_load_linux(X86MachineState *x86ms,
> fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr);
> fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(kernel_cmdline) + 1);
> fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);
> +kernel_loader_context.cmdline_data = (char *)kernel_cmdline;
> +kernel_loader_context.cmdline_size = strlen(kernel_cmdline) + 1;
I just wanted to check my understanding: I'm guessing you didn't set
`kernel_loader_context.cmdline_size` to `cmdline_size` (defined above)
so guest owners don't have to be aware of whatever alignment precaution
QEMU takes when producing their own measurement, right?
Otherwise:
Reviewed-by: Connor Kuehl
Re: [PATCH 10/11] target/i386/monitor: Move SEV specific commands to sev.c
On 6/10/21 1:45 AM, Philippe Mathieu-Daudé wrote: > Having the HMP/QMP commands defined in monitor.c makes the stubs > rather complicated when SEV is not built in. To simplify, move the > SEV functions to sev.c, and remove a layer of stubs. > > Also make it clearer when SEV is not built in, so developers don't > try to enable it when it is not enablable: > > - before: > > (qemu) info sev > SEV is not enabled > > - after: > > (qemu) info sev > SEV is not available in this QEMU > > Signed-off-by: Philippe Mathieu-Daudé > --- > target/i386/monitor.c | 96 -- > target/i386/sev-sysemu-stub.c | 29 +++ > target/i386/sev.c | 97 +++ Hi Philippe, I agree that the split from monitor.c makes it easier to follow. Instead of putting the QMP entry points in sev-sysemu-stub. and sev.c, what do you think of placing them in sev-qmp-stub.c and sev-qmp.c, respectively? I find that appealing from a code organization/module boundary perspective. Connor
Re: [PATCH 05/11] target/i386/sev_i386.h: Remove unused headers
On 6/10/21 1:45 AM, Philippe Mathieu-Daudé wrote: > Declarations don't require these headers, remove them. > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Connor Kuehl
Re: [PATCH 06/11] target/i386/sev: Remove sev_get_me_mask()
On 6/10/21 1:45 AM, Philippe Mathieu-Daudé wrote:
> -uint64_t
> -sev_get_me_mask(void)
> -{
> -return sev_guest ? sev_guest->me_mask : ~0;
> -}
> -
> uint32_t
> sev_get_cbit_position(void)
> {
> @@ -810,8 +803,6 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error
> **errp)
> goto err;
> }
>
> -sev->me_mask = ~(1UL << sev->cbitpos);
> -
> devname = object_property_get_str(OBJECT(sev), "sev-device", NULL);
> sev->sev_fd = open(devname, O_RDWR);
> if (sev->sev_fd < 0) {
>
Brijesh, do you remember if this was added with the intent that it would
be useful in a future series?
Otherwise:
Reviewed-by: Connor Kuehl
Re: [PATCH 07/11] target/i386/sev: Mark unreachable code with g_assert_not_reached()
On 6/10/21 1:45 AM, Philippe Mathieu-Daudé wrote: > The unique sev_encrypt_flash() invocation (in pc_system_flash_map) > is protected by the "if (sev_enabled())" check, so is not > reacheable. > Replace the abort() call in sev_es_save_reset_vector() by > g_assert_not_reached() which meaning is clearer. > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Connor Kuehl
Re: [PATCH 04/11] target/i386/cpu: Add missing 'qapi/error.h' header
On 6/10/21 1:45 AM, Philippe Mathieu-Daudé wrote:
> Commit 00b81053244 ("target-i386: Remove assert_no_error usage")
> forgot to add the "qapi/error.h", add it now.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Connor Kuehl
Re: [PATCH 03/11] target/i386/monitor: Return QMP error when SEV is disabled in build
On 6/10/21 1:45 AM, Philippe Mathieu-Daudé wrote:
> If the management layer tries to inject a secret, it gets an empty
> response in case the binary built without SEV:
>
> { "execute": "sev-inject-launch-secret",
> "arguments": { "packet-header": "mypkt", "secret": "mypass", "gpa":
> 4294959104 }
> }
> {
> "return": {
> }
> }
>
> Make it clearer by returning an error, mentioning the feature is
> disabled:
>
> { "execute": "sev-inject-launch-secret",
> "arguments": { "packet-header": "mypkt", "secret": "mypass", "gpa":
> 4294959104 }
> }
> {
> "error": {
> "class": "GenericError",
> "desc": "this feature or command is not currently supported"
> }
> }
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Connor Kuehl
Re: [PATCH] Add Connor Kuehl as reviewer for AMD SEV
On 6/8/21 3:45 PM, Daniel P. Berrangé wrote: >> Right, I am just worried that if I am the only person that shows up in >> the get_maintainer.pl output, the submitter will have to know some other >> way who a relevant maintainer is that can take the patches otherwise >> they won't be CC'd. Or we'll have to hope a relevant maintainer sees >> them on the list. Or I'll have to chase down a maintainer myself >> assuming the reviews all check out. :-) > > Well there's no real guarantee that any of the previous committers will > take the patch even if they are listed by get_maintainer. This is typical > with anything lacking a maintainer assigned. We typically hope that > whoever runs the "misc" queue sees the patch and picks it up, but often > it requires pings to remind someone to pick it up. > > The only real right answer here is to actually get someone as the > nominated maintainer. Every other scenario is a just a band aid and > is not a good experiance for contributors. A nominated reviewer is > usually hoped to be a stepping stone to someone becoming maintainer > in future, so in that sense the fact that only you will be cc'd is > sort of intentional :-) That makes perfect sense. I'll forge on ahead, then :-) Thanks! Connor
Re: [PATCH] Add Connor Kuehl as reviewer for AMD SEV
On 6/8/21 2:34 PM, Dr. David Alan Gilbert wrote: >> Note: because there's no maintainer entry, when running >> ./scripts/get_maintainers.pl on target/i386/sev.c, my name and the qemu >> mailing list is the only thing that shows up... it doesn't even show >> previous committers (as it would before applying this patch). Which is >> probably not great considering I do not make pull requests to QEMU. >> >> Is the way forward to get someone to sign up as a maintainer before >> applying a patch like this? > > If you wanted to do a submaintainer for it and send it to one of the x86 > maintainers rather than having to do full pulls? I'm not opposed to this. I think I have a few of the right people on CC, so let's see if they weigh in on this. Unless it means I have to manage a GPG key again... (just kidding, kind of...) Connor
Re: [PATCH] Add Connor Kuehl as reviewer for AMD SEV
On 6/8/21 3:10 PM, Daniel P. Berrangé wrote: > On Tue, Jun 08, 2021 at 02:25:37PM -0500, Connor Kuehl wrote: >> It may not be appropriate for me to take over as a maintainer at this time, >> but I would consider myself familiar with AMD SEV and what this code is >> meant to be doing as part of a VMM for launching SEV-protected guests. >> >> To that end, I would be happy to volunteer as a reviewer for SEV-related >> changes so that I am CC'd on them and can help share the review burden with >> whoever does maintain this code. >> >> Signed-off-by: Connor Kuehl >> --- >> Note: because there's no maintainer entry, when running >> ./scripts/get_maintainers.pl on target/i386/sev.c, my name and the qemu >> mailing list is the only thing that shows up... it doesn't even show >> previous committers (as it would before applying this patch). Which is >> probably not great considering I do not make pull requests to QEMU. >> >> Is the way forward to get someone to sign up as a maintainer before >> applying a patch like this? > > There's no requirement to have a maintainer before having a reviewer. > If any of the existing committers shown do send pull requests, it is > probably co-incidental since they're not listed as official maintainers, > and being listed as Reviewer doesn't commit you to doing pull requests. > > That said if you're the only nominated reviewer and actually do useful > reviews, you will probably quickly find yourself the defacto maintainer > in 12 months time and end up doing pull requests... Right, I am just worried that if I am the only person that shows up in the get_maintainer.pl output, the submitter will have to know some other way who a relevant maintainer is that can take the patches otherwise they won't be CC'd. Or we'll have to hope a relevant maintainer sees them on the list. Or I'll have to chase down a maintainer myself assuming the reviews all check out. :-) Connor
Re: [PATCH] Add Connor Kuehl as reviewer for AMD SEV
On 6/8/21 2:25 PM, Connor Kuehl wrote: > It may not be appropriate for me to take over as a maintainer at this time, > but I would consider myself familiar with AMD SEV and what this code is > meant to be doing as part of a VMM for launching SEV-protected guests. > > To that end, I would be happy to volunteer as a reviewer for SEV-related > changes so that I am CC'd on them and can help share the review burden with > whoever does maintain this code. > > Signed-off-by: Connor Kuehl > --- > Note: because there's no maintainer entry, when running > ./scripts/get_maintainers.pl on target/i386/sev.c, my name and the qemu > mailing list is the only thing that shows up... it doesn't even show > previous committers (as it would before applying this patch). Which is > probably not great considering I do not make pull requests to QEMU. > > Is the way forward to get someone to sign up as a maintainer before > applying a patch like this? I need to resend this patch since I realized I forgot to add target/i386/sev_i386.h, and target/i386/sev-stub.c, but I still am wondering about the answer to the question above. Connor
[PATCH] Add Connor Kuehl as reviewer for AMD SEV
It may not be appropriate for me to take over as a maintainer at this time, but I would consider myself familiar with AMD SEV and what this code is meant to be doing as part of a VMM for launching SEV-protected guests. To that end, I would be happy to volunteer as a reviewer for SEV-related changes so that I am CC'd on them and can help share the review burden with whoever does maintain this code. Signed-off-by: Connor Kuehl --- Note: because there's no maintainer entry, when running ./scripts/get_maintainers.pl on target/i386/sev.c, my name and the qemu mailing list is the only thing that shows up... it doesn't even show previous committers (as it would before applying this patch). Which is probably not great considering I do not make pull requests to QEMU. Is the way forward to get someone to sign up as a maintainer before applying a patch like this? MAINTAINERS | 4 1 file changed, 4 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 7d9cd29042..3eb7ce8fc6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2938,6 +2938,10 @@ F: hw/core/clock-vmstate.c F: hw/core/qdev-clock.c F: docs/devel/clocks.rst +AMD Secure Encrypted Virtualization +R: Connor Kuehl +F: target/i386/sev.c + Usermode Emulation -- Overall usermode emulation -- 2.31.1
Re: [Virtio-fs] [PATCH 0/9] virtiofsd: Allow using file handles instead of O_PATH FDs
On 6/4/21 11:13 AM, Max Reitz wrote: > Hi, > > This is the C virtiofsd equivalent to > https://gitlab.com/virtio-fs/virtiofsd-rs/-/merge_requests/26. As such, > the summary is pretty much the same: > > Storing an O_PATH file descriptor in every lo_inode object means we have > a lot of FDs open, which is sometimes bad. This series adds an option > (-o inode_file_handles) that will make virtiofsd attempt to generate a > file handle for new inodes and store that instead of an FD. When an FD > is needed for a given inode, we open the handle. > > To accomplish this, lo_inode.fd is should not be accessed directly > anymore, but only through helper functions (mainly lo_inode_fd() and > lo_inode_open()). A TempFd object is added to hide the difference > between FDs that are bound to the lo_inode object (and so need not be > closed after use) and temporary FDs from open_by_handle_at() (which do > need to be closed after use). > > To prevent the problem I spent a long time talking about (if we don’t > have an FD open for every inode, the inode can be deleted, its ID > reused, and then the lookup in lo_data.inodes will find the old deleted > inode), patch 7 adds a second hash table lo_data.inodes_by_handle that > maps file handles to lo_inode objects. (Because file handles include a > generation ID, so we can discern between the old and the new inode.) > > Patch 9 is completely optional, but I just really felt compelled to > write it. > > > Max Reitz (9): > virtiofsd: Add TempFd structure > virtiofsd: Use lo_inode_open() instead of openat() > virtiofsd: Add lo_inode_fd() helper > virtiofsd: Let lo_fd() return a TempFd > virtiofsd: Let lo_inode_open() return a TempFd > virtiofsd: Add lo_inode.fhandle > virtiofsd: Add inodes_by_handle hash table > virtiofsd: Optionally fill lo_inode.fhandle > virtiofsd: Add lazy lo_do_find() > > tools/virtiofsd/helper.c | 3 + > tools/virtiofsd/passthrough_ll.c | 809 +- > tools/virtiofsd/passthrough_seccomp.c | 2 + > 3 files changed, 667 insertions(+), 147 deletions(-) > For the series: Reviewed-by: Connor Kuehl
Re: [PATCH] sev: sev_get_attestation_report use g_autofree
On 6/3/21 6:30 AM, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" > > Removes a whole bunch of g_free's and a goto. > > Signed-off-by: Dr. David Alan Gilbert Reviewed-by: Connor Kuehl
Re: [PATCH v2 1/3] doc: Fix some mistakes in the SEV documentation
On 4/23/21 3:08 PM, Tom Lendacky wrote: > From: Tom Lendacky > > Fix some spelling and grammar mistakes in the amd-memory-encryption.txt > file. No new information added. > > Signed-off-by: Tom Lendacky For the series: Reviewed-by: Connor Kuehl
Re: [Virtio-fs] [PATCH 7/7] virtiofsd: Set req->reply_sent right after sending reply
On 5/11/21 4:37 PM, Vivek Goyal wrote:
> There is no reason to set it in label "err". We should be able to set
> it right after sending reply. It is easier to read.
>
> Signed-off-by: Vivek Goyal
> ---
> tools/virtiofsd/fuse_virtio.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index aa53808ef9..b1767dd5b9 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -446,12 +446,9 @@ int virtio_send_data_iov(struct fuse_session *se, struct
> fuse_chan *ch,
> vu_queue_notify(dev, q);
> pthread_mutex_unlock(>vq_lock);
> vu_dispatch_unlock(qi->virtio_dev);
> +req->reply_sent = true;
>
> err:
Just a really minor comment: after all these changes, I would venture
that "out" is a more appropriate label name than "err" at this point.
> -if (ret == 0) {
> -req->reply_sent = true;
> -}
> -
> return ret;
> }
>
>
Re: [Virtio-fs] [PATCH 0/7] virtiofsd: Few cleanups in virtio_send_data_iov()
On 5/11/21 4:37 PM, Vivek Goyal wrote: > Hi, > > Code in virtio_send_data_iov() little twisted and complicated. This > patch series just tries to simplify it a bit to make it little easier > to read this piece of code. > > Thanks > Vivek > > Vivek Goyal (7): > virtiofsd: Check for EINTR in preadv() and retry > virtiofsd: Get rid of unreachable code in read > virtiofsd: Use iov_discard_front() to skip bytes > virtiofsd: get rid of in_sg_left variable > virtiofsd: Simplify skip byte logic > virtiofsd: Check EOF before short read > virtiofsd: Set req->reply_sent right after sending reply > > tools/virtiofsd/fuse_virtio.c | 67 +++ > 1 file changed, 21 insertions(+), 46 deletions(-) > With the codestyle fix to appease the bot: Reviewed-by: Connor Kuehl (For the series)
Re: [PATCH 1/3] virtiofsd: Find original inode ID of mount points
On 5/12/21 7:55 AM, Max Reitz wrote: > Mount point directories represent two inodes: On one hand, they are a > normal directory on their parent filesystem. On the other, they are > the > root node of the filesystem mounted there. Thus, they have two inode > IDs. > > Right now, we only report the latter inode ID (i.e. the inode ID of > the > mounted filesystem's root node). This is fine once the guest has > auto-mounted a submount there (so this inode ID goes with a device ID > that is distinct from the parent filesystem), but before the > auto-mount, > they have the device ID of the parent and the inode ID for the > submount. > This is problematic because this is likely exactly the same > st_dev/st_ino combination as the parent filesystem's root node. This > leads to problems for example with `find`, which will thus complain > about a filesystem loop if it has visited the parent filesystem's root > node before, and then refuse to descend into the submount. > > There is a way to find the mount directory's original inode ID, and > that > is to readdir(3) the parent directory, look for the mount directory, > and > read the dirent.d_ino field. Using this, we can let lookup and > readdirplus return that original inode ID, which the guest will thus > show until the submount is auto-mounted. (Then, it will invoke > getattr > and that stat(2) call will return the inode ID for the submount.) > > Signed-off-by: Max Reitz > --- This is a clever way of uncovering the inode ID. Reviewed-by: Connor Kuehl
Re: [PATCH] 9pfs: add link to 9p developer docs
On 5/6/21 8:12 AM, Christian Schoenebeck wrote: > To lower the entry level for new developers, add a link to the > 9p developer docs (i.e. qemu wiki) at the beginning of 9p source > files, that is to: https://wiki.qemu.org/Documentation/9p > > Signed-off-by: Christian Schoenebeck > --- > hw/9pfs/9p-local.c | 5 + > hw/9pfs/9p-posix-acl.c | 5 + > hw/9pfs/9p-proxy.c | 5 + > hw/9pfs/9p-synth.c | 5 + > hw/9pfs/9p-util.c | 5 + > hw/9pfs/9p-xattr-user.c| 5 + > hw/9pfs/9p-xattr.c | 5 + > hw/9pfs/9p.c | 5 + > hw/9pfs/codir.c| 5 + > hw/9pfs/cofile.c | 5 + > hw/9pfs/cofs.c | 5 + > hw/9pfs/coth.c | 5 + > hw/9pfs/coxattr.c | 5 + > hw/9pfs/virtio-9p-device.c | 5 + > hw/9pfs/xen-9p-backend.c | 5 + > tests/qtest/libqos/virtio-9p.c | 5 + > tests/qtest/virtio-9p-test.c | 5 + Would it be helpful to also add this link to the virtio-9p stanza in MAINTAINERS? Something like: W: https://wiki.qemu.org/Documentation/9p Connor
Re: [PATCH] docs: add table of contents to QAPI references
On 5/11/21 4:25 AM, Daniel P. Berrangé wrote: > The QAPI reference docs for the guest agent, storage daemon and QMP are > all rather long and hard to navigate unless you already know the name of > the command and can do full text search for it. > > A table of contents in each doc will help people locate stuff much more > easily. > > Signed-off-by: Daniel P. Berrangé > --- This looks so much better! Reviewed-by: Connor Kuehl
Re: [PATCH 0/2] net/tap: minor fixes to interaction with the bridge helper
On 5/5/21 7:12 AM, Daniel P. Berrangé wrote: > QEMU fails to report a good error message if the bridge helper exits > with success code, but forgets to pass back an FD. > > There is also a minor portability problem impacting FreeBSD/NetBSD > dealing with cmsg initialization. > > Daniel P. Berrangé (2): > net/tap: fix FreeBSD portability problem receiving TAP FD > net/tap: fix error reporting when bridge helper forgets to send an FD > > net/tap.c | 34 +++--- > 1 file changed, 23 insertions(+), 11 deletions(-) > Just a minor question on patch 2. Otherwise, for the series: Reviewed-by: Connor Kuehl
Re: [PATCH 2/2] net/tap: fix error reporting when bridge helper forgets to send an FD
On 5/5/21 7:12 AM, Daniel P. Berrangé wrote:
> [..]
> The recv_fd() method returns -1 on error, 0 on end of file, or an FD
> $ qemu-system-x86_64 -netdev bridge,br=br99,helper=/bin/true,id=ns0
> qemu-system-x86_64: -netdev bridge,br=br99,helper=/bin/true,id=ns0:
> bridge helper did not send a file descriptor
> [..]
> }
> +
> +/*
> + * ret == 0 means EOF, and if status == 0 then helper
> + * exited cleanly but forgot to send us an FD. Opps...
> + */
> +if (ret == 0) {
> +error_setg(errp, "bridge helper did not send a file
> descriptor");
Is it possible to include the name of the helper that's at fault here?
Could be helpful for debugging if the command line includes multiple
netdev opts with a helper parameter (unless that's literally impossible).
Connor
Re: [PATCH v3] target/i386/sev: add support to query the attestation report
On 4/29/21 12:07 PM, Brijesh Singh wrote: > The SEV FW >= 0.23 added a new command that can be used to query the > attestation report containing the SHA-256 digest of the guest memory > and VMSA encrypted with the LAUNCH_UPDATE and sign it with the PEK. > > Note, we already have a command (LAUNCH_MEASURE) that can be used to > query the SHA-256 digest of the guest memory encrypted through the > LAUNCH_UPDATE. The main difference between previous and this command > is that the report is signed with the PEK and unlike the LAUNCH_MEASURE > command the ATTESATION_REPORT command can be called while the guest typo: 'ATTESATION_REPORT' > is running. > > Add a QMP interface "query-sev-attestation-report" that can be used > to get the report encoded in base64. > > Cc: James Bottomley > Cc: Tom Lendacky > Cc: Eric Blake > Cc: Paolo Bonzini > Cc: k...@vger.kernel.org > Reviewed-by: James Bottomley > Tested-by: James Bottomley > Signed-off-by: Brijesh Singh Looks good to me! Reviewed-by: Connor Kuehl
[PATCH v3] Document qemu-img options data_file and data_file_raw
The contents of this patch were initially developed and posted by Han Han[1], however, it appears the original patch was not applied. Since then, the relevant documentation has been moved and adapted to a new format. I've taken most of the original wording and tweaked it according to some of the feedback from the original patch submission. I've also adapted it to restructured text, which is the format the documentation currently uses. [1] https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg01253.html Fixes: https://bugzilla.redhat.com/1763105 Signed-off-by: Han Han Suggested-by: Max Reitz [ Max: provided description of data_file_raw behavior ] Signed-off-by: Connor Kuehl --- John, my apologies, I failed to CC you on my last revision (v2) where I addressed your comments. Changes since v2: * Pulled in Max's explanation of data_file_raw behaviors with some minor wording tweaks. Changes since v1: * Clarify different behaviors with these options when using qemu-img create vs amend (Max) * Touch on the negative case of how the file becomes inconsistent (John) docs/tools/qemu-img.rst | 31 +++ 1 file changed, 31 insertions(+) diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index c9efcfaefc..cfe1147879 100644 --- a/docs/tools/qemu-img.rst +++ b/docs/tools/qemu-img.rst @@ -866,6 +866,37 @@ Supported image file formats: issue ``lsattr filename`` to check if the NOCOW flag is set or not (Capital 'C' is NOCOW flag). + ``data_file`` +Filename where all guest data will be stored. If this option is used, +the qcow2 file will only contain the image's metadata. + +Note: Data loss will occur if the given filename already exists when +using this option with ``qemu-img create`` since ``qemu-img`` will create +the data file anew, overwriting the file's original contents. To simply +update the reference to point to the given pre-existing file, use +``qemu-img amend``. + + ``data_file_raw`` +If this option is set to ``on``, QEMU will always keep the external data +file consistent as a standalone read-only raw image. + +It does this by forwarding all write accesses to the qcow2 file through to +the raw data file, including their offsets. Therefore, data that is visible +on the qcow2 node (i.e., to the guest) at some offset is visible at the same +offset in the raw data file. This results in a read-only raw image. Writes +that bypass the qcow2 metadata may corrupt the qcow2 metadata because the +out-of-band writes may result in the metadata falling out of sync with the +raw image. + +If this option is ``off``, QEMU will use the data file to store data in an +arbitrary manner. The file’s content will not make sense without the +accompanying qcow2 metadata. Where data is written will have no relation to +its offset as seen by the guest, and some writes (specifically zero writes) +may not be forwarded to the data file at all, but will only be handled by +modifying qcow2 metadata. + +This option can only be enabled if ``data_file`` is set. + ``Other`` QEMU also supports various other image file formats for -- 2.30.2
Re: [PATCH v2] Document qemu-img options data_file and data_file_raw
On 4/30/21 9:45 AM, Max Reitz wrote: >> + ``data_file_raw`` >> +If this option is set to ``on``, QEMU will always keep the external >> +data file consistent as a standalone read-only raw image. It does >> +this by forwarding updates through to the raw image in addition to >> +updating the image metadata. If set to ``off``, QEMU will only >> +update the image metadata without forwarding the changes through >> +to the raw image. The default value is ``off``. > > Hm, what updates and what changes? I mean, the first part makes sense (the > “It does this by...”), but the second part doesn’t. qemu will still forward > most writes to the data file. (Not all, but most.) > > (Also, nit pick: With data_file_raw=off, the data file is not a raw image. > (You still call it that in the penultimate sentence.)) > When you write data to a qcow2 file with data_file, the data also goes to the > data_file, most of the time. The exception is when it can be handled with a > metadata update, i.e. when it's a zero write or discard. > > In addition, such updates (i.e. zero writes, I presume) not happening to the > data file are usually a minor problem. The real problem is that without > data_file_raw, data clusters can be allocated anywhere in the data file, > whereas with data_file_raw, they are allocated at their respective guest > offset (i.e. the host offset always equals the guest offset). > > I personally would have been fine with the first sentence, but if we want > more of an explanation... Perhaps: > > < > If this option is set to ``on``, QEMU will always keep the external data file > consistent as a standalone read-only raw image. > > It does this by effectively forwarding all write accesses that happen to the > qcow2 file to the raw data file, including their offsets. Therefore, data > that is visible on the qcow2 node (i.e., to the guest) at some offset is > visible at the same offset in the raw data file. > > If this option is ``off``, QEMU will use the data file just to store data in > an effectively arbitrary manner. The file’s content will not make sense > without the accompanying qcow2 metadata. Where data is written will have no > relation to its offset as seen by the guest, and some writes (specifically > zero writes) may not be forwarded to the data file at all, but will only be > handled by modifying qcow2 metadata. > > In short: With data_file_raw, the data file reads as a valid raw VM image > file. Without it, its content can only be interpreted by reading the > accompanying qcow2 metadata. > > Note that this option only makes the data file valid as a read-only raw > image. You should not write to it, as this may effectively corrupt the qcow2 > metadata (for example, dirty bitmaps may become out of sync). > > EOF > > This got longer than I wanted it to be. Hm. Anyway, what do you think? I found it very helpful. I'll incorporate your explanation into the next revision. I'm wondering what the most appropriate trailer would be for the next revision? Suggested-by: Max [..] Co-developed-by: Max [..] Let me know if you have a strong preference, otherwise I'll go with Suggested-by: Thank you, Connor
[PATCH 1/2] [RESEND] sev: use explicit indices for mapping firmware error codes to strings
This can help lower any margin for error when making future additions to
the list, especially if they're made out of order.
While doing so, make capitalization of ASID consistent with its usage in
the SEV firmware spec (Asid -> ASID).
Signed-off-by: Connor Kuehl
Reviewed-by: Philippe Mathieu-Daudé
---
target/i386/sev.c | 46 +++---
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 72b9e2ab40..9e2e47012f 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -88,29 +88,29 @@ static SevGuestState *sev_guest;
static Error *sev_mig_blocker;
static const char *const sev_fw_errlist[] = {
-"",
-"Platform state is invalid",
-"Guest state is invalid",
-"Platform configuration is invalid",
-"Buffer too small",
-"Platform is already owned",
-"Certificate is invalid",
-"Policy is not allowed",
-"Guest is not active",
-"Invalid address",
-"Bad signature",
-"Bad measurement",
-"Asid is already owned",
-"Invalid ASID",
-"WBINVD is required",
-"DF_FLUSH is required",
-"Guest handle is invalid",
-"Invalid command",
-"Guest is active",
-"Hardware error",
-"Hardware unsafe",
-"Feature not supported",
-"Invalid parameter"
+[SEV_RET_SUCCESS]= "",
+[SEV_RET_INVALID_PLATFORM_STATE] = "Platform state is invalid",
+[SEV_RET_INVALID_GUEST_STATE]= "Guest state is invalid",
+[SEV_RET_INAVLID_CONFIG] = "Platform configuration is invalid",
+[SEV_RET_INVALID_LEN]= "Buffer too small",
+[SEV_RET_ALREADY_OWNED] = "Platform is already owned",
+[SEV_RET_INVALID_CERTIFICATE]= "Certificate is invalid",
+[SEV_RET_POLICY_FAILURE] = "Policy is not allowed",
+[SEV_RET_INACTIVE] = "Guest is not active",
+[SEV_RET_INVALID_ADDRESS]= "Invalid address",
+[SEV_RET_BAD_SIGNATURE] = "Bad signature",
+[SEV_RET_BAD_MEASUREMENT]= "Bad measurement",
+[SEV_RET_ASID_OWNED] = "ASID is already owned",
+[SEV_RET_INVALID_ASID] = "Invalid ASID",
+[SEV_RET_WBINVD_REQUIRED]= "WBINVD is required",
+[SEV_RET_DFFLUSH_REQUIRED] = "DF_FLUSH is required",
+[SEV_RET_INVALID_GUEST] = "Guest handle is invalid",
+[SEV_RET_INVALID_COMMAND]= "Invalid command",
+[SEV_RET_ACTIVE] = "Guest is active",
+[SEV_RET_HWSEV_RET_PLATFORM] = "Hardware error",
+[SEV_RET_HWSEV_RET_UNSAFE] = "Hardware unsafe",
+[SEV_RET_UNSUPPORTED]= "Feature not supported",
+[SEV_RET_INVALID_PARAM] = "Invalid parameter",
};
#define SEV_FW_MAX_ERROR ARRAY_SIZE(sev_fw_errlist)
--
2.30.2
[PATCH 0/2] [RESEND] SEV firmware error list touchups
Connor Kuehl (2): sev: use explicit indices for mapping firmware error codes to strings sev: add missing firmware error conditions target/i386/sev.c | 48 --- 1 file changed, 25 insertions(+), 23 deletions(-) -- 2.30.2
[PATCH v2] Document qemu-img options data_file and data_file_raw
The contents of this patch were initially developed and posted by Han Han[1], however, it appears the original patch was not applied. Since then, the relevant documentation has been moved and adapted to a new format. I've taken most of the original wording and tweaked it according to some of the feedback from the original patch submission. I've also adapted it to restructured text, which is the format the documentation currently uses. [1] https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg01253.html Fixes: https://bugzilla.redhat.com/1763105 Signed-off-by: Han Han Signed-off-by: Connor Kuehl --- Changes since v1: * Clarify different behaviors with these options when using qemu-img create vs amend (Max) * Touch on the negative case of how the file becomes inconsistent (John) docs/tools/qemu-img.rst | 20 1 file changed, 20 insertions(+) diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index c9efcfaefc..87b4a65535 100644 --- a/docs/tools/qemu-img.rst +++ b/docs/tools/qemu-img.rst @@ -866,6 +866,26 @@ Supported image file formats: issue ``lsattr filename`` to check if the NOCOW flag is set or not (Capital 'C' is NOCOW flag). + ``data_file`` +Filename where all guest data will be stored. If this option is used, +the qcow2 file will only contain the image's metadata. + +Note: Data loss will occur if the given filename already exists when +using this option with ``qemu-img create`` since ``qemu-img`` will create +the data file anew, overwriting the file's original contents. To simply +update the reference to point to the given pre-existing file, use +``qemu-img amend``. + + ``data_file_raw`` +If this option is set to ``on``, QEMU will always keep the external +data file consistent as a standalone read-only raw image. It does +this by forwarding updates through to the raw image in addition to +updating the image metadata. If set to ``off``, QEMU will only +update the image metadata without forwarding the changes through +to the raw image. The default value is ``off``. + +This option can only be enabled if ``data_file`` is set. + ``Other`` QEMU also supports various other image file formats for -- 2.30.2
[PATCH 2/2] [RESEND] sev: add missing firmware error conditions
The SEV userspace header[1] exports a couple of other error conditions that
aren't listed in QEMU's SEV implementation, so let's just round out the
list.
[1] linux-headers/linux/psp-sev.h
Signed-off-by: Connor Kuehl
Reviewed-by: Philippe Mathieu-Daudé
---
target/i386/sev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 9e2e47012f..dfafd3b543 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -111,6 +111,8 @@ static const char *const sev_fw_errlist[] = {
[SEV_RET_HWSEV_RET_UNSAFE] = "Hardware unsafe",
[SEV_RET_UNSUPPORTED]= "Feature not supported",
[SEV_RET_INVALID_PARAM] = "Invalid parameter",
+[SEV_RET_RESOURCE_LIMIT] = "Required firmware resource depleted",
+[SEV_RET_SECURE_DATA_INVALID]= "Part-specific integrity check failure",
};
#define SEV_FW_MAX_ERROR ARRAY_SIZE(sev_fw_errlist)
--
2.30.2
Re: [PATCH v2 1/2] meson: Select 'have_system' when virtiofsd is enabled
On 4/29/21 3:33 AM, Philippe Mathieu-Daudé wrote:
> When not explicitly select a sysemu target and building virtiofsd,
> the seccomp/cap-ng libraries are not resolved, leading to this error:
>
> $ configure --target-list=i386-linux-user --disable-tools --enable-virtiofsd
> tools/meson.build:12:6: ERROR: Problem encountered: virtiofsd requires
> libcap-ng-devel and seccomp-devel
>
> Fix by enabling sysemu (have_system) when virtiofsd is built.
>
> Reported-by: Mahmoud Mandour
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> meson.build | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/meson.build b/meson.build
> index c6f4b0cf5e8..f858935ad95 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -51,6 +51,8 @@
>have_system = have_system or target.endswith('-softmmu')
> endforeach
> have_tools = 'CONFIG_TOOLS' in config_host
> +# virtiofsd depends on sysemu
> +have_system = have_system or not get_option('virtiofsd').disabled()
I don't think we should satisfy virtiofsd dependencies transiently by
depending on system emulation targets.
It's my understanding (and I'm happy to be corrected on this) that the
virtiofsd binary is orthogonal to system emulation tools. Consider a
situation in which someone wants to develop virtiofsd but doesn't want
to wait for the rest of QEMU to build and instead use their own
qemu-system-x86_64 installed by their distro.
Connor
> have_block = have_system or have_tools
>
> python = import('python').find_installation()
>
Re: [PATCH] tools/meson.build: Error on enabling virtiofsd and have_system is false
On 4/28/21 9:13 AM, Mahmoud Mandour wrote: >> I am not entirely sure if this is true. The error message before this >> patch is applied is: >> >> ../tools/meson.build:12:6: ERROR: Problem encountered: virtiofsd >> requires libcap-ng-devel and seccomp-devel >> >> From what I know about virtiofsd, I know it definitely depends on those >> two things. >> >> Is it possible that the error here is that the top-level meson.build is >> not properly collecting the seccomp and libcap-ng dependencies if the >> configure invocation doesn't require a system emulation target? > I also thought that this is the case since I also specifically get this > error message > if I enable virtiofsd and specify a target list with only Linux-user > targets while nothing > in tools/meson.build specifies so. But I think that even if it correctly > managed the > dependencies it would include and build virtiofsd unnecessarily and that's > not what we want(?) I think that's exactly what we want for the default case because virtiofsd is enabled by default (../configure --help). Even if the virtiofsd dependencies are satisfied, if one doesn't want to build virtiofsd, they can pass --disable-virtiofsd to the configure invocation. Connor
Re: [PATCH] tools/meson.build: Error on enabling virtiofsd and have_system is false
On 4/28/21 8:35 AM, Mahmoud Mandour wrote:
> Previously, on configuring with --enable-virtiofsd and specifying
> a target list that does not contain a full-system emulation target,
> a spurious error message is emitted. This patch introduces a
> meaningful error message for such case.
>
> Signed-off-by: Mahmoud Mandour
> ---
> tools/meson.build | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/meson.build b/tools/meson.build
> index 3e5a0abfa2..f6a4ced2f4 100644
> --- a/tools/meson.build
> +++ b/tools/meson.build
> @@ -5,7 +5,9 @@ have_virtiofsd = (targetos == 'linux' and
> 'CONFIG_VHOST_USER' in config_host)
>
> if get_option('virtiofsd').enabled()
> - if not have_virtiofsd
> + if not have_system
> +error('virtiofsd requires full-system emulation target(s)')
I am not entirely sure if this is true. The error message before this
patch is applied is:
../tools/meson.build:12:6: ERROR: Problem encountered: virtiofsd
requires libcap-ng-devel and seccomp-devel
>From what I know about virtiofsd, I know it definitely depends on those
two things.
Is it possible that the error here is that the top-level meson.build is
not properly collecting the seccomp and libcap-ng dependencies if the
configure invocation doesn't require a system emulation target?
Thanks,
Connor
> + elif not have_virtiofsd
> if targetos != 'linux'
>error('virtiofsd requires Linux')
> elif not seccomp.found() or not libcap_ng.found()
>
[PATCH v4 1/2] iotests/231: Update expected deprecation message
The deprecation message in the expected output has technically been
wrong since the wrong version of a patch was applied to it. Because of
this, the test fails. Correct the expected output so that it passes.
Signed-off-by: Connor Kuehl
Reviewed-by: Max Reitz
Reviewed-by: Stefano Garzarella
---
v3 -> v4:
* Added Stefano's s-o-b to the commit message
tests/qemu-iotests/231.out | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 579ba11c16..747dd221bb 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -1,9 +1,7 @@
QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
+qemu-img: warning: RBD options encoded in the filename as keyvalue pairs is
deprecated
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
*** done
--
2.30.2
[PATCH v4 2/2] block/rbd: Add an escape-aware strchr helper
Sometimes the parser needs to further split a token it has collected
from the token input stream. Right now, it does a cursory check to see
if the relevant characters appear in the token to determine if it should
break it down further.
However, qemu_rbd_next_tok() will escape characters as it removes tokens
from the token stream and plain strchr() won't. This can make the
initial strchr() check slightly misleading since it implies
qemu_rbd_next_tok() will find the token and split on it, except the
reality is that qemu_rbd_next_tok() will pass over it if it is escaped.
Use a custom strchr to avoid mixing escaped and unescaped string
operations. Furthermore, this code is identical to how
qemu_rbd_next_tok() seeks its next token, so incorporate this custom
strchr into the body of that function to reduce duplication.
Reported-by: Han Han
Fixes: https://bugzilla.redhat.com/1873913
Signed-off-by: Connor Kuehl
---
v3 -> v4:
* Replace qemu_rbd_next_tok() seek loop with qemu_rbd_strchr() since
they're identical
block/rbd.c| 32 +---
tests/qemu-iotests/231 | 4
tests/qemu-iotests/231.out | 3 +++
3 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index f098a89c7b..26f64cce7c 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -113,21 +113,31 @@ static int qemu_rbd_connect(rados_t *cluster,
rados_ioctx_t *io_ctx,
const char *keypairs, const char *secretid,
Error **errp);
+static char *qemu_rbd_strchr(char *src, char delim)
+{
+char *p;
+
+for (p = src; *p; ++p) {
+if (*p == delim) {
+return p;
+}
+if (*p == '\\' && p[1] != '\0') {
+++p;
+}
+}
+
+return NULL;
+}
+
+
static char *qemu_rbd_next_tok(char *src, char delim, char **p)
{
char *end;
*p = NULL;
-for (end = src; *end; ++end) {
-if (*end == delim) {
-break;
-}
-if (*end == '\\' && end[1] != '\0') {
-end++;
-}
-}
-if (*end == delim) {
+end = qemu_rbd_strchr(src, delim);
+if (end) {
*p = end + 1;
*end = '\0';
}
@@ -171,7 +181,7 @@ static void qemu_rbd_parse_filename(const char *filename,
QDict *options,
qemu_rbd_unescape(found_str);
qdict_put_str(options, "pool", found_str);
-if (strchr(p, '@')) {
+if (qemu_rbd_strchr(p, '@')) {
image_name = qemu_rbd_next_tok(p, '@', );
found_str = qemu_rbd_next_tok(p, ':', );
@@ -181,7 +191,7 @@ static void qemu_rbd_parse_filename(const char *filename,
QDict *options,
image_name = qemu_rbd_next_tok(p, ':', );
}
/* Check for namespace in the image_name */
-if (strchr(image_name, '/')) {
+if (qemu_rbd_strchr(image_name, '/')) {
found_str = qemu_rbd_next_tok(image_name, '/', _name);
qemu_rbd_unescape(found_str);
qdict_put_str(options, "namespace", found_str);
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
index 0f66d0ca36..8e6c6447c1 100755
--- a/tests/qemu-iotests/231
+++ b/tests/qemu-iotests/231
@@ -55,6 +55,10 @@ _filter_conf()
$QEMU_IMG info
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}"
2>&1 | _filter_conf
$QEMU_IMG info
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
2>&1 | _filter_conf
+# Regression test: the qemu-img invocation is expected to fail, but it should
+# not seg fault the parser.
+$QEMU_IMG create "rbd:rbd/aa\/bb:conf=${BOGUS_CONF}" 1M 2>&1 | _filter_conf
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 747dd221bb..a785a6e859 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -4,4 +4,7 @@ unable to get monitor info from DNS SRV with service name:
ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
+Formatting 'rbd:rbd/aa\/bb:conf=BOGUS_CONF', fmt=raw size=1048576
+unable to get monitor info from DNS SRV with service name: ceph-mon
+qemu-img: rbd:rbd/aa\/bb:conf=BOGUS_CONF: error connecting: No such file or
directory
*** done
--
2.30.2
[PATCH v4 0/2] Fix segfault in qemu_rbd_parse_filename
Connor Kuehl (2): iotests/231: Update expected deprecation message block/rbd: Add an escape-aware strchr helper block/rbd.c| 32 +--- tests/qemu-iotests/231 | 4 tests/qemu-iotests/231.out | 7 --- 3 files changed, 29 insertions(+), 14 deletions(-) -- 2.30.2
Re: [PATCH v3 2/2] block/rbd: Add an escape-aware strchr helper
On 4/21/21 6:04 AM, Stefano Garzarella wrote:
>> +static char *qemu_rbd_strchr(char *src, char delim)
>> +{
>> +char *p;
>> +
>> +for (p = src; *p; ++p) {
>> +if (*p == delim) {
>> +return p;
>> +}
>> +if (*p == '\\' && p[1] != '\0') {
>> +++p;
>> +}
>> +}
>> +
>> +return NULL;
>> +}
>> +
>
> IIUC this is similar to the code used in qemu_rbd_next_tok().
> To avoid code duplication can we use this new function inside it?
Hi Stefano! Good catch. I think you're right. I've incorporated your
suggestion into my next revision. The only thing I changed was the
if-statement from:
if (end && *end == delim) {
to:
if (end) {
Since qemu_rbd_strchr() will only ever return a non-NULL pointer if it
was able to find 'delim'.
Connor
>
> I mean something like this (not tested):
>
> diff --git a/block/rbd.c b/block/rbd.c
> index f098a89c7b..eb6a839362 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -119,15 +119,8 @@ static char *qemu_rbd_next_tok(char *src, char delim,
> char **p)
>
> *p = NULL;
>
> -for (end = src; *end; ++end) {
> -if (*end == delim) {
> -break;
> -}
> -if (*end == '\\' && end[1] != '\0') {
> -end++;
> -}
> -}
> -if (*end == delim) {
> +end = qemu_rbd_strchr(src, delim);
> +if (end && *end == delim) {
> *p = end + 1;
> *end = '\0';
> }
>
>
> The rest LGTM!
>
> Thanks for fixing this issue,
> Stefano
>
Re: [Virtio-fs] [PATCH 2/2] virtiofsd: Add help for -o xattr-mapping
On 4/19/21 2:07 PM, Vivek Goyal wrote: >> This is a helpful note, but it doesn't tell the whole story. I think >> it'd be helpful to add one last note to this option which is to >> recommend reading the virtiofsd(1) man-page for more information on >> xattrmap rules. > > Is there a virtiofsd man page as well? All I see is > docs/tools/virtiofsd.rst. Yes, it's generated from that file. Should be located in qemu/build/docs/virtiofsd.1 after building QEMU. Connor
Re: [PATCH 0/5] block, migration: improve debugging of migration bdrv_flush failure
On 4/15/21 8:58 AM, Daniel P. Berrangé wrote: I spent a while debugging a tricky migration failure today which was ultimately caused by fdatasync() getting EACCESS. The existing probes were not sufficient to diagnose this, so I had to resort to GDB. This improves probes and block error reporting to make future diagnosis possible without GDB. Daniel P. Berrangé (5): migration: add trace point when vm_stop_force_state fails softmmu: add trace point when bdrv_flush_all fails block: preserve errno from fdatasync failures block: add trace point when fdatasync fails block: remove duplicate trace.h include block/file-posix.c | 10 +- block/trace-events | 1 + migration/migration.c | 1 + migration/trace-events | 1 + softmmu/cpus.c | 7 ++- softmmu/trace-events | 3 +++ 6 files changed, 17 insertions(+), 6 deletions(-) For the series: Reviewed-by: Connor Kuehl
Re: [PATCH 0/2] virtiofsd: Enable xattr if xattrmap is used
On Wed Apr 14, 2021 at 3:12 PM CDT, Carlos Venegas wrote: > > Using xattrmap for Kata Containers we found that xattr is should be used > or xattrmap wont work. These patches enable xattr when -o xattrmap is > used. Also, they add help for the xattrmap option on `virtiofsd --help` > output. > > Carlos Venegas (2): > virtiofsd: Allow use "-o xattrmap" without "-o xattr" > virtiofsd: Add help for -o xattr-mapping Good usability improvement. For the series: Reviewed-by: Connor Kuehl
Re: [Virtio-fs] [PATCH 2/2] virtiofsd: Add help for -o xattr-mapping
On Wed Apr 14, 2021 at 3:12 PM CDT, Carlos Venegas wrote: > The option is not documented in help. > > Add small help about the option. > > Signed-off-by: Carlos Venegas > --- > tools/virtiofsd/helper.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c > index 28243b51b2..5e98ed702b 100644 > --- a/tools/virtiofsd/helper.c > +++ b/tools/virtiofsd/helper.c > @@ -172,6 +172,9 @@ void fuse_cmdline_help(void) > " default: no_writeback\n" > " -o xattr|no_xattr enable/disable xattr\n" > " default: no_xattr\n" > + " -o xattrmap= Enable xattr mapping (enables xattr)\n" > + " is a string consists of a series of rules\n" > + " e.g. -o xattrmap=:map::user.virtiofs.:\n" This is a helpful note, but it doesn't tell the whole story. I think it'd be helpful to add one last note to this option which is to recommend reading the virtiofsd(1) man-page for more information on xattrmap rules. Connor
Re: General question about parsing an rbd filename
On 4/9/21 9:27 AM, Markus Armbruster wrote: Connor Kuehl writes: block/rbd.c hints that: * Configuration values containing :, @, or = can be escaped with a * leading "\". Right now, much of the parsing code will allow anyone to escape _anything_ so long as it's preceded by '\'. Is this the intended behavior? Or should the parser be updated to allow escaping only certain sequences. I can't answer this question, but perhaps I can get us a bit closer to an answer. The commend you quoted in part is about "rbd:" pseudo-filenames. By "parsing code", you probably mean qemu_rbd_parse_filename(). It uses qemu_rbd_next_tok() to split off one part after the other, stopping at a special delimiter character, and qemu_rbd_unescape() to unescape most, but not all parts. Both treat '\' followed by a character other than '\0' specially. qemu_rbd_next_tok() doesn't stop at an escaped delimiter character. qemu_rbd_unescape() unescapes escaped characters. I believe the comment you quoted is basically trying to say "to use a character that would normally be a delimiter, escape it with '\'". It doesn't say these are the only characters you may escape. I agree with your interpretation here. Not unescaping some parts feels iffy to me. I wonder if my reading of it placed too much emphasis on the "Configuration values" part of it, which I understand to be the optional key,value pairs that come after the image name. Thank you, Connor
Re: [PATCH] Document qemu-img options data_file and data_file_raw
On 3/23/21 6:15 PM, John Snow wrote: On 3/1/21 12:28 PM, Connor Kuehl wrote: [..] diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index b615aa8419..5cc585dc27 100644 --- a/docs/tools/qemu-img.rst +++ b/docs/tools/qemu-img.rst @@ -866,6 +866,18 @@ Supported image file formats: issue ``lsattr filename`` to check if the NOCOW flag is set or not (Capital 'C' is NOCOW flag). + ``data_file`` + Pathname that refers to a file that will store all guest data. If + this option is used, the qcow2 file will only contain the image's + metadata. + Might recommend "filename" simply for parity with *FILENAME* argument. (This is the first appearance of "Pathname" in this file without spaces, though "Path name" is indeed used several times.) + ``data_file_raw`` + If this option is set to ``on``, QEMU will always keep the external + data file consistent as a standalone read-only raw image. The default + value is ``off``. + + This option can only be enabled if ``data_file`` is set. + How does this interact with caching options, if it does? What happens in the negative case -- how does the file become inconsistent? Hi, First, just wanted to share the same note I'm sending to Max's review: Hey, I just wanted to leave a note indicating that I'm not ignoring this review; I've incorporated it in my next version but I am waiting until after the freeze to send it. Second: I've been trying to figure out if and how this affects caching. I do not know yet. I will keep digging. My next version of the patch does touch on the negative case though, describing how it becomes inconsistent. Thank you! Connor
Re: [PATCH] Document qemu-img options data_file and data_file_raw
On 3/26/21 4:24 AM, Max Reitz wrote: On 01.03.21 18:28, Connor Kuehl wrote: [..] diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index b615aa8419..5cc585dc27 100644 --- a/docs/tools/qemu-img.rst +++ b/docs/tools/qemu-img.rst @@ -866,6 +866,18 @@ Supported image file formats: issue ``lsattr filename`` to check if the NOCOW flag is set or not (Capital 'C' is NOCOW flag). + ``data_file`` + Pathname that refers to a file that will store all guest data. If + this option is used, the qcow2 file will only contain the image's + metadata. I think I would like a note here about the fact that when passing this option to qemu-img create, the given data file will be newly created, i.e. if it already contains data, all that data will be lost. And perhaps also note that qemu-img amend on the other hand will only change the reference in the qcow2 file, so the given file should already exist and will not be overwritten. (“Pathname that refers to a file” sounds like the file may already exist before this operation, which may give people ideas. (Not that the ideas were bad, it’s just that they have to take care. Referencing qemu-img amend should give them a hint on how to do it right.)) Hey, I just wanted to leave a note indicating that I'm not ignoring this review; I've incorporated it in my next version but I am waiting until after the freeze to send it. Thank you! Connor
[PATCH v3 0/2] Fix segfault in qemu_rbd_parse_filename
Connor Kuehl (2): iotests/231: Update expected deprecation message block/rbd: Add an escape-aware strchr helper block/rbd.c| 20 ++-- tests/qemu-iotests/231 | 4 tests/qemu-iotests/231.out | 7 --- 3 files changed, 26 insertions(+), 5 deletions(-) -- 2.30.2
[PATCH v3 1/2] iotests/231: Update expected deprecation message
The deprecation message in the expected output has technically been
wrong since the wrong version of a patch was applied to it. Because of
this, the test fails. Correct the expected output so that it passes.
Signed-off-by: Connor Kuehl
Reviewed-by: Max Reitz
---
tests/qemu-iotests/231.out | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 579ba11c16..747dd221bb 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -1,9 +1,7 @@
QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
+qemu-img: warning: RBD options encoded in the filename as keyvalue pairs is
deprecated
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
*** done
--
2.30.2
[PATCH v3 2/2] block/rbd: Add an escape-aware strchr helper
Sometimes the parser needs to further split a token it has collected
from the token input stream. Right now, it does a cursory check to see
if the relevant characters appear in the token to determine if it should
break it down further.
However, qemu_rbd_next_tok() will escape characters as it removes tokens
from the token stream and plain strchr() won't. This can make the
initial strchr() check slightly misleading since it implies
qemu_rbd_next_tok() will find the token and split on it, except the
reality is that qemu_rbd_next_tok() will pass over it if it is escaped.
Use a custom strchr to avoid mixing escaped and unescaped string
operations.
Reported-by: Han Han
Fixes: https://bugzilla.redhat.com/1873913
Signed-off-by: Connor Kuehl
---
v2 -> v3:
* Update qemu_rbd_strchr to only skip if there's a delimiter AND the
next character is not the NUL terminator
block/rbd.c| 20 ++--
tests/qemu-iotests/231 | 4
tests/qemu-iotests/231.out | 3 +++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..291e3f09e1 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -134,6 +134,22 @@ static char *qemu_rbd_next_tok(char *src, char delim, char
**p)
return src;
}
+static char *qemu_rbd_strchr(char *src, char delim)
+{
+char *p;
+
+for (p = src; *p; ++p) {
+if (*p == delim) {
+return p;
+}
+if (*p == '\\' && p[1] != '\0') {
+++p;
+}
+}
+
+return NULL;
+}
+
static void qemu_rbd_unescape(char *src)
{
char *p;
@@ -171,7 +187,7 @@ static void qemu_rbd_parse_filename(const char *filename,
QDict *options,
qemu_rbd_unescape(found_str);
qdict_put_str(options, "pool", found_str);
-if (strchr(p, '@')) {
+if (qemu_rbd_strchr(p, '@')) {
image_name = qemu_rbd_next_tok(p, '@', );
found_str = qemu_rbd_next_tok(p, ':', );
@@ -181,7 +197,7 @@ static void qemu_rbd_parse_filename(const char *filename,
QDict *options,
image_name = qemu_rbd_next_tok(p, ':', );
}
/* Check for namespace in the image_name */
-if (strchr(image_name, '/')) {
+if (qemu_rbd_strchr(image_name, '/')) {
found_str = qemu_rbd_next_tok(image_name, '/', _name);
qemu_rbd_unescape(found_str);
qdict_put_str(options, "namespace", found_str);
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
index 0f66d0ca36..8e6c6447c1 100755
--- a/tests/qemu-iotests/231
+++ b/tests/qemu-iotests/231
@@ -55,6 +55,10 @@ _filter_conf()
$QEMU_IMG info
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}"
2>&1 | _filter_conf
$QEMU_IMG info
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
2>&1 | _filter_conf
+# Regression test: the qemu-img invocation is expected to fail, but it should
+# not seg fault the parser.
+$QEMU_IMG create "rbd:rbd/aa\/bb:conf=${BOGUS_CONF}" 1M 2>&1 | _filter_conf
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 747dd221bb..a785a6e859 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -4,4 +4,7 @@ unable to get monitor info from DNS SRV with service name:
ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
+Formatting 'rbd:rbd/aa\/bb:conf=BOGUS_CONF', fmt=raw size=1048576
+unable to get monitor info from DNS SRV with service name: ceph-mon
+qemu-img: rbd:rbd/aa\/bb:conf=BOGUS_CONF: error connecting: No such file or
directory
*** done
--
2.30.2
Re: [PATCH v2 2/2] block/rbd: Add an escape-aware strchr helper
On 4/6/21 9:24 AM, Max Reitz wrote:
On 01.04.21 23:01, Connor Kuehl wrote:
[..]
diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..c0e4d4a952 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -134,6 +134,22 @@ static char *qemu_rbd_next_tok(char *src, char
delim, char **p)
return src;
}
+static char *qemu_rbd_strchr(char *src, char delim)
+{
+ char *p;
+
+ for (p = src; *p; ++p) {
+ if (*p == delim) {
+ return p;
+ }
+ if (*p == '\\') {
+ ++p;
+ }
+ }
+
+ return NULL;
+}
+
So I thought you could make qemu_rbd_do_next_tok() to do this. (I
didn’t say you should, but bear with me.) That would be possible by
giving it a new parameter (e.g. @find), and if that is set, return @end
if *end == delim after the loop, and NULL otherwise.
Now, if you add wrapper functions to make it nice, there’s not much more
difference in lines added compared to just adding a new function, but it
does mean your function should basically be the same as
qemu_rbd_next_tok(), except that no splitting happens, that there is no
*p, and that @end is returned instead of @src.
Do you have a strong preference for this? I agree that
qemu_rbd_next_tok() could grow this functionality, but I think it'd be
simpler to keep it separate in the form of qemu_rbd_strchr().
So there is one difference, and that is that qemu_rbd_next_tok() has
this condition to skip escaped characters:
if (*end == '\\' && end[1] != '\0') {
where qemu_rbd_strchr() has only:
if (*p == '\\') {
And I think qemu_rbd_next_tok() is right; if the string in question has
a trailing backslash, qemu_rbd_strchr() will ignore the final NUL and
continue searching past the end of the string.
Aha, good catch. I'll fix this up.
Thank you,
Connor
General question about parsing an rbd filename
Hi, block/rbd.c hints that: * Configuration values containing :, @, or = can be escaped with a * leading "\". Right now, much of the parsing code will allow anyone to escape _anything_ so long as it's preceded by '\'. Is this the intended behavior? Or should the parser be updated to allow escaping only certain sequences. Just curious. Thanks, Connor
[PATCH v2 2/2] block/rbd: Add an escape-aware strchr helper
Sometimes the parser needs to further split a token it has collected
from the token input stream. Right now, it does a cursory check to see
if the relevant characters appear in the token to determine if it should
break it down further.
However, qemu_rbd_next_tok() will escape characters as it removes tokens
from the token stream and plain strchr() won't. This can make the
initial strchr() check slightly misleading since it implies
qemu_rbd_next_tok() will find the token and split on it, except the
reality is that qemu_rbd_next_tok() will pass over it if it is escaped.
Use a custom strchr to avoid mixing escaped and unescaped string
operations.
Reported-by: Han Han
Fixes: https://bugzilla.redhat.com/1873913
Signed-off-by: Connor Kuehl
---
block/rbd.c| 20 ++--
tests/qemu-iotests/231 | 4
tests/qemu-iotests/231.out | 3 +++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..c0e4d4a952 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -134,6 +134,22 @@ static char *qemu_rbd_next_tok(char *src, char delim, char
**p)
return src;
}
+static char *qemu_rbd_strchr(char *src, char delim)
+{
+char *p;
+
+for (p = src; *p; ++p) {
+if (*p == delim) {
+return p;
+}
+if (*p == '\\') {
+++p;
+}
+}
+
+return NULL;
+}
+
static void qemu_rbd_unescape(char *src)
{
char *p;
@@ -171,7 +187,7 @@ static void qemu_rbd_parse_filename(const char *filename,
QDict *options,
qemu_rbd_unescape(found_str);
qdict_put_str(options, "pool", found_str);
-if (strchr(p, '@')) {
+if (qemu_rbd_strchr(p, '@')) {
image_name = qemu_rbd_next_tok(p, '@', );
found_str = qemu_rbd_next_tok(p, ':', );
@@ -181,7 +197,7 @@ static void qemu_rbd_parse_filename(const char *filename,
QDict *options,
image_name = qemu_rbd_next_tok(p, ':', );
}
/* Check for namespace in the image_name */
-if (strchr(image_name, '/')) {
+if (qemu_rbd_strchr(image_name, '/')) {
found_str = qemu_rbd_next_tok(image_name, '/', _name);
qemu_rbd_unescape(found_str);
qdict_put_str(options, "namespace", found_str);
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
index 0f66d0ca36..8e6c6447c1 100755
--- a/tests/qemu-iotests/231
+++ b/tests/qemu-iotests/231
@@ -55,6 +55,10 @@ _filter_conf()
$QEMU_IMG info
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}"
2>&1 | _filter_conf
$QEMU_IMG info
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
2>&1 | _filter_conf
+# Regression test: the qemu-img invocation is expected to fail, but it should
+# not seg fault the parser.
+$QEMU_IMG create "rbd:rbd/aa\/bb:conf=${BOGUS_CONF}" 1M 2>&1 | _filter_conf
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 747dd221bb..a785a6e859 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -4,4 +4,7 @@ unable to get monitor info from DNS SRV with service name:
ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
+Formatting 'rbd:rbd/aa\/bb:conf=BOGUS_CONF', fmt=raw size=1048576
+unable to get monitor info from DNS SRV with service name: ceph-mon
+qemu-img: rbd:rbd/aa\/bb:conf=BOGUS_CONF: error connecting: No such file or
directory
*** done
--
2.30.2
[PATCH v2 1/2] iotests/231: Update expected deprecation message
The deprecation message in the expected output has technically been
wrong since the wrong version of a patch was applied to it. Because of
this, the test fails. Correct the expected output so that it passes.
Signed-off-by: Connor Kuehl
Reviewed-by: Max Reitz
---
Reworded the commit log and added Max's R-b.
tests/qemu-iotests/231.out | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 579ba11c16..747dd221bb 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -1,9 +1,7 @@
QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
+qemu-img: warning: RBD options encoded in the filename as keyvalue pairs is
deprecated
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
*** done
--
2.30.2
[PATCH v2 0/2] Fix segfault in qemu_rbd_parse_filename
Replaced the change to qemu_rbd_next_tok with a standalone escape-aware helper for patch 2. Connor Kuehl (2): iotests/231: Update expected deprecation message block/rbd: Add an escape-aware strchr helper block/rbd.c| 20 ++-- tests/qemu-iotests/231 | 4 tests/qemu-iotests/231.out | 7 --- 3 files changed, 26 insertions(+), 5 deletions(-) -- 2.30.2
Re: [PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok()
On 4/1/21 12:24 PM, Max Reitz wrote:
On 01.04.21 17:52, Connor Kuehl wrote:
That's qemu_rbd_unescape()'s job! No need to duplicate the labor.
Furthermore, this was causing some confusion in the parsing logic to
where the caller might test for the presence of a character to split on
like so:
if (strchr(image_name, '/')) {
found_str = qemu_rbd_next_tok(image_name, '/', _name);
[..]
When qemu_rbd_next_tok() performs unescaping as a side effect, the
parser can get confused thinking that it can split this string, but
really the delimiter '/' gets unescaped and so qemu_rbd_next_tok() never
"finds" the delimiter and consumes the rest of the token input stream.
I don’t fully understand. I understand the strchr() problem and the
thing you explain next. But I would have thought that’s a problem of
using strchr(), i.e. that we’d need a custom function to do strchr() but
consider escaped characters. If it’s just about true/false like in your
example, it could be a new version of qemu_rbd_next_tok() that does not
modify the string.
I went back and forth a lot on the different ways this can be fixed
before sending this, and I agree the most consistent fix here would be
to add an escape-aware strchr. Initially, adding another libc-like
function with more side effects wasn't as appealing to me as removing
the side effects entirely to separate mechanism vs. policy. Your example
below convinced me that this patch would split the token in unexpected
ways. I'll send a v2 :-)
Thanks,
Connor
[..]
Should it? I would have fully expected it to not be split and the
parser complains that the input is invalid. Or, let’s say,
"foo\/bar/baz” should be split into “foo\/bar” and “baz”.
Re: [PATCH 1/2] iotests/231: Update expected deprecation message
On 4/1/21 12:07 PM, Max Reitz wrote: On 01.04.21 18:52, Max Reitz wrote: On 01.04.21 17:52, Connor Kuehl wrote: The deprecation message changed slightly at some point in the past but the expected output wasn't updated along with it; causing it to fail. Fix it, so it passes. Signed-off-by: Connor Kuehl --- tests/qemu-iotests/231.out | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) Uh, well, you know what, I can’t find any version where there was any other output. Even back in 66e6a735e97450ac50fcaf40f78600c688534cae, where this test was introduced, I get this diff. What’s going on there? Okay. So: Jeff’s original patch[1] included the “Future versions may cease to parse...” part. v1 of his subsequent pull request[2] did, too. But v2[3] didn’t. Looks like Markus made a comment on v4 of the patch, and then Jeff fixed up the patch in his branch, but didn’t change the test. In any case it’s clear that the reference output was wrong all along. About the “no monitors specified” part... The only place where I can find “no monitors” is in Jeff’s patches to add this iotest. I have no idea where that orignated from. So: Reviewed-by: Max Reitz Thanks! And excellent sleuthing. This accidental conspiracy went much farther beyond the git log than I thought... Connor [1] https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00282.html [2] https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00307.html [3] https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00592.html
[PATCH 1/2] iotests/231: Update expected deprecation message
The deprecation message changed slightly at some point in the past but
the expected output wasn't updated along with it; causing it to fail.
Fix it, so it passes.
Signed-off-by: Connor Kuehl
---
tests/qemu-iotests/231.out | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 579ba11c16..747dd221bb 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -1,9 +1,7 @@
QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
+qemu-img: warning: RBD options encoded in the filename as keyvalue pairs is
deprecated
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
*** done
--
2.30.2
[PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok()
That's qemu_rbd_unescape()'s job! No need to duplicate the labor.
Furthermore, this was causing some confusion in the parsing logic to
where the caller might test for the presence of a character to split on
like so:
if (strchr(image_name, '/')) {
found_str = qemu_rbd_next_tok(image_name, '/', _name);
[..]
When qemu_rbd_next_tok() performs unescaping as a side effect, the
parser can get confused thinking that it can split this string, but
really the delimiter '/' gets unescaped and so qemu_rbd_next_tok() never
"finds" the delimiter and consumes the rest of the token input stream.
This is problematic because qemu_rbd_next_tok() also steals the input
pointer to the token stream and sets it to NULL. This causes a segfault
where the parser expects to split one string into two.
In this case, the parser is determining if a string is a
namespace/image_name pair like so:
"foo/bar"
And clearly it's looking to split it like so:
namespace: foo
image_name: bar
but if the input is "foo\/bar", it *should* split into
namespace: foo\
image_name: bar
and its subordinate parts can be unescaped after tokenization.
So, instead of tokenizing *and* escaping all at once, do one before the
other to avoid stumbling into a segfault by confusing the parsing logic.
Reported-by: Han Han
Fixes: https://bugzilla.redhat.com/1873913
Signed-off-by: Connor Kuehl
---
block/rbd.c| 3 ---
tests/qemu-iotests/231 | 4
tests/qemu-iotests/231.out | 3 +++
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..9bed0863e5 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -123,9 +123,6 @@ static char *qemu_rbd_next_tok(char *src, char delim, char
**p)
if (*end == delim) {
break;
}
-if (*end == '\\' && end[1] != '\0') {
-end++;
-}
}
if (*end == delim) {
*p = end + 1;
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
index 0f66d0ca36..8e6c6447c1 100755
--- a/tests/qemu-iotests/231
+++ b/tests/qemu-iotests/231
@@ -55,6 +55,10 @@ _filter_conf()
$QEMU_IMG info
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}"
2>&1 | _filter_conf
$QEMU_IMG info
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
2>&1 | _filter_conf
+# Regression test: the qemu-img invocation is expected to fail, but it should
+# not seg fault the parser.
+$QEMU_IMG create "rbd:rbd/aa\/bb:conf=${BOGUS_CONF}" 1M 2>&1 | _filter_conf
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 747dd221bb..a785a6e859 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -4,4 +4,7 @@ unable to get monitor info from DNS SRV with service name:
ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
+Formatting 'rbd:rbd/aa\/bb:conf=BOGUS_CONF', fmt=raw size=1048576
+unable to get monitor info from DNS SRV with service name: ceph-mon
+qemu-img: rbd:rbd/aa\/bb:conf=BOGUS_CONF: error connecting: No such file or
directory
*** done
--
2.30.2
[PATCH 0/2] Fix segfault in qemu_rbd_parse_filename
Connor Kuehl (2): iotests/231: Update expected deprecation message block/rbd: Don't unescape in qemu_rbd_next_tok() block/rbd.c| 3 --- tests/qemu-iotests/231 | 4 tests/qemu-iotests/231.out | 7 --- 3 files changed, 8 insertions(+), 6 deletions(-) -- 2.30.2
Re: [Virtio-fs] [PATCH] virtiofsd: Fix security.capability comparison
On 4/1/21 9:58 AM, Dr. David Alan Gilbert (git) wrote:
From: "Dr. David Alan Gilbert"
My security fix for the security.capability remap has a silly early
segfault in a simple case where there is an xattrmapping but it doesn't
remap the securty.capability.
s/securty/security
Fixes: e586edcb41054 ("virtiofs: drop remapped security.capability xattr as
needed")
Signed-off-by: Dr. David Alan Gilbert
Reviewed-by: Connor Kuehl
---
tools/virtiofsd/passthrough_ll.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index b144320e48..1553d2ef45 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2636,7 +2636,8 @@ static void parse_xattrmap(struct lo_data *lo)
strerror(ret));
exit(1);
}
-if (!strcmp(lo->xattr_security_capability, "security.capability")) {
+if (!lo->xattr_security_capability ||
+!strcmp(lo->xattr_security_capability, "security.capability")) {
/* 1-1 mapping, don't need to do anything */
free(lo->xattr_security_capability);
lo->xattr_security_capability = NULL;
Re: [PATCH 0/2] SEV firmware error list touchups
On 3/22/21 5:18 AM, Philippe Mathieu-Daudé wrote: Hi Connor, On 3/15/21 3:08 PM, Connor Kuehl wrote: On 2/18/21 9:16 AM, Connor Kuehl wrote: Connor Kuehl (2): sev: use explicit indices for mapping firmware error codes to strings sev: add missing firmware error conditions target/i386/sev.c | 48 --- 1 file changed, 25 insertions(+), 23 deletions(-) Eduardo, Paolo, Richard: ping Looks too late for 6.0 now. Can you repost/ping after QEMU 6.0 is release? Sure. Connor
[PATCH 0/1] iotests: fix 051.out expected output after error
Oops, sorry about the churn. I can see why this would have caused a failure but I'm surprised I can't reproduce this when I run the test locally. Christian, would you be willing to test this patch out as a quick sanity check too? Connor Kuehl (1): iotests: fix 051.out expected output after error text touchups tests/qemu-iotests/051.out | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) -- 2.30.2
[PATCH 1/1] iotests: fix 051.out expected output after error text touchups
A patch was recently applied that touched up some error messages that
pertained to key names like 'node-name'. The trouble is it only updated
tests/qemu-iotests/051.pc.out and not tests/qemu-iotests/051.out as
well.
Do that now.
Fixes: 785ec4b1b9 ("block: Clarify error messages pertaining to
'node-name'")
Signed-off-by: Connor Kuehl
---
tests/qemu-iotests/051.out | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index de4771bcb3..db8c14b903 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -61,13 +61,13 @@ QEMU X.Y.Z monitor - type 'help' for more information
(qemu) quit
Testing: -drive file=TEST_DIR/t.qcow2,node-name=123foo
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=123foo: Invalid node name
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=123foo: Invalid node-name:
'123foo'
Testing: -drive file=TEST_DIR/t.qcow2,node-name=_foo
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=_foo: Invalid node name
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=_foo: Invalid node-name:
'_foo'
Testing: -drive file=TEST_DIR/t.qcow2,node-name=foo#12
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=foo#12: Invalid node name
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=foo#12: Invalid node-name:
'foo#12'
=== Device without drive ===
--
2.30.2
[PATCH] MAINTAINERS: add virtio-fs mailing list
General discussion and patch reviews take place on this list for both virtiofsd (tools/virtiofsd/*) and the guest kernel module. Signed-off-by: Connor Kuehl --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 25fc49d1dc..8921bc2119 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1917,6 +1917,7 @@ F: tools/virtiofsd/* F: hw/virtio/vhost-user-fs* F: include/hw/virtio/vhost-user-fs.h F: docs/tools/virtiofsd.rst +L: virtio...@redhat.com virtio-input M: Gerd Hoffmann -- 2.30.2
Re: [PATCH] tools/virtiofsd: include --socket-group in help
On 3/18/21 5:09 AM, Alex Bennée wrote:
I confused myself wandering if this had been merged by looking at the
help output. It seems fuse_opt doesn't automagically add to help
output so lets do it now.
Updates: f6698f2b03 ("tools/virtiofsd: add support for --socket-group")
Signed-off-by: Alex Bennée
---
tools/virtiofsd/fuse_lowlevel.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
index 1aa26c6333..58e32fc963 100644
--- a/tools/virtiofsd/fuse_lowlevel.c
+++ b/tools/virtiofsd/fuse_lowlevel.c
@@ -2450,6 +2450,7 @@ void fuse_lowlevel_help(void)
printf(
"-o allow_root allow access by root\n"
"--socket-path=PATH path for the vhost-user socket\n"
+"--socket-group=GRNAME name of group for the vhost-user
socket\n"
"--fd=FDNUM fd number of vhost-user socket\n"
"--thread-pool-size=NUM thread pool size limit (default
%d)\n",
THREAD_POOL_SIZE);
And it looks like this is already in the man page too, so I think this
patches the last place it was missing from, nice!
Reviewed-by: Connor Kuehl
Re: [PATCH 0/2] SEV firmware error list touchups
On 2/18/21 9:16 AM, Connor Kuehl wrote: Connor Kuehl (2): sev: use explicit indices for mapping firmware error codes to strings sev: add missing firmware error conditions target/i386/sev.c | 48 --- 1 file changed, 25 insertions(+), 23 deletions(-) Eduardo, Paolo, Richard: ping
Re: [PATCH] Document qemu-img options data_file and data_file_raw
Ping (+Kevin Wolf to CC) Kevin, would this be appropriate for your tree? On 3/1/21 11:28 AM, Connor Kuehl wrote: The contents of this patch were initially developed and posted by Han Han[1], however, it appears the original patch was not applied. Since then, the relevant documentation has been moved and adapted to a new format. I've taken most of the original wording and tweaked it according to some of the feedback from the original patch submission. I've also adapted it to restructured text, which is the format the documentation currently uses. [1] https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg01253.html Reported-by: Han Han Co-developed-by: Han Han Fixes: https://bugzilla.redhat.com/1763105 Signed-off-by: Connor Kuehl --- docs/tools/qemu-img.rst | 12 1 file changed, 12 insertions(+) diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index b615aa8419..5cc585dc27 100644 --- a/docs/tools/qemu-img.rst +++ b/docs/tools/qemu-img.rst @@ -866,6 +866,18 @@ Supported image file formats: issue ``lsattr filename`` to check if the NOCOW flag is set or not (Capital 'C' is NOCOW flag). + ``data_file`` +Pathname that refers to a file that will store all guest data. If +this option is used, the qcow2 file will only contain the image's +metadata. + + ``data_file_raw`` +If this option is set to ``on``, QEMU will always keep the external +data file consistent as a standalone read-only raw image. The default +value is ``off``. + +This option can only be enabled if ``data_file`` is set. + ``Other`` QEMU also supports various other image file formats for
Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Don't allow empty filenames
On 3/12/21 8:10 AM, Greg Kurz wrote: POSIX.1-2017 clearly stipulates that empty filenames aren't allowed ([1] and [2]). Since virtiofsd is supposed to mirror the host file system hierarchy and the host can be assumed to be linux, we don't really expect clients to pass requests with an empty path in it. If they do so anyway, this would eventually cause an error when trying to create/lookup the actual inode on the underlying POSIX filesystem. But this could still confuse some code that wouldn't be ready to cope with this. Filter out empty names coming from the client at the top level, so that the rest doesn't have to care about it. This is done everywhere we already call is_safe_path_component(), but in a separate helper since the usual error for empty path names is ENOENT instead of EINVAL. [1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_170 [2] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_13 Signed-off-by: Greg Kurz Reviewed-by: Connor Kuehl
Re: [Virtio-fs] [PATCH 1/3] virtiofsd: Don't allow empty paths in lookup_name()
On 3/12/21 8:10 AM, Greg Kurz wrote:
When passed an empty filename, lookup_name() returns the inode of
the parent directory, unless the parent is the root in which case
the st_dev doesn't match and lo_find() returns NULL. This is
because lookup_name() passes AT_EMPTY_PATH down to fstatat() or
statx().
This behavior doesn't quite make sense because users of lookup_name()
then pass the name to unlinkat(), renameat() or renameat2(), all of
which will always fail on empty names.
Drop AT_EMPTY_PATH from the flags in lookup_name() so that it has
the consistent behavior of "returning an existing child inode or
NULL" for all directories.
Signed-off-by: Greg Kurz
---
tools/virtiofsd/passthrough_ll.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index fc7e1b1e8e2b..27a6c636dcaf 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -1308,8 +1308,7 @@ static struct lo_inode *lookup_name(fuse_req_t req,
fuse_ino_t parent,
return NULL;
}
-res = do_statx(lo, dir->fd, name, ,
- AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW, _id);
+res = do_statx(lo, dir->fd, name, , AT_SYMLINK_NOFOLLOW, _id);
lo_inode_put(lo, );
if (res == -1) {
return NULL;
Should the statx() in lo_do_lookup() have this flag removed as well? I
don't think its callers will pass in an empty name because:
- One of your later patches prevents lo_mknod_symlink() from doing so
- lo_create() will fail an earlier call against the host file system
(open)
- lo_do_readdir() shouldn't be reading empty names because that
implies someone was able to pull off making an entry with an empty name
However, I don't know if there will one day be future callers to
lo_do_lookup() that will depend on that flag.
If the answer to the above is no, then:
Reviewed-by: Connor Kuehl
Re: [Virtio-fs] [PATCH 2/3] virtiofsd: Convert some functions to return bool
On 3/12/21 8:10 AM, Greg Kurz wrote:
Both currently only return 0 or 1.
Signed-off-by: Greg Kurz
---
tools/virtiofsd/passthrough_ll.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 27a6c636dcaf..f63016d35626 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -221,17 +221,17 @@ static struct lo_inode *lo_find(struct lo_data *lo,
struct stat *st,
static int xattr_map_client(const struct lo_data *lo, const char *client_name,
char **out_name);
-static int is_dot_or_dotdot(const char *name)
+static bool is_dot_or_dotdot(const char *name)
{
return name[0] == '.' &&
(name[1] == '\0' || (name[1] == '.' && name[2] == '\0'));
}
/* Is `path` a single path component that is not "." or ".."? */
-static int is_safe_path_component(const char *path)
+static bool is_safe_path_component(const char *path)
{
if (strchr(path, '/')) {
-return 0;
+return false;
}
return !is_dot_or_dotdot(path);
Reviewed-by: Connor Kuehl
Re: [PATCH] Document qemu-img options data_file and data_file_raw
On 3/1/21 11:28 AM, Connor Kuehl wrote: The contents of this patch were initially developed and posted by Han Han[1], however, it appears the original patch was not applied. Since then, the relevant documentation has been moved and adapted to a new format. I've taken most of the original wording and tweaked it according to some of the feedback from the original patch submission. I've also adapted it to restructured text, which is the format the documentation currently uses. [1] https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg01253.html Reported-by: Han Han Co-developed-by: Han Han Fixes: https://bugzilla.redhat.com/1763105 Signed-off-by: Connor Kuehl I am not sure whose tree this is best suited for; ./scripts/get_maintainers.pl only showed other contributors to this file and not an explicit maintainer, per se. Connor
Re: [PATCH 0/2] SEV firmware error list touchups
On 2/18/21 9:16 AM, Connor Kuehl wrote: Connor Kuehl (2): sev: use explicit indices for mapping firmware error codes to strings sev: add missing firmware error conditions target/i386/sev.c | 48 --- 1 file changed, 25 insertions(+), 23 deletions(-) Ping. Also, I am not sure whose tree these are best suited for. Connor
[PATCH v2 2/2] blockdev: Clarify error messages pertaining to 'node-name'
Signed-off-by: Connor Kuehl
---
blockdev.c | 13 +++--
tests/qemu-iotests/245 | 6 +++---
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index cd438e60e3..7c7ab2b386 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1515,13 +1515,13 @@ static void external_snapshot_prepare(BlkActionState
*common,
s->has_snapshot_node_name ? s->snapshot_node_name : NULL;
if (node_name && !snapshot_node_name) {
-error_setg(errp, "New overlay node name missing");
+error_setg(errp, "New overlay node-name missing");
goto out;
}
if (snapshot_node_name &&
bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
-error_setg(errp, "New overlay node name already in use");
+error_setg(errp, "New overlay node-name already in use");
goto out;
}
@@ -3598,13 +3598,14 @@ void qmp_x_blockdev_reopen(BlockdevOptions *options,
Error **errp)
/* Check for the selected node name */
if (!options->has_node_name) {
-error_setg(errp, "Node name not specified");
+error_setg(errp, "node-name not specified");
goto fail;
}
bs = bdrv_find_node(options->node_name);
if (!bs) {
-error_setg(errp, "Cannot find node named '%s'", options->node_name);
+error_setg(errp, "Failed to find node with node-name='%s'",
+ options->node_name);
goto fail;
}
@@ -3635,7 +3636,7 @@ void qmp_blockdev_del(const char *node_name, Error **errp)
bs = bdrv_find_node(node_name);
if (!bs) {
-error_setg(errp, "Cannot find node %s", node_name);
+error_setg(errp, "Failed to find node with node-name='%s'", node_name);
return;
}
if (bdrv_has_blk(bs)) {
@@ -3758,7 +3759,7 @@ void qmp_x_blockdev_set_iothread(const char *node_name,
StrOrNull *iothread,
bs = bdrv_find_node(node_name);
if (!bs) {
-error_setg(errp, "Cannot find node %s", node_name);
+error_setg(errp, "Failed to find node with node-name='%s'", node_name);
return;
}
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index f8eba7719a..a2a0482469 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -140,8 +140,8 @@ class TestBlockdevReopen(iotests.QMPTestCase):
self.reopen(opts, {'file': 'hd0-file'})
# We cannot change any of these
-self.reopen(opts, {'node-name': 'not-found'}, "Cannot find node named
'not-found'")
-self.reopen(opts, {'node-name': ''}, "Cannot find node named ''")
+self.reopen(opts, {'node-name': 'not-found'}, "Failed to find node
with node-name='not-found'")
+self.reopen(opts, {'node-name': ''}, "Failed to find node with
node-name=''")
self.reopen(opts, {'node-name': None}, "Invalid parameter type for
'node-name', expected: string")
self.reopen(opts, {'driver': 'raw'}, "Cannot change the option
'driver'")
self.reopen(opts, {'driver': ''}, "Invalid parameter ''")
@@ -158,7 +158,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
# node-name is optional in BlockdevOptions, but x-blockdev-reopen
needs it
del opts['node-name']
-self.reopen(opts, {}, "Node name not specified")
+self.reopen(opts, {}, "node-name not specified")
# Check that nothing has changed
self.check_node_graph(original_graph)
--
2.29.2
[PATCH v2 0/2] Clarify error messages pertaining to 'node-name'
v2: - Moved summary into patch #1 - Updated test cases that were missed in v1 from running 'make check'. This time I used 'make check-block SPEED=thorough' and some more grepping to make sure I didn't miss any. - Rebased Connor Kuehl (2): block: Clarify error messages pertaining to 'node-name' blockdev: Clarify error messages pertaining to 'node-name' block.c | 8 blockdev.c| 13 +++-- tests/qemu-iotests/030| 4 ++-- tests/qemu-iotests/040| 4 ++-- tests/qemu-iotests/051.pc.out | 6 +++--- tests/qemu-iotests/081.out| 2 +- tests/qemu-iotests/085.out| 6 +++--- tests/qemu-iotests/087.out| 2 +- tests/qemu-iotests/206.out| 2 +- tests/qemu-iotests/210.out| 2 +- tests/qemu-iotests/211.out| 2 +- tests/qemu-iotests/212.out| 2 +- tests/qemu-iotests/213.out| 2 +- tests/qemu-iotests/223.out| 4 ++-- tests/qemu-iotests/237.out| 2 +- tests/qemu-iotests/245| 10 +- tests/qemu-iotests/249.out| 2 +- tests/qemu-iotests/300| 4 ++-- 18 files changed, 39 insertions(+), 38 deletions(-) -- 2.29.2
[PATCH v2 1/2] block: Clarify error messages pertaining to 'node-name'
Some error messages contain ambiguous representations of the 'node-name'
parameter. This can be particularly confusing when exchanging QMP
messages (C = client, S = server):
C: {"execute": "block_resize", "arguments": { "device": "my_file", "size":
26843545600 }}
S: {"error": {"class": "GenericError", "desc": "Cannot find device=my_file nor
node_name="}}
^
This error message suggests one could send a message with a key called
'node_name':
C: {"execute": "block_resize", "arguments": { "node_name": "my_file", "size":
26843545600 }}
^
but using the underscore is actually incorrect, the parameter should be
'node-name':
S: {"error": {"class": "GenericError", "desc": "Parameter 'node_name' is
unexpected"}}
This behavior was uncovered in bz1651437, but I ended up going down a
rabbit hole looking for other areas where this miscommunication might
occur and changing those accordingly as well.
Fixes: https://bugzilla.redhat.com/1651437
Signed-off-by: Connor Kuehl
---
block.c | 8
tests/qemu-iotests/030| 4 ++--
tests/qemu-iotests/040| 4 ++--
tests/qemu-iotests/051.pc.out | 6 +++---
tests/qemu-iotests/081.out| 2 +-
tests/qemu-iotests/085.out| 6 +++---
tests/qemu-iotests/087.out| 2 +-
tests/qemu-iotests/206.out| 2 +-
tests/qemu-iotests/210.out| 2 +-
tests/qemu-iotests/211.out| 2 +-
tests/qemu-iotests/212.out| 2 +-
tests/qemu-iotests/213.out| 2 +-
tests/qemu-iotests/223.out| 4 ++--
tests/qemu-iotests/237.out| 2 +-
tests/qemu-iotests/245| 4 ++--
tests/qemu-iotests/249.out| 2 +-
tests/qemu-iotests/300| 4 ++--
17 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/block.c b/block.c
index a1f3cecd75..2daff6d29a 100644
--- a/block.c
+++ b/block.c
@@ -1440,7 +1440,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
* Check for empty string or invalid characters, but not if it is
* generated (generated names use characters not available to the user)
*/
-error_setg(errp, "Invalid node name");
+error_setg(errp, "Invalid node-name: '%s'", node_name);
return;
}
@@ -1453,7 +1453,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
/* takes care of avoiding duplicates node names */
if (bdrv_find_node(node_name)) {
-error_setg(errp, "Duplicate node name");
+error_setg(errp, "Duplicate nodes with node-name='%s'", node_name);
goto out;
}
@@ -5432,7 +5432,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
}
}
-error_setg(errp, "Cannot find device=%s nor node_name=%s",
+error_setg(errp, "Cannot find device=\'%s\' nor node-name=\'%s\'",
device ? device : "",
node_name ? node_name : "");
return NULL;
@@ -6752,7 +6752,7 @@ BlockDriverState *check_to_replace_node(BlockDriverState
*parent_bs,
AioContext *aio_context;
if (!to_replace_bs) {
-error_setg(errp, "Node name '%s' not found", node_name);
+error_setg(errp, "Failed to find node with node-name='%s'", node_name);
return NULL;
}
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 12aa9ed37e..5fb65b4bef 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -153,7 +153,7 @@ class TestSingleDrive(iotests.QMPTestCase):
def test_device_not_found(self):
result = self.vm.qmp('block-stream', device='nonexistent')
self.assert_qmp(result, 'error/desc',
-'Cannot find device=nonexistent nor node_name=nonexistent')
+'Cannot find device=\'nonexistent\' nor node-name=\'nonexistent\'')
def test_job_id_missing(self):
result = self.vm.qmp('block-stream', device='mid')
@@ -507,7 +507,7 @@ class TestParallelOps(iotests.QMPTestCase):
# Error: the base node does not exist
result = self.vm.qmp('block-stream', device='node4', base_node='none',
job_id='stream')
self.assert_qmp(result, 'error/desc',
-'Cannot find device= nor node_name=none')
+'Cannot find device=\'\' nor node-name=\'none\'')
# Error: the base node is not a backing file of the top node
result = self.vm.qmp('block-stream', device='node4',
base_node='node6', job_id='stream')
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 7ebc9ed825..336ff7c4f2 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -175,13 +175
Re: [PATCH 0/2] Clarify error messages pertaining to 'node-name'
On 3/3/21 3:53 AM, Kevin Wolf wrote:
Am 02.03.2021 um 00:36 hat Connor Kuehl geschrieben:
Some error messages contain ambiguous representations of the 'node-name'
parameter. This can be particularly confusing when exchanging QMP
messages (C = client, S = server):
C: {"execute": "block_resize", "arguments": { "device": "my_file", "size":
26843545600 }}
S: {"error": {"class": "GenericError", "desc": "Cannot find device=my_file nor
node_name="}}
^
Arguably, this error message isn't great anyway because of the empty
string node name. We didn't even look for a node name, so why mention it
in the error message?
But your patches are certainly a good improvement already. I would have
merged them, but git grep 'nor node_name=' shows that you missed to
update a few tests, so they fail after the series. I suppose you only
caught the ones that are run by default in 'make check' and missed the
ones that require running the qemu-iotests 'check' script manually.
Ah, good catch! Yes, I was only using `make check`, I'll use the `check`
script to uncover the other failures and fix them accordingly.
[..]
This is a good explanation for the change you're making. I think it
deserves being committed to the repository in the commit message for
patch 1.
I'll move this to patch #1 in the next revision of this series.
Thank you!
Connor
[PATCH 2/2] blockdev: Clarify error messages pertaining to 'node-name'
Signed-off-by: Connor Kuehl
---
blockdev.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index cd438e60e3..7c7ab2b386 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1515,13 +1515,13 @@ static void external_snapshot_prepare(BlkActionState
*common,
s->has_snapshot_node_name ? s->snapshot_node_name : NULL;
if (node_name && !snapshot_node_name) {
-error_setg(errp, "New overlay node name missing");
+error_setg(errp, "New overlay node-name missing");
goto out;
}
if (snapshot_node_name &&
bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
-error_setg(errp, "New overlay node name already in use");
+error_setg(errp, "New overlay node-name already in use");
goto out;
}
@@ -3598,13 +3598,14 @@ void qmp_x_blockdev_reopen(BlockdevOptions *options,
Error **errp)
/* Check for the selected node name */
if (!options->has_node_name) {
-error_setg(errp, "Node name not specified");
+error_setg(errp, "node-name not specified");
goto fail;
}
bs = bdrv_find_node(options->node_name);
if (!bs) {
-error_setg(errp, "Cannot find node named '%s'", options->node_name);
+error_setg(errp, "Failed to find node with node-name='%s'",
+ options->node_name);
goto fail;
}
@@ -3635,7 +3636,7 @@ void qmp_blockdev_del(const char *node_name, Error **errp)
bs = bdrv_find_node(node_name);
if (!bs) {
-error_setg(errp, "Cannot find node %s", node_name);
+error_setg(errp, "Failed to find node with node-name='%s'", node_name);
return;
}
if (bdrv_has_blk(bs)) {
@@ -3758,7 +3759,7 @@ void qmp_x_blockdev_set_iothread(const char *node_name,
StrOrNull *iothread,
bs = bdrv_find_node(node_name);
if (!bs) {
-error_setg(errp, "Cannot find node %s", node_name);
+error_setg(errp, "Failed to find node with node-name='%s'", node_name);
return;
}
--
2.29.2
[PATCH 0/2] Clarify error messages pertaining to 'node-name'
Some error messages contain ambiguous representations of the 'node-name'
parameter. This can be particularly confusing when exchanging QMP
messages (C = client, S = server):
C: {"execute": "block_resize", "arguments": { "device": "my_file", "size":
26843545600 }}
S: {"error": {"class": "GenericError", "desc": "Cannot find device=my_file nor
node_name="}}
^
This error message suggests one could send a message with a key called
'node_name':
C: {"execute": "block_resize", "arguments": { "node_name": "my_file", "size":
26843545600 }}
^
but using the underscore is actually incorrect, the parameter should be
'node-name':
S: {"error": {"class": "GenericError", "desc": "Parameter 'node_name' is
unexpected"}}
This behavior was uncovered in bz1651437[1], but I ended up going down a
rabbit hole looking for other areas where this miscommunication might
occur and changing those accordingly as well.
[1] https://bugzilla.redhat.com/1651437
Connor Kuehl (2):
block: Clarify error messages pertaining to 'node-name'
blockdev: Clarify error messages pertaining to 'node-name'
block.c| 8
blockdev.c | 13 +++--
tests/qemu-iotests/040 | 4 ++--
tests/qemu-iotests/249.out | 2 +-
4 files changed, 14 insertions(+), 13 deletions(-)
--
2.29.2
[PATCH 1/2] block: Clarify error messages pertaining to 'node-name'
Reported-by: Tingting Mao
Fixes: https://bugzilla.redhat.com/1651437
Signed-off-by: Connor Kuehl
---
block.c| 8
tests/qemu-iotests/040 | 4 ++--
tests/qemu-iotests/249.out | 2 +-
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index a1f3cecd75..2daff6d29a 100644
--- a/block.c
+++ b/block.c
@@ -1440,7 +1440,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
* Check for empty string or invalid characters, but not if it is
* generated (generated names use characters not available to the user)
*/
-error_setg(errp, "Invalid node name");
+error_setg(errp, "Invalid node-name: '%s'", node_name);
return;
}
@@ -1453,7 +1453,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
/* takes care of avoiding duplicates node names */
if (bdrv_find_node(node_name)) {
-error_setg(errp, "Duplicate node name");
+error_setg(errp, "Duplicate nodes with node-name='%s'", node_name);
goto out;
}
@@ -5432,7 +5432,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
}
}
-error_setg(errp, "Cannot find device=%s nor node_name=%s",
+error_setg(errp, "Cannot find device=\'%s\' nor node-name=\'%s\'",
device ? device : "",
node_name ? node_name : "");
return NULL;
@@ -6752,7 +6752,7 @@ BlockDriverState *check_to_replace_node(BlockDriverState
*parent_bs,
AioContext *aio_context;
if (!to_replace_bs) {
-error_setg(errp, "Node name '%s' not found", node_name);
+error_setg(errp, "Failed to find node with node-name='%s'", node_name);
return NULL;
}
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 7ebc9ed825..336ff7c4f2 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -175,13 +175,13 @@ class TestSingleDrive(ImageCommitTestCase):
self.assert_no_active_block_jobs()
result = self.vm.qmp('block-commit', device='drive0',
top_node='badfile', base_node='base')
self.assert_qmp(result, 'error/class', 'GenericError')
-self.assert_qmp(result, 'error/desc', "Cannot find device= nor
node_name=badfile")
+self.assert_qmp(result, 'error/desc', "Cannot find device='' nor
node-name='badfile'")
def test_base_node_invalid(self):
self.assert_no_active_block_jobs()
result = self.vm.qmp('block-commit', device='drive0', top_node='mid',
base_node='badfile')
self.assert_qmp(result, 'error/class', 'GenericError')
-self.assert_qmp(result, 'error/desc', "Cannot find device= nor
node_name=badfile")
+self.assert_qmp(result, 'error/desc', "Cannot find device='' nor
node-name='badfile'")
def test_top_path_and_node(self):
self.assert_no_active_block_jobs()
diff --git a/tests/qemu-iotests/249.out b/tests/qemu-iotests/249.out
index 92ec81db03..d2bf9be85e 100644
--- a/tests/qemu-iotests/249.out
+++ b/tests/qemu-iotests/249.out
@@ -18,7 +18,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.
'filter-node-name': '1234'}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}
-{"error": {"class": "GenericError", "desc": "Invalid node name"}}
+{"error": {"class": "GenericError", "desc": "Invalid node-name: '1234'"}}
=== Send a write command to a drive opened in read-only mode (2)
--
2.29.2
[PATCH] Document qemu-img options data_file and data_file_raw
The contents of this patch were initially developed and posted by Han Han[1], however, it appears the original patch was not applied. Since then, the relevant documentation has been moved and adapted to a new format. I've taken most of the original wording and tweaked it according to some of the feedback from the original patch submission. I've also adapted it to restructured text, which is the format the documentation currently uses. [1] https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg01253.html Reported-by: Han Han Co-developed-by: Han Han Fixes: https://bugzilla.redhat.com/1763105 Signed-off-by: Connor Kuehl --- docs/tools/qemu-img.rst | 12 1 file changed, 12 insertions(+) diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index b615aa8419..5cc585dc27 100644 --- a/docs/tools/qemu-img.rst +++ b/docs/tools/qemu-img.rst @@ -866,6 +866,18 @@ Supported image file formats: issue ``lsattr filename`` to check if the NOCOW flag is set or not (Capital 'C' is NOCOW flag). + ``data_file`` +Pathname that refers to a file that will store all guest data. If +this option is used, the qcow2 file will only contain the image's +metadata. + + ``data_file_raw`` +If this option is set to ``on``, QEMU will always keep the external +data file consistent as a standalone read-only raw image. The default +value is ``off``. + +This option can only be enabled if ``data_file`` is set. + ``Other`` QEMU also supports various other image file formats for -- 2.29.2
