Re: [Qemu-devel] [PULL 00/32] tcg patch queue
On 12/15/18 1:18 PM, Peter Maydell wrote: > This didn't pass 'make check' on sparc64 host. > It looks like the handful of tests that exercise TCG > in the process of doing what they do failed, and there > was a tcg assert in there too: ... > qemu-system-x86_64: /home/pm215/qemu/tcg/sparc/tcg-target.inc.c:319: > patch_reloc: Assertion `check_fit_ptr(value, 13)' failed. If there were extra asserts, they should have been outside tcg/foo/*. Any extra asserts inside I cannot explain. Unfortunately, sparc and s390 are two that I cannot test directly. r~
Re: [Qemu-devel] [PATCH v5 00/14] arm: nRF51 Devices and Microbit Support
On Mon, Nov 12, 2018 at 04:42:10PM -0500, Steffen Görtz wrote: > This series contains additional peripheral devices for the nRF51822 > microcontroller. > > Included devices: > - Random Number Generator > - Non-volatile Memories > - General purpose I/O > - Timer > - Stub for clock peripheral Hi Steffen, I have pushed fixup commits addressing the easy comments: https://github.com/stefanha/qemu/commits/pouze-microbit The remaining issues are the periodic timer and how to model flash memory (address_space_write()). Feel free to squash my fixup commits if they are useful to you. Stefan signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v5 18/73] ppc: convert to cpu_halted
On Thu, Dec 13, 2018 at 12:03:58AM -0500, Emilio G. Cota wrote:
> In ppce500_spin.c, acquire the lock just once to update
> both cpu->halted and cpu->stopped.
>
> In hw/ppc/spapr_hcall.c, acquire the lock just once to
> update cpu->halted and call cpu_has_work, since later
> in the series we'll acquire the BQL (if not already held)
> from cpu_has_work.
>
> Cc: David Gibson
> Cc: Alexander Graf
> Cc: qemu-...@nongnu.org
> Reviewed-by: Richard Henderson
> Signed-off-by: Emilio G. Cota
Acked-by: David Gibson
> ---
> target/ppc/helper_regs.h| 2 +-
> hw/ppc/e500.c | 4 ++--
> hw/ppc/ppc.c| 10 +-
> hw/ppc/ppce500_spin.c | 6 --
> hw/ppc/spapr_cpu_core.c | 4 ++--
> hw/ppc/spapr_hcall.c| 4 +++-
> hw/ppc/spapr_rtas.c | 6 +++---
> target/ppc/excp_helper.c| 4 ++--
> target/ppc/kvm.c| 4 ++--
> target/ppc/translate_init.inc.c | 6 +++---
> 10 files changed, 27 insertions(+), 23 deletions(-)
>
> diff --git a/target/ppc/helper_regs.h b/target/ppc/helper_regs.h
> index 5efd18049e..9298052ac5 100644
> --- a/target/ppc/helper_regs.h
> +++ b/target/ppc/helper_regs.h
> @@ -161,7 +161,7 @@ static inline int hreg_store_msr(CPUPPCState *env,
> target_ulong value,
> #if !defined(CONFIG_USER_ONLY)
> if (unlikely(msr_pow == 1)) {
> if (!env->pending_interrupts && (*env->check_pow)(env)) {
> -cs->halted = 1;
> +cpu_halted_set(cs, 1);
> excp = EXCP_HALTED;
> }
> }
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index e6747fce28..6843c545b7 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -657,7 +657,7 @@ static void ppce500_cpu_reset_sec(void *opaque)
>
> /* Secondary CPU starts in halted state for now. Needs to change when
> implementing non-kernel boot. */
> -cs->halted = 1;
> +cpu_halted_set(cs, 1);
> cs->exception_index = EXCP_HLT;
> }
>
> @@ -671,7 +671,7 @@ static void ppce500_cpu_reset(void *opaque)
> cpu_reset(cs);
>
> /* Set initial guest state. */
> -cs->halted = 0;
> +cpu_halted_set(cs, 0);
> env->gpr[1] = (16 * MiB) - 8;
> env->gpr[3] = bi->dt_base;
> env->gpr[4] = 0;
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index ec4be25f49..d1a5a0b877 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -151,7 +151,7 @@ static void ppc6xx_set_irq(void *opaque, int pin, int
> level)
> /* XXX: Note that the only way to restart the CPU is to reset it
> */
> if (level) {
> LOG_IRQ("%s: stop the CPU\n", __func__);
> -cs->halted = 1;
> +cpu_halted_set(cs, 1);
> }
> break;
> case PPC6xx_INPUT_HRESET:
> @@ -230,10 +230,10 @@ static void ppc970_set_irq(void *opaque, int pin, int
> level)
> /* XXX: TODO: relay the signal to CKSTP_OUT pin */
> if (level) {
> LOG_IRQ("%s: stop the CPU\n", __func__);
> -cs->halted = 1;
> +cpu_halted_set(cs, 1);
> } else {
> LOG_IRQ("%s: restart the CPU\n", __func__);
> -cs->halted = 0;
> +cpu_halted_set(cs, 0);
> qemu_cpu_kick(cs);
> }
> break;
> @@ -361,10 +361,10 @@ static void ppc40x_set_irq(void *opaque, int pin, int
> level)
> /* Level sensitive - active low */
> if (level) {
> LOG_IRQ("%s: stop the CPU\n", __func__);
> -cs->halted = 1;
> +cpu_halted_set(cs, 1);
> } else {
> LOG_IRQ("%s: restart the CPU\n", __func__);
> -cs->halted = 0;
> +cpu_halted_set(cs, 0);
> qemu_cpu_kick(cs);
> }
> break;
> diff --git a/hw/ppc/ppce500_spin.c b/hw/ppc/ppce500_spin.c
> index c45fc858de..4b3532730f 100644
> --- a/hw/ppc/ppce500_spin.c
> +++ b/hw/ppc/ppce500_spin.c
> @@ -107,9 +107,11 @@ static void spin_kick(CPUState *cs, run_on_cpu_data data)
> map_start = ldq_p(>addr) & ~(map_size - 1);
> mmubooke_create_initial_mapping(env, 0, map_start, map_size);
>
> -cs->halted = 0;
> -cs->exception_index = -1;
> +cpu_mutex_lock(cs);
> +cpu_halted_set(cs, 0);
> cs->stopped = false;
> +cpu_mutex_unlock(cs);
> +cs->exception_index = -1;
> qemu_cpu_kick(cs);
> }
>
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 2398ce62c0..4c9c60b53b 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -37,7 +37,7 @@ static void spapr_cpu_reset(void *opaque)
> /* All CPUs start halted. CPU0 is unhalted from the machine level
> * reset code and the rest are explicitly started up by the guest
> * using an RTAS call */
> -cs->halted = 1;
> +cpu_halted_set(cs, 1);
>
> /* Set
Re: [Qemu-devel] [PATCH v5 63/73] ppc: convert to cpu_has_work_with_iothread_lock
On Thu, Dec 13, 2018 at 12:04:43AM -0500, Emilio G. Cota wrote:
> Soon we will call cpu_has_work without the BQL.
>
> Cc: David Gibson
> Cc: Alexander Graf
> Cc: qemu-...@nongnu.org
> Reviewed-by: Richard Henderson
> Signed-off-by: Emilio G. Cota
Acked-by: David Gibson
> ---
> target/ppc/translate_init.inc.c | 16
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 757eb6df16..cf453cb099 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -8462,6 +8462,8 @@ static bool cpu_has_work_POWER7(CPUState *cs)
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> CPUPPCState *env = >env;
>
> +g_assert(qemu_mutex_iothread_locked());
> +
> if (cpu_halted(cs)) {
> if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD)) {
> return false;
> @@ -8505,7 +8507,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
> pcc->pcr_supported = PCR_COMPAT_2_06 | PCR_COMPAT_2_05;
> pcc->init_proc = init_proc_POWER7;
> pcc->check_pow = check_pow_nocheck;
> -cc->has_work = cpu_has_work_POWER7;
> +cc->has_work_with_iothread_lock = cpu_has_work_POWER7;
> pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
> PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES |
> PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE |
> @@ -8616,6 +8618,8 @@ static bool cpu_has_work_POWER8(CPUState *cs)
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> CPUPPCState *env = >env;
>
> +g_assert(qemu_mutex_iothread_locked());
> +
> if (cpu_halted(cs)) {
> if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD)) {
> return false;
> @@ -8667,7 +8671,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
> pcc->pcr_supported = PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_COMPAT_2_05;
> pcc->init_proc = init_proc_POWER8;
> pcc->check_pow = check_pow_nocheck;
> -cc->has_work = cpu_has_work_POWER8;
> +cc->has_work_with_iothread_lock = cpu_has_work_POWER8;
> pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
> PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES |
> PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE |
> @@ -8808,6 +8812,8 @@ static bool cpu_has_work_POWER9(CPUState *cs)
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> CPUPPCState *env = >env;
>
> +g_assert(qemu_mutex_iothread_locked());
> +
> if (cpu_halted(cs)) {
> if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD)) {
> return false;
> @@ -8861,7 +8867,7 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
> PCR_COMPAT_2_05;
> pcc->init_proc = init_proc_POWER9;
> pcc->check_pow = check_pow_nocheck;
> -cc->has_work = cpu_has_work_POWER9;
> +cc->has_work_with_iothread_lock = cpu_has_work_POWER9;
> pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
> PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES |
> PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE |
> @@ -10253,6 +10259,8 @@ static bool ppc_cpu_has_work(CPUState *cs)
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> CPUPPCState *env = >env;
>
> +g_assert(qemu_mutex_iothread_locked());
> +
> return msr_ee && (cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD);
> }
>
> @@ -10452,7 +10460,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void
> *data)
> cc->class_by_name = ppc_cpu_class_by_name;
> pcc->parent_parse_features = cc->parse_features;
> cc->parse_features = ppc_cpu_parse_featurestr;
> -cc->has_work = ppc_cpu_has_work;
> +cc->has_work_with_iothread_lock = ppc_cpu_has_work;
> cc->do_interrupt = ppc_cpu_do_interrupt;
> cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
> cc->dump_state = ppc_cpu_dump_state;
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v5 05/14] hw/nvram/nrf51_nvm: Add nRF51 non-volatile memories
On Mon, Nov 26, 2018 at 05:43:59PM +, Peter Maydell wrote:
> On Mon, 26 Nov 2018 at 00:24, Steffen Görtz wrote:
> >
> > Hi Peter,
> >
> > thank you for your remarks!
> >
> > >> +};
> > >> +
> > >> +static uint64_t ficr_read(void *opaque, hwaddr offset
> > >
> > >> +value &= ~(NRF51_PAGE_SIZE - 1);
> > >> +if (value < (s->flash_size - NRF51_PAGE_SIZE)) {
> > >> +memset(s->storage + value / 4, 0xFF, NRF51_PAGE_SIZE);
> > >
> > > Can the guest try to execute from the flash storage? If so
> > > then just updating the backing storage directly like this is
> > > not sufficient to ensure that QEMU discards any now-stale
> > > translated code blocks from the affected memory.
> >
> > What else is necessary to invalidate stale blocks?
>
> You need an AddressSpace that points to the MemoryRegion(s)
> you're altering, and you need to use the operations on
> the AddressSpace like address_space_write(). These will
> under the hood do the right thing with TB invalidation.
I'm not sure about this. The memory region looks like this:
{parent_obj = {class = 0x565ee350, free = 0x0, Python Exception There is no member named keys.:
properties = 0x5672f860, ref = 1, parent = 0x566620f0}, romd_mode =
true, ram = false, subpage = false, readonly = false,
nonvolatile = false, rom_device = true, flush_coalesced_mmio = false,
global_locking = true, dirty_log_mask = 0 '\000', is_iommu = false, ram_block =
0x56768b40,
owner = 0x566620f0, ops = 0x5615d360 , opaque =
0x566620f0, container = 0x0, size = 262144, addr = 0, destructor =
0x55893f00 ,
align = 2097152, terminates = true, ram_device = false, enabled = true,
warning_printed = false, vga_logging_count = 0 '\000', alias = 0x0,
alias_offset = 0, priority = 0, subregions = {
tqh_first = 0x0, tqh_last = 0x56662778}, subregions_link = {tqe_next =
0x0, tqe_prev = 0x0}, coalesced = {tqh_first = 0x0, tqh_last = 0x56662798},
name = 0x568033d0 "nrf51_soc.flash", ioeventfd_nb = 0, ioeventfds = 0x0}
I see nothing that invalidates TBs in the address_space_write() code for
MMIO memory regions (not RAM). Only the RAM case calls
invalidate_and_set_dirty().
There are a few complications with this device:
1. Stores from the CPU are only honored when the NRF51_NVMC_CONFIG_WEN
write enable bit is set. NRF51_NVMC_ERASEPCRx and
NRF51_NVMC_ERASEALL commands use a separate erase enable bit
(NRF51_NVMC_CONFIG_EEN) and are therefore different from normal
writes.
2. Stores from the CPU can only flip 1s to 0s (this is NOR flash). When
we erase a page of flash memory it must be set to 0xff (i.e. flip
0s to 1s).
3. nrf51_nvm.c:flash_write() does not mark the page dirty for live
migration.
My questions:
1. Is the current rom+mmio device approach okay or should it be modelled
differently?
2. Erase operations cannot use ordinary address_space_write() for the
reasons mentioned above. Should this device directly call
cpu_physical_memory_set_dirty_range() and tb_invalidate_phys_range()?
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v5 09/73] ppc: convert to helper_cpu_halted_set
On Thu, Dec 13, 2018 at 12:03:49AM -0500, Emilio G. Cota wrote:
> Cc: David Gibson
> Cc: Alexander Graf
> Cc: qemu-...@nongnu.org
> Reviewed-by: Richard Henderson
> Reviewed-by: Alex Bennée
> Signed-off-by: Emilio G. Cota
Acked-by: David Gibson
> ---
> target/ppc/translate.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 2b37910248..4bb78f403e 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -1609,8 +1609,7 @@ GEN_LOGICAL2(nor, tcg_gen_nor_tl, 0x03, PPC_INTEGER);
> static void gen_pause(DisasContext *ctx)
> {
> TCGv_i32 t0 = tcg_const_i32(0);
> -tcg_gen_st_i32(t0, cpu_env,
> - -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted));
> +gen_helper_cpu_halted_set(cpu_env, t0);
> tcg_temp_free_i32(t0);
>
> /* Stop translation, this gives other CPUs a chance to run */
> @@ -3584,8 +3583,7 @@ static void gen_sync(DisasContext *ctx)
> static void gen_wait(DisasContext *ctx)
> {
> TCGv_i32 t0 = tcg_const_i32(1);
> -tcg_gen_st_i32(t0, cpu_env,
> - -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted));
> +gen_helper_cpu_halted_set(cpu_env, t0);
> tcg_temp_free_i32(t0);
> /* Stop translation, as the CPU is supposed to sleep from now */
> gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PULL 00/27] ppc-for-4.0 queue 20181213
On Fri, Dec 14, 2018 at 04:03:08PM +, Peter Maydell wrote:
> On Thu, 13 Dec 2018 at 04:01, David Gibson
> wrote:
> >
> > The following changes since commit 4b3aab204204ca742836219b97b538d90584f4f2:
> >
> > Merge remote-tracking branch
> > 'remotes/vivier2/tags/trivial-patches-pull-request' into staging
> > (2018-12-11 22:26:44 +)
> >
> > are available in the Git repository at:
> >
> > git://github.com/dgibson/qemu.git tags/ppc-for-4.0-20181213
> >
> > for you to fetch changes up to 67888a17b6683600ae3fa64ca275c737ba8a9a45:
> >
> > spapr/xive: use the VCPU id as a NVT identifier (2018-12-13 09:44:04
> > +1100)
> >
> >
> > ppc patch queue 2018-12-13
> >
> > Here's the first ppc and spapr pull request for 4.0. Highlights are:
> >
> > * The start of support for the POWER9 "XIVE" interrupt controller
> >(not complete enough to use yet, but we're getting there)
> > * A number of g_new vs. g_malloc cleanups
> > * Some IRQ wiring cleanups
> > * A fix for how we advertise NUMA nodes to the guest for pseries
> >
> > ---
>
>
> Compile errors in the windows cross-build. These look like
> they're assumptions that "long" is 64 bits, which it is not on Windows.
> For instance the PPC_BIT macro should be using the ULL suffix, not UL
> ("UL" is almost always a bug: either the constant is 32-bit, in
> which case "U" is what you want, or it's 64-bit and you need "ULL").
Bother, sorry. I got sidetracked debugging some problems that turned
out to be in master and forgot to run the windows builds.
> Using __builtin_ffsl() directly in target/ppc/cpu.h also looks
> a bit dubious -- this should be rephrased to use ctz32() or ctz64()
> instead.
>
> In file included from /home/petmay01/qemu-for-merges/hw/intc/xive.c:13:0:
> /home/petmay01/qemu-for-merges/hw/intc/xive.c: In function
> 'xive_router_notify':
> /home/petmay01/qemu-for-merges/target/ppc/cpu.h:76:33: error: overflow
> in implicit constant conversion [-Werror=overflow]
> #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
> ^
> /home/petmay01/qemu-for-merges/target/ppc/cpu.h:84:50: note: in
> definition of macro 'MASK_TO_LSH'
> # define MASK_TO_LSH(m) (__builtin_ffsl(m) - 1)
> ^
> /home/petmay01/qemu-for-merges/include/hw/ppc/xive_regs.h:129:34:
> note: in expansion of macro 'GETFIELD'
> #define GETFIELD_BE64(m, v) GETFIELD(m, be64_to_cpu(v))
> ^
> /home/petmay01/qemu-for-merges/hw/intc/xive.c:1366:28: note: in
> expansion of macro 'GETFIELD_BE64'
> GETFIELD_BE64(EAS_END_BLOCK, eas.w),
> ^
> /home/petmay01/qemu-for-merges/include/hw/ppc/xive_regs.h:120:25:
> note: in expansion of macro 'PPC_BITMASK'
> #define EAS_END_BLOCK PPC_BITMASK(4, 7)/* Destination END block# */
> ^
> /home/petmay01/qemu-for-merges/hw/intc/xive.c:1366:42: note: in
> expansion of macro 'EAS_END_BLOCK'
> GETFIELD_BE64(EAS_END_BLOCK, eas.w),
> ^
> /home/petmay01/qemu-for-merges/target/ppc/cpu.h:89:46: error: right
> shift count is negative [-Werror=shift-count-negative]
> #define GETFIELD(m, v) (((v) & (m)) >> MASK_TO_LSH(m))
> ^
> /home/petmay01/qemu-for-merges/include/hw/ppc/xive_regs.h:129:34:
> note: in expansion of macro 'GETFIELD'
> #define GETFIELD_BE64(m, v) GETFIELD(m, be64_to_cpu(v))
> ^
> /home/petmay01/qemu-for-merges/hw/intc/xive.c:1366:28: note: in
> expansion of macro 'GETFIELD_BE64'
> GETFIELD_BE64(EAS_END_BLOCK, eas.w),
> ^
> /home/petmay01/qemu-for-merges/target/ppc/cpu.h:76:33: error: overflow
> in implicit constant conversion [-Werror=overflow]
> #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
> ^
> /home/petmay01/qemu-for-merges/target/ppc/cpu.h:84:50: note: in
> definition of macro 'MASK_TO_LSH'
> # define MASK_TO_LSH(m) (__builtin_ffsl(m) - 1)
> ^
> /home/petmay01/qemu-for-merges/include/hw/ppc/xive_regs.h:129:34:
> note: in expansion of macro 'GETFIELD'
> #define GETFIELD_BE64(m, v) GETFIELD(m, be64_to_cpu(v))
> ^
> /home/petmay01/qemu-for-merges/hw/intc/xive.c:1367:28: note: in
> expansion of macro 'GETFIELD_BE64'
> GETFIELD_BE64(EAS_END_INDEX, eas.w),
> ^
> /home/petmay01/qemu-for-merges/include/hw/ppc/xive_regs.h:121:25:
> note: in expansion of macro 'PPC_BITMASK'
> #define EAS_END_INDEX PPC_BITMASK(8, 31) /* Destination END index
Re: [Qemu-devel] [PATCH v5 44/73] ppc: convert to cpu_interrupt_request
On Thu, Dec 13, 2018 at 12:04:24AM -0500, Emilio G. Cota wrote:
> Cc: David Gibson
> Cc: Alexander Graf
> Cc: qemu-...@nongnu.org
> Reviewed-by: Richard Henderson
> Signed-off-by: Emilio G. Cota
Acked-by: David Gibson
> ---
> hw/ppc/ppc.c| 2 +-
> target/ppc/excp_helper.c| 2 +-
> target/ppc/kvm.c| 4 ++--
> target/ppc/translate_init.inc.c | 14 +++---
> 4 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index d1a5a0b877..bc1cefa13f 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -91,7 +91,7 @@ void ppc_set_irq(PowerPCCPU *cpu, int n_IRQ, int level)
>
> LOG_IRQ("%s: %p n_IRQ %d level %d => pending %08" PRIx32
> "req %08x\n", __func__, env, n_IRQ, level,
> -env->pending_interrupts, CPU(cpu)->interrupt_request);
> +env->pending_interrupts, cpu_interrupt_request(CPU(cpu)));
>
> if (locked) {
> qemu_mutex_unlock_iothread();
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 737c9c72be..75a434f46b 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -753,7 +753,7 @@ static void ppc_hw_interrupt(CPUPPCState *env)
>
> qemu_log_mask(CPU_LOG_INT, "%s: %p pending %08x req %08x me %d ee %d\n",
>__func__, env, env->pending_interrupts,
> - cs->interrupt_request, (int)msr_me, (int)msr_ee);
> + cpu_interrupt_request(cs), (int)msr_me, (int)msr_ee);
> #endif
> /* External reset */
> if (env->pending_interrupts & (1 << PPC_INTERRUPT_RESET)) {
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 557faa3637..cebb73bd98 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1336,7 +1336,7 @@ void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run)
> * interrupt, reset, etc) in PPC-specific env->irq_input_state. */
> if (!cap_interrupt_level &&
> run->ready_for_interrupt_injection &&
> -(cs->interrupt_request & CPU_INTERRUPT_HARD) &&
> +(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD) &&
> (env->irq_input_state & (1< {
> /* For now KVM disregards the 'irq' argument. However, in the
> @@ -1378,7 +1378,7 @@ static int kvmppc_handle_halt(PowerPCCPU *cpu)
> CPUState *cs = CPU(cpu);
> CPUPPCState *env = >env;
>
> -if (!(cs->interrupt_request & CPU_INTERRUPT_HARD) && (msr_ee)) {
> +if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD) && (msr_ee)) {
> cpu_halted_set(cs, 1);
> cs->exception_index = EXCP_HLT;
> }
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 986bbd7eb4..757eb6df16 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -8463,7 +8463,7 @@ static bool cpu_has_work_POWER7(CPUState *cs)
> CPUPPCState *env = >env;
>
> if (cpu_halted(cs)) {
> -if (!(cs->interrupt_request & CPU_INTERRUPT_HARD)) {
> +if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD)) {
> return false;
> }
> if ((env->pending_interrupts & (1u << PPC_INTERRUPT_EXT)) &&
> @@ -8487,7 +8487,7 @@ static bool cpu_has_work_POWER7(CPUState *cs)
> }
> return false;
> } else {
> -return msr_ee && (cs->interrupt_request & CPU_INTERRUPT_HARD);
> +return msr_ee && (cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD);
> }
> }
>
> @@ -8617,7 +8617,7 @@ static bool cpu_has_work_POWER8(CPUState *cs)
> CPUPPCState *env = >env;
>
> if (cpu_halted(cs)) {
> -if (!(cs->interrupt_request & CPU_INTERRUPT_HARD)) {
> +if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD)) {
> return false;
> }
> if ((env->pending_interrupts & (1u << PPC_INTERRUPT_EXT)) &&
> @@ -8649,7 +8649,7 @@ static bool cpu_has_work_POWER8(CPUState *cs)
> }
> return false;
> } else {
> -return msr_ee && (cs->interrupt_request & CPU_INTERRUPT_HARD);
> +return msr_ee && (cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD);
> }
> }
>
> @@ -8809,7 +8809,7 @@ static bool cpu_has_work_POWER9(CPUState *cs)
> CPUPPCState *env = >env;
>
> if (cpu_halted(cs)) {
> -if (!(cs->interrupt_request & CPU_INTERRUPT_HARD)) {
> +if (!(cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD)) {
> return false;
> }
> /* External Exception */
> @@ -8842,7 +8842,7 @@ static bool cpu_has_work_POWER9(CPUState *cs)
> }
> return false;
> } else {
> -return msr_ee && (cs->interrupt_request & CPU_INTERRUPT_HARD);
> +return msr_ee && (cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD);
> }
> }
>
> @@ -10253,7 +10253,7 @@ static bool ppc_cpu_has_work(CPUState *cs)
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> CPUPPCState *env = >env;
>
> -return
[Qemu-devel] hw/input/ps2.c : BTN_SIDE and BTN_EXTRA not forwarded
Running qemu 3.1.0. virt-viewer 7.0. spice, spice-gtk, and spice-protocol all git versions from the past week or so. I have a Logitech G600 mouse. The scroll wheel can be pushed left or right. On Arch Linux host, "evtest" shows these as event codes 275 (BTN_SIDE) and 276 (BTN_EXTRA.) In host, they work as expected, by default as back and forward in supporting programs such as web browsers. On Arch Linux guest, "evtest" shows these events as supported for the "ImExPS/2 Generic Explorer Mouse", but it doesn't show those events as happening when I push the scroll wheel left or right. Other events work fine. On Windows 7 guest, there's no effect from pushing the scroll wheel left or right, either. I'm happy to help debug where the event forwarding is breaking down, but have no idea how to do that. Patch v1 for these buttons from Nov 24, 2016: https://www.mail-archive.com/qemu-devel@nongnu.org/msg415246.html Patch v2 from Nov 28, 2016: https://www.mail-archive.com/qemu-devel@nongnu.org/msg415690.html Patch v3 from Dec 6, 2016: https://www.mail-archive.com/qemu-devel@nongnu.org/msg417007.html The v1 notes say: 'Note that the guest has to switch the ps2 mouse into IMEX mode, otherwise events of the extra buttons are ignored. For example on a Windows guest one needs to manually select the "Microsoft PS/2 Mouse" driver.' I'll admit I'm not sure what IMEX mode is. QEMU is providing the PS/2 mouse emulation by default, and I don't see a way to give qemu options for it. Regardless, following this note's instructions for "IMEX mode", in Windows 7 guest, changing the driver from the default "Microsoft - PS/2 Compatible Mouse" to "Microsoft - Microsoft PS/2 Mouse" and rebooting guest has no effect. The extra buttons still don't work. Windows 7 Device Manager does show 2 "Mice and other pointing devices". First is "HID-compliant mouse" (HID\VID_0627_0001_) which shows it's USB, so I'm guessing that's the absolute movement EvTouch USB Graphics Tablet. Second is the PS/2 - currently set to "Microsoft PS/2 Mouse" (ACPI\PNP0F13).
Re: [Qemu-devel] [PULL v2 00/32] QAPI patches for 2018-12-13
On Fri, 14 Dec 2018 at 06:06, Markus Armbruster wrote: > > git-request-pull master public pull-qapi-2018-12-13-v2 > The following changes since commit c3ec0fa1a8e815ecfec9eabb9c20ee206c313e07: > > Merge remote-tracking branch 'remotes/armbru/tags/pull-monitor-2018-12-12' > into staging (2018-12-13 13:41:44 +) > > are available in the Git repository at: > > git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2018-12-13-v2 > > for you to fetch changes up to 335d10cd8e2c3bb6067804b095aaf6371fc1983e: > > qapi: add conditions to REPLICATION type/commands on the schema (2018-12-14 > 06:52:48 +0100) > > > QAPI patches for 2018-12-13 > > * Rewrite the ugly parts of string-input-visitor > * Support conditional QAPI enum, struct, union and alternate members > > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/4.0 for any user-visible changes. -- PMM
Re: [Qemu-devel] [PULL 00/32] tcg patch queue
On Fri, 14 Dec 2018 at 03:19, Richard Henderson wrote: > > The following changes since commit 2d894e48362ad2a576fca929dcca1787f43a8af6: > > Merge remote-tracking branch 'remotes/stefanha/tags/tracing-pull-request' > into staging (2018-12-13 17:50:45 +) > > are available in the Git repository at: > > https://github.com/rth7680/qemu.git tags/pull-tcg-20181213 > > for you to fetch changes up to 99f70ba5b6b4566509b2069a8d29c6686b8115de: > > xxhash: match output against the original xxhash32 (2018-12-13 18:56:11 > -0600) > > > - Remove retranslation remenents > - Return success from patch_reloc > - Preserve 32-bit values as zero-extended on x86_64 > - Make bswap during memory ops as optional > - Cleanup xxhash This didn't pass 'make check' on sparc64 host. It looks like the handful of tests that exercise TCG in the process of doing what they do failed, and there was a tcg assert in there too: ERROR:/home/pm215/qemu/tests/bios-tables-test.c:607:test_smbios_structs: assertion failed: (test_bit(data->required_struct_types[i], struct_bitmap)) ERROR:/home/pm215/qemu/tests/bios-tables-test.c:607:test_smbios_structs: assertion failed: (test_bit(data->required_struct_types[i], struct_bitmap)) /x86_64/pxe/ipv4/pc/e1000: qemu-system-x86_64: /home/pm215/qemu/tcg/sparc/tcg-target.inc.c:319: patch_reloc: Assertion `check_fit_ptr(value, 13)' failed. ERROR:/home/pm215/qemu/tests/boot-sector.c:161:boot_sector_test: assertion failed (signature == SIGNATURE): (0x == 0xdead) ERROR:/home/pm215/qemu/tests/boot-sector.c:161:boot_sector_test: assertion failed (signature == SIGNATURE): (0x == 0xdead) ERROR:/home/pm215/qemu/tests/boot-sector.c:161:boot_sector_test: assertion failed (signature == SIGNATURE): (0x == 0xdead) ERROR:/home/pm215/qemu/tests/boot-sector.c:161:boot_sector_test: assertion failed (signature == SIGNATURE): (0x == 0xdead) ERROR:/home/pm215/qemu/tests/vmgenid-test.c:104:read_guid_from_memory: assertion failed: (vmgenid_addr) ERROR:/home/pm215/qemu/tests/vmgenid-test.c:104:read_guid_from_memory: assertion failed: (vmgenid_addr) So I suspect there's a sparc TCG issue here. On s390 I also saw ERROR:/home/linux1/qemu/tests/migration-test.c:355:migrate_set_parameter: assertion failed: (qdict_haskey(rsp, "return")) but that may be an unrelated intermittent. thanks -- PMM
Re: [Qemu-devel] [PULL 0/3] 9p patches 2018-12-13
On Fri, 14 Dec 2018 19:12:07 + Peter Maydell wrote: > On Thu, 13 Dec 2018 at 09:03, Greg Kurz wrote: > > > > The following changes since commit bb9bf94b3e8926553290bc9a7cb84315af422086: > > > > Merge remote-tracking branch > > 'remotes/ehabkost/tags/machine-next-pull-request' into staging (2018-12-11 > > 19:18:58 +) > > > > are available in the Git repository at: > > > > https://github.com/gkurz/qemu.git tags/for-upstream > > > > for you to fetch changes up to 93aee84f575d46699f49af3c96194012527e0b22: > > > > 9p: remove support for the "handle" backend (2018-12-12 14:18:10 +0100) > > > > > > Most notable change in this PR is the full removal of the "handle" fsdev > > backend. > > > > > > Greg Kurz (3): > > 9p: use g_new(T, n) instead of g_malloc(sizeof(T) * n) > > xen/9pfs: use g_new(T, n) instead of g_malloc(sizeof(T) * n) > > 9p: remove support for the "handle" backend > > Applied, thanks. > > Please update the changelog at https://wiki.qemu.org/ChangeLog/4.0 > for any user-visible changes. > I've updated https://wiki.qemu.org/ChangeLog/4.0#Incompatible_changes accordingly. Cheers, -- Greg > -- PMM
[Qemu-devel] [PATCH] target/ppc: fix compilation breakage on windows
Fix the PPC_BIT definitions to use ULL instead in UL and replace __builtin_ffssl() by the equivalent ctz routines. Signed-off-by: Cédric Le Goater --- Compile tested with --cross-prefix=x86_64-w64-mingw32-. When I have some more time, I might try runtime on windows also. The PPC compile failures have been there for a while (pre 2.12) and the MASK_TO_LSH macro was not used until the XIVE definitions were introduced. I let you guys decide on how you want to proceed, but I think it is safe to merge this patch as a prereq of the pull request. Thanks, C. target/ppc/cpu.h | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index ab68abe8a23c..a3d3e91eb4ce 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -70,18 +70,18 @@ #define PPC_ELF_MACHINE EM_PPC #endif -#define PPC_BIT(bit)(0x8000UL >> (bit)) -#define PPC_BIT32(bit) (0x8000UL >> (bit)) -#define PPC_BIT8(bit) (0x80UL >> (bit)) +#define PPC_BIT(bit)(0x8000ULL >> (bit)) +#define PPC_BIT32(bit) (0x8000ULL >> (bit)) +#define PPC_BIT8(bit) (0x80ULL >> (bit)) #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs)) #define PPC_BITMASK32(bs, be) ((PPC_BIT32(bs) - PPC_BIT32(be)) | \ PPC_BIT32(bs)) #define PPC_BITMASK8(bs, be)((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs)) #if HOST_LONG_BITS == 32 -# define MASK_TO_LSH(m) (__builtin_ffsll(m) - 1) +# define MASK_TO_LSH(m) ctz32(m) #elif HOST_LONG_BITS == 64 -# define MASK_TO_LSH(m) (__builtin_ffsl(m) - 1) +# define MASK_TO_LSH(m) ctz64(m) #else # error Unknown sizeof long #endif -- 2.17.2
Re: [Qemu-devel] [PATCH v2 20/22] qemu-nbd: Add --list option
On Sat, Dec 15, 2018 at 07:53:22AM -0600, Eric Blake wrote: > @@ -557,13 +660,14 @@ int main(int argc, char **argv) > Error *local_err = NULL; > BlockdevDetectZeroesOptions detect_zeroes = > BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF; > QDict *options = NULL; > -const char *export_name = ""; /* Default export name */ > +const char *export_name = NULL; You might want to comment that export_name is now a tri-state value, with NULL meaning that it's not set by the user. Nevertheless the patch looks good, and the addition of the example (including host name setting) resolves my previous problem with the new functionality. Therefore: Reviewed-by: Richard W.M. Jones Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
Re: [Qemu-devel] [PATCH v2 19/22] nbd/client: Add meta contexts to nbd_receive_export_list()
On Sat, Dec 15, 2018 at 07:53:21AM -0600, Eric Blake wrote:
> We want to be able to detect whether a given qemu NBD server is
> exposing the right export(s) and dirty bitmaps, at least for
> regression testing. We could use 'nbd-client -l' from the upstream
> NBD project to list exports, but it's annoying to rely on
> out-of-tree binaries; furthermore, nbd-client doesn't necessarily
> know about all of the qemu NBD extensions. Thus, we plan on adding
> a new mode to qemu-nbd that merely sniffs all possible information
> from the server during handshake phase, then disconnects and dumps
> the information.
>
> This patch continues the work of the previous patch, by adding the
> ability to track the list of available meta contexts into
> NBDExportInfo. It benefits from the recent refactoring patches
> with a new nbd_list_meta_contexts() that reuses much of the same
> framework as setting a meta context.
>
> Signed-off-by: Eric Blake
>
> ---
> v2: new patch to split out collection of meta context collection
> s/free/g_free/ [Vladimir]
> ---
> include/block/nbd.h | 2 ++
> nbd/client.c| 41 +++--
> 2 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 09d2157efe0..6d9fbb941d7 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -284,6 +284,8 @@ struct NBDExportInfo {
>
> /* Set by server results during nbd_receive_export_list() */
> char *description;
> +int n_contexts;
> +char **contexts;
> };
> typedef struct NBDExportInfo NBDExportInfo;
>
> diff --git a/nbd/client.c b/nbd/client.c
> index 0e6c575ccad..d392b5e8bee 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -808,6 +808,36 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> }
> }
>
> +/*
> + * nbd_list_meta_contexts:
> + * Request the server to list all meta contexts for export @info->name.
> + * return 0 if list is complete (even if empty),
> + *-1 with errp set for any other error
> + */
> +static int nbd_list_meta_contexts(QIOChannel *ioc,
> + NBDExportInfo *info,
> + Error **errp)
> +{
> +int ret;
> +
> +if (nbd_send_one_meta_context(ioc, NBD_OPT_LIST_META_CONTEXT,
> + info->name, NULL, errp) < 0) {
> +return -1;
> +}
> +
> +while (1) {
> +char *context;
> +
> +ret = nbd_receive_one_meta_context(ioc, NBD_OPT_LIST_META_CONTEXT,
> + , NULL, errp);
> +if (ret <= 0) {
> +return ret;
> +}
> +info->contexts = g_renew(char *, info->contexts, ++info->n_contexts);
> +info->contexts[info->n_contexts - 1] = context;
> +}
> +}
> +
> /*
> * nbd_start_negotiate:
> * Start the handshake to the server. After a positive return, the server
> @@ -1054,7 +1084,7 @@ int nbd_receive_negotiate(QIOChannel *ioc,
> QCryptoTLSCreds *tlscreds,
> /* Clean up result of nbd_receive_export_list */
> void nbd_free_export_list(NBDExportInfo *info, int count)
> {
> -int i;
> +int i, j;
>
> if (!info) {
> return;
> @@ -1063,6 +1093,10 @@ void nbd_free_export_list(NBDExportInfo *info, int
> count)
> for (i = 0; i < count; i++) {
> g_free(info[i].name);
> g_free(info[i].description);
> +for (j = 0; j < info[i].n_contexts; j++) {
> +g_free(info[i].contexts[j]);
> +}
> +g_free(info[i].contexts);
> }
> g_free(info);
> }
> @@ -1130,7 +1164,10 @@ int nbd_receive_export_list(QIOChannel *ioc,
> QCryptoTLSCreds *tlscreds,
> break;
> }
>
> -/* TODO: Grab meta contexts */
> +if (result == 3 &&
> +nbd_list_meta_contexts(ioc, [i], errp) < 0) {
> +goto out;
> +}
> }
>
> /* Send NBD_OPT_ABORT as a courtesy before hanging up */
> --
> 2.17.2
Reviewed-by: Richard W.M. Jones
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
Re: [Qemu-devel] [PATCH v2 14/22] nbd/client: Split out nbd_receive_one_meta_context()
On Sat, Dec 15, 2018 at 07:53:16AM -0600, Eric Blake wrote:
> Refactor nbd_negotiate_simple_meta_context() to more closely
> resemble the pattern of nbd_receive_list(), separating the
> argument validation for one pass from the caller making a loop
> over passes. No major semantic change (although one error
> message loses the original query). The diff may be a bit hard
> to follow due to indentation changes and handling ACK first
> rather than last.
>
> Signed-off-by: Eric Blake
>
> ---
> v2: split patch into easier-to-review pieces [Rich, Vladimir]
> ---
> nbd/client.c | 144 +++
> nbd/trace-events | 2 +-
> 2 files changed, 84 insertions(+), 62 deletions(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index 5b6b9964097..0e5a9d59dbd 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -669,6 +669,83 @@ static int nbd_send_one_meta_context(QIOChannel *ioc,
> return ret;
> }
>
> +/* nbd_receive_one_meta_context:
> + * Called in a loop to receive and trace one set/list meta context reply.
> + * Pass non-NULL @name or @id to collect results back to the caller, which
> + * must eventually call g_free().
> + * return 1 if more contexts are expected,
> + *0 if operation is complete,
> + *-1 with errp set for any error
> + */
> +static int nbd_receive_one_meta_context(QIOChannel *ioc,
> +uint32_t opt,
> +char **name,
> +uint32_t *id,
> +Error **errp)
> +{
> +int ret;
> +NBDOptionReply reply;
> +char *local_name = NULL;
> +uint32_t local_id;
> +
> +if (nbd_receive_option_reply(ioc, opt, , errp) < 0) {
> +return -1;
> +}
> +
> +ret = nbd_handle_reply_err(ioc, , errp);
> +if (ret <= 0) {
> +return ret;
> +}
> +
> +if (reply.type == NBD_REP_ACK) {
> +if (reply.length != 0) {
> +error_setg(errp, "Unexpected length to ACK response");
> +nbd_send_opt_abort(ioc);
> +return -1;
> +}
> +return 0;
> +} else if (reply.type != NBD_REP_META_CONTEXT) {
> +error_setg(errp, "Unexpected reply type %u (%s), expected %u (%s)",
> + reply.type, nbd_rep_lookup(reply.type),
> + NBD_REP_META_CONTEXT,
> nbd_rep_lookup(NBD_REP_META_CONTEXT));
> +nbd_send_opt_abort(ioc);
> +return -1;
> +}
> +
> +if (reply.length <= sizeof(local_id) ||
> +reply.length > NBD_MAX_BUFFER_SIZE) {
> +error_setg(errp, "Failed to negotiate meta context, server "
> + "answered with unexpected length %" PRIu32,
> + reply.length);
> +nbd_send_opt_abort(ioc);
> +return -1;
> +}
> +
> +if (nbd_read(ioc, _id, sizeof(local_id), errp) < 0) {
> +return -1;
> +}
> +local_id = be32_to_cpu(local_id);
> +
> +reply.length -= sizeof(local_id);
> +local_name = g_malloc(reply.length + 1);
> +if (nbd_read(ioc, local_name, reply.length, errp) < 0) {
> +g_free(local_name);
> +return -1;
> +}
> +local_name[reply.length] = '\0';
> +trace_nbd_opt_meta_reply(nbd_opt_lookup(opt), local_name, local_id);
> +
> +if (name) {
> +*name = local_name;
> +} else {
> +g_free(local_name);
> +}
> +if (id) {
> +*id = local_id;
> +}
> +return 1;
> +}
> +
> /* nbd_negotiate_simple_meta_context:
> * Request the server to set the meta context for export @info->name
> * using @info->x_dirty_bitmap with a fallback to "base:allocation",
> @@ -690,7 +767,6 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> * function should lose the term _simple.
> */
> int ret;
> -NBDOptionReply reply;
> const char *context = info->x_dirty_bitmap ?: "base:allocation";
> bool received = false;
>
> @@ -699,44 +775,17 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> return -1;
> }
>
> -if (nbd_receive_option_reply(ioc, NBD_OPT_SET_META_CONTEXT, ,
> - errp) < 0)
> -{
> -return -1;
> -}
> -
> -ret = nbd_handle_reply_err(ioc, , errp);
> -if (ret <= 0) {
> -return ret;
> -}
> -
> -while (reply.type == NBD_REP_META_CONTEXT) {
> +while (1) {
> char *name;
>
> -if (reply.length <= sizeof(info->context_id) ||
> -reply.length > NBD_MAX_BUFFER_SIZE) {
> -error_setg(errp, "Failed to negotiate meta context '%s', server "
> - "answered with unexpected length %" PRIu32, context,
> - reply.length);
> -nbd_send_opt_abort(ioc);
> +ret = nbd_receive_one_meta_context(ioc, NBD_OPT_SET_META_CONTEXT,
> + , >context_id, errp);
> +
Re: [Qemu-devel] [PATCH v2 17/22] nbd/client: Pull out oldstyle size determination
On Sat, Dec 15, 2018 at 07:53:19AM -0600, Eric Blake wrote:
> Another refactoring creating nbd_negotiate_finish_oldstyle()
> for further reuse during 'qemu-nbd --list'.
>
> Signed-off-by: Eric Blake
> ---
> v2: new patch [Vladimir]
> ---
> nbd/client.c | 49 -
> 1 file changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index 8b0ae20fae8..4bdfba43068 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -811,7 +811,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> * Start the handshake to the server. After a positive return, the server
> * is ready to accept additional NBD_OPT requests.
> * Returns: negative errno: failure talking to server
> - * 0: server is oldstyle, client must still parse export size
> + * 0: server is oldstyle, must call nbd_negotiate_finish_oldstyle
> * 1: server is newstyle, but can only accept EXPORT_NAME
> * 2: server is newstyle, but lacks structured replies
> * 3: server is newstyle and set up for structured replies
> @@ -916,6 +916,36 @@ static int nbd_start_negotiate(QIOChannel *ioc,
> QCryptoTLSCreds *tlscreds,
> }
> }
>
> +/*
> + * nbd_negotiate_finish_oldstyle:
> + * Populate @info with the size and export flags from an oldstyle server,
> + * but does not consume 124 bytes of reserved zero padding.
> + * Returns 0 on success, -1 with @errp set on failure
> + */
> +static int nbd_negotiate_finish_oldstyle(QIOChannel *ioc, NBDExportInfo
> *info,
> + Error **errp)
> +{
> +uint32_t oldflags;
> +
> +if (nbd_read(ioc, >size, sizeof(info->size), errp) < 0) {
> +error_prepend(errp, "Failed to read export length: ");
> +return -EINVAL;
> +}
> +info->size = be64_to_cpu(info->size);
> +
> +if (nbd_read(ioc, , sizeof(oldflags), errp) < 0) {
> +error_prepend(errp, "Failed to read export flags: ");
> +return -EINVAL;
> +}
> +oldflags = be32_to_cpu(oldflags);
> +if (oldflags & ~0x) {
> +error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags);
> +return -EINVAL;
> +}
> +info->flags = oldflags;
> +return 0;
> +}
> +
> /*
> * nbd_receive_negotiate:
> * Connect to server, complete negotiation, and move into transmission phase.
> @@ -929,7 +959,6 @@ int nbd_receive_negotiate(QIOChannel *ioc,
> QCryptoTLSCreds *tlscreds,
> int result;
> bool zeroes = true;
> bool base_allocation = info->base_allocation;
> -uint32_t oldflags;
>
> assert(info->name);
> trace_nbd_receive_negotiate_name(info->name);
> @@ -1002,23 +1031,9 @@ int nbd_receive_negotiate(QIOChannel *ioc,
> QCryptoTLSCreds *tlscreds,
> error_setg(errp, "Server does not support non-empty export
> names");
> return -EINVAL;
> }
> -
> -if (nbd_read(ioc, >size, sizeof(info->size), errp) < 0) {
> -error_prepend(errp, "Failed to read export length: ");
> +if (nbd_negotiate_finish_oldstyle(ioc, info, errp) < 0) {
> return -EINVAL;
> }
> -info->size = be64_to_cpu(info->size);
> -
> -if (nbd_read(ioc, , sizeof(oldflags), errp) < 0) {
> -error_prepend(errp, "Failed to read export flags: ");
> -return -EINVAL;
> -}
> -oldflags = be32_to_cpu(oldflags);
> -if (oldflags & ~0x) {
> -error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags);
> -return -EINVAL;
> -}
> -info->flags = oldflags;
> break;
> default:
> return result;
Straightforward refactoring, so:
Reviewed-by: Richard W.M. Jones
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
Re: [Qemu-devel] [PATCH v2 18/22] nbd/client: Add nbd_receive_export_list()
On Sat, Dec 15, 2018 at 07:53:20AM -0600, Eric Blake wrote:
> We want to be able to detect whether a given qemu NBD server is
> exposing the right export(s) and dirty bitmaps, at least for
> regression testing. We could use 'nbd-client -l' from the upstream
> NBD project to list exports, but it's annoying to rely on
> out-of-tree binaries; furthermore, nbd-client doesn't necessarily
> know about all of the qemu NBD extensions. Thus, we plan on adding
> a new mode to qemu-nbd that merely sniffs all possible information
> from the server during handshake phase, then disconnects and dumps
> the information.
>
> This patch adds the low-level client code for grabbing the list
> of exports. It benefits from the recent refactoring patches, as
> well as a minor tweak of changing nbd_opt_go() to nbd_opt_info_or_go(),
> in order to share as much code as possible when it comes to doing
> validation of server replies. The resulting information is stored
> in an array of NBDExportInfo which has been expanded to any
> description string, along with a convenience function for freeing
> the list.
>
> Signed-off-by: Eric Blake
>
> ---
> v2: split out oldstyle size computation into earlier patch [Vladimir]
> rename nbd_opt_info_or_go [Rich]
> split out collection of meta context collection into later patch
> ---
> include/block/nbd.h | 15 -
> nbd/client.c| 138 ++--
> nbd/trace-events| 2 +-
> 3 files changed, 146 insertions(+), 9 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index ae5fe28f486..09d2157efe0 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2016-2017 Red Hat, Inc.
> + * Copyright (C) 2016-2018 Red Hat, Inc.
> * Copyright (C) 2005 Anthony Liguori
> *
> * Network Block Device
> @@ -262,6 +262,9 @@ struct NBDExportInfo {
> /* Set by client before nbd_receive_negotiate() */
> bool request_sizes;
> char *x_dirty_bitmap;
> +
> +/* Set by client before nbd_receive_negotiate(), or by server results
> + * during nbd_receive_export_list() */
> char *name; /* must be non-NULL */
>
> /* In-out fields, set by client before nbd_receive_negotiate() and
> @@ -269,7 +272,8 @@ struct NBDExportInfo {
> bool structured_reply;
> bool base_allocation; /* base:allocation context for
> NBD_CMD_BLOCK_STATUS */
>
> -/* Set by server results during nbd_receive_negotiate() */
> +/* Set by server results during nbd_receive_negotiate() and
> + * nbd_receive_export_list() */
> uint64_t size;
> uint16_t flags;
> uint32_t min_block;
> @@ -277,12 +281,19 @@ struct NBDExportInfo {
> uint32_t max_block;
>
> uint32_t context_id;
> +
> +/* Set by server results during nbd_receive_export_list() */
> +char *description;
> };
> typedef struct NBDExportInfo NBDExportInfo;
>
> int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
>const char *hostname, QIOChannel **outioc,
>NBDExportInfo *info, Error **errp);
> +void nbd_free_export_list(NBDExportInfo *info, int count);
> +int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> +const char *hostname, NBDExportInfo **info,
> +Error **errp);
> int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info,
> Error **errp);
> int nbd_send_request(QIOChannel *ioc, NBDRequest *request);
> diff --git a/nbd/client.c b/nbd/client.c
> index 4bdfba43068..0e6c575ccad 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -332,7 +332,8 @@ static int nbd_receive_list(QIOChannel *ioc, char **name,
> char **description,
> * used, 0 if NBD_OPT_GO is unsupported (fall back to NBD_OPT_LIST and
> * NBD_OPT_EXPORT_NAME in that case), and > 0 if the export is good to
> * go (with the rest of @info populated). */
> -static int nbd_opt_go(QIOChannel *ioc, NBDExportInfo *info, Error **errp)
> +static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
> + NBDExportInfo *info, Error **errp)
> {
> NBDOptionReply reply;
> uint32_t len = strlen(info->name);
> @@ -345,7 +346,8 @@ static int nbd_opt_go(QIOChannel *ioc, NBDExportInfo
> *info, Error **errp)
> * flags still 0 is a witness of a broken server. */
> info->flags = 0;
>
> -trace_nbd_opt_go_start(info->name);
> +assert(opt == NBD_OPT_GO || opt == NBD_OPT_INFO);
> +trace_nbd_opt_go_start(nbd_opt_lookup(opt), info->name);
> buf = g_malloc(4 + len + 2 + 2 * info->request_sizes + 1);
> stl_be_p(buf, len);
> memcpy(buf + 4, info->name, len);
> @@ -354,7 +356,7 @@ static int nbd_opt_go(QIOChannel *ioc, NBDExportInfo
> *info, Error **errp)
> if (info->request_sizes) {
> stw_be_p(buf + 4 + len + 2, NBD_INFO_BLOCK_SIZE);
> }
> -error =
[Qemu-devel] [Bug 1802915] Re: GTK display refresh rate is throttled
Instead of changing the value, I think there should be a function that determines the ideal "GUI_REFRESH_INTERVAL_DEFAULT" value, with the option to override it. That way, monitor greater than 60 Hz can also benefit. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1802915 Title: GTK display refresh rate is throttled Status in QEMU: New Bug description: Guest OS running with GL enabled GTK display shows a reduced refresh rate, e.g. moving cursor around with iGVT-g DMA Buf. Apparently, a default refresh interval GUI_REFRESH_INTERVAL_DEFAULT (30ms) is defined in include/ui/console.h, throttling the display refresh rate at 33Hz. To correct this throttle issue, a shorter interval (16 or 17 milliseconds) should be applied to display change listener or the default value should be modified. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1802915/+subscriptions
Re: [Qemu-devel] [PATCH v2 13/22] nbd/client: Split out nbd_send_one_meta_context()
On Sat, Dec 15, 2018 at 07:53:15AM -0600, Eric Blake wrote:
> Refactor nbd_negotiate_simple_meta_context() to pull out the
> code that can be reused to send a LIST request for 0 or 1 query.
> No semantic change.
>
> Signed-off-by: Eric Blake
>
> ---
> v2: split patch into easier-to-review pieces [Rich, Vladimir]
> ---
> nbd/client.c | 64 ++--
> nbd/trace-events | 2 +-
> 2 files changed, 46 insertions(+), 20 deletions(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index b6a85fc3ef8..5b6b9964097 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -627,6 +627,48 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
> return QIO_CHANNEL(tioc);
> }
>
> +/* nbd_send_one_meta_context:
> + * Send 0 or 1 set/list meta context queries.
> + * return 0 on success, -1 with errp set for any error
> + */
> +static int nbd_send_one_meta_context(QIOChannel *ioc,
> + uint32_t opt,
> + const char *export,
> + const char *query,
> + Error **errp)
> +{
> +int ret;
> +uint32_t export_len = strlen(export);
> +uint32_t queries = !!query;
> +uint32_t context_len = 0;
> +uint32_t data_len;
> +char *data;
> +char *p;
> +
> +data_len = sizeof(export_len) + export_len + sizeof(queries);
> +if (query) {
> +context_len = strlen(query);
> +data_len += sizeof(context_len) + context_len;
> +} else {
> +assert(opt == NBD_OPT_LIST_META_CONTEXT);
> +}
> +data = g_malloc(data_len);
> +p = data;
> +
> +trace_nbd_opt_meta_request(nbd_opt_lookup(opt), query ?: "(all)",
> export);
> +stl_be_p(p, export_len);
> +memcpy(p += sizeof(export_len), export, export_len);
> +stl_be_p(p += export_len, queries);
> +if (query) {
> +stl_be_p(p += sizeof(uint32_t), context_len);
> +memcpy(p += sizeof(context_len), query, context_len);
> +}
> +
> +ret = nbd_send_option_request(ioc, opt, data_len, data, errp);
> +g_free(data);
> +return ret;
> +}
> +
> /* nbd_negotiate_simple_meta_context:
> * Request the server to set the meta context for export @info->name
> * using @info->x_dirty_bitmap with a fallback to "base:allocation",
> @@ -651,26 +693,10 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> NBDOptionReply reply;
> const char *context = info->x_dirty_bitmap ?: "base:allocation";
> bool received = false;
> -uint32_t export_len = strlen(info->name);
> -uint32_t context_len = strlen(context);
> -uint32_t data_len = sizeof(export_len) + export_len +
> -sizeof(uint32_t) + /* number of queries */
> -sizeof(context_len) + context_len;
> -char *data = g_malloc(data_len);
> -char *p = data;
>
> -trace_nbd_opt_meta_request(context, info->name);
> -stl_be_p(p, export_len);
> -memcpy(p += sizeof(export_len), info->name, export_len);
> -stl_be_p(p += export_len, 1);
> -stl_be_p(p += sizeof(uint32_t), context_len);
> -memcpy(p += sizeof(context_len), context, context_len);
> -
> -ret = nbd_send_option_request(ioc, NBD_OPT_SET_META_CONTEXT, data_len,
> data,
> - errp);
> -g_free(data);
> -if (ret < 0) {
> -return ret;
> +if (nbd_send_one_meta_context(ioc, NBD_OPT_SET_META_CONTEXT,
> + info->name, context, errp) < 0) {
> +return -1;
> }
>
> if (nbd_receive_option_reply(ioc, NBD_OPT_SET_META_CONTEXT, ,
> diff --git a/nbd/trace-events b/nbd/trace-events
> index 446d10b8603..00872a6f9d4 100644
> --- a/nbd/trace-events
> +++ b/nbd/trace-events
> @@ -11,7 +11,7 @@ nbd_receive_query_exports_start(const char *wantname)
> "Querying export list for
> nbd_receive_query_exports_success(const char *wantname) "Found desired
> export name '%s'"
> nbd_receive_starttls_new_client(void) "Setting up TLS"
> nbd_receive_starttls_tls_handshake(void) "Starting TLS handshake"
> -nbd_opt_meta_request(const char *context, const char *export) "Requesting to
> set meta context %s for export %s"
> +nbd_opt_meta_request(const char *optname, const char *context, const char
> *export) "Requesting %s %s for export %s"
> nbd_opt_meta_reply(const char *context, uint32_t id) "Received mapping of
> context %s to id %" PRIu32
> nbd_receive_negotiate(void *tlscreds, const char *hostname) "Receiving
> negotiation tlscreds=%p hostname=%s"
> nbd_receive_negotiate_magic(uint64_t magic) "Magic is 0x%" PRIx64
Although a straightforward refactoring, we still lost the comment
/* number of queries */. I'd still perhaps like to see a bit more
explanation of the layout and reasoning behind the data buffer.
But anyway ..
Reviewed-by: Richard W.M. Jones
Rich.
--
Richard Jones, Virtualization Group, Red Hat
Re: [Qemu-devel] [PATCH v2 12/22] nbd/client: Improve error handling in nbd_negotiate_simple_meta_context()
On Sat, Dec 15, 2018 at 07:53:14AM -0600, Eric Blake wrote:
> Always allocate space for the reply returned by the server and
> hoist the trace earlier, as it is more interesting to trace the
> server's reply (even if it is unexpected) than parroting our
> request only on success. After all, skipping the allocation
> for a wrong size was merely a micro-optimization that only
> benefitted a broken server, rather than the common case of a
> compliant server that meets our expectations.
>
> Then turn the reply handling into a loop (even though we still
> never iterate more than once), to make this code easier to use
> when later patches do support multiple server replies. This
> changes the error message for a server with two replies (a
> corner case we are unlikely to hit in practice) from:
>
> Unexpected reply type 4 (meta context), expected 0 (ack)
>
> to:
>
> Server replied with more than one context
>
> Signed-off-by: Eric Blake
>
> ---
> v2: split patch into easier-to-review pieces [Rich, Vladimir]
> ---
> nbd/client.c | 16
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index bcccd5f555e..b6a85fc3ef8 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -684,10 +684,11 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> return ret;
> }
>
> -if (reply.type == NBD_REP_META_CONTEXT) {
> +while (reply.type == NBD_REP_META_CONTEXT) {
I'm not sure I understand why this change is safe.
As far as I can see reply.type is only updated in the loop by
nbd_receive_option_reply, and that reads from the server, and so the
server might keep sending NBD_REP_META_CONTEXT packets (instead of the
expected NBD_REP_ACK), so it could now loop forever against a
malicious server? (This is not taking into account any later patches)
Rich.
> char *name;
>
> -if (reply.length != sizeof(info->context_id) + context_len) {
> +if (reply.length <= sizeof(info->context_id) ||
> +reply.length > NBD_MAX_BUFFER_SIZE) {
> error_setg(errp, "Failed to negotiate meta context '%s', server "
> "answered with unexpected length %" PRIu32, context,
> reply.length);
> @@ -708,6 +709,15 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> return -1;
> }
> name[reply.length] = '\0';
> +trace_nbd_opt_meta_reply(name, info->context_id);
> +
> +if (received) {
> +error_setg(errp, "Server replied with more than one context");
> +g_free(name);
> +nbd_send_opt_abort(ioc);
> +return -1;
> +}
> +
> if (strcmp(context, name)) {
> error_setg(errp, "Failed to negotiate meta context '%s', server "
> "answered with different context '%s'", context,
> @@ -717,8 +727,6 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> return -1;
> }
> g_free(name);
> -
> -trace_nbd_opt_meta_reply(context, info->context_id);
> received = true;
>
> /* receive NBD_REP_ACK */
> --
> 2.17.2
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
Re: [Qemu-devel] [PATCH v2 09/22] nbd/client: Refactor nbd_receive_list()
On Sat, Dec 15, 2018 at 07:53:11AM -0600, Eric Blake wrote:
> Right now, nbd_receive_list() is only called by
> nbd_receive_query_exports(), which in turn is only called if the
> server lacks NBD_OPT_GO but has working option negotiation, and is
> merely used as a quality-of-implementation trick since servers
> can't give decent errors for NBD_OPT_EXPORT_NAME. However, servers
> that lack NBD_OPT_GO are becoming increasingly rare (nbdkit was a
> latecomer, in Aug 2018, but qemu has been such a server since commit
> f37708f6 in July 2017 and released in 2.10), so it no longer makes
> sense to micro-optimize that function for performance.
>
> Furthermore, when debugging a server's implementation, tracing the
> full reply (both names and descriptions) is useful, not to mention
> that upcoming patches adding 'qemu-nbd --list' will want to collect
> that data. And when you consider that a server can send an export
> name up to the NBD protocol length limit of 4k; but our current
> NBD_MAX_NAME_SIZE is only 256, we can't trace all valid server
> names without more storage, but 4k is large enough that the heap
> is better than the stack for long names.
>
> Thus, I'm changing the division of labor, with nbd_receive_list()
> now always malloc'ing a result on success (the malloc is bounded
> by the fact that we reject servers with a reply length larger
> than 32M), and moving the comparison to 'wantname' to the caller.
>
> There is a minor change in behavior where a server with 0 exports
> (an immediate NBD_REP_ACK reply) is now no longer distinguished
> from a server without LIST support (NBD_REP_ERR_UNSUP); this
> information could be preserved with a complication to the calling
> contract to provide a bit more information, but I didn't see the
> point. After all, the worst that can happen if our guess at a
> match is wrong is that the caller will get a cryptic disconnect
> when NBD_OPT_EXPORT_NAME fails (which is no different from what
> would happen if we had not tried LIST), while treating an empty
> list as immediate failure would prevent connecting to really old
> servers that really did lack LIST. Besides, NBD servers with 0
> exports are rare (qemu can do it when using QMP nbd-server-start
> without nbd-server-add - but qemu understands NBD_OPT_GO and
> thus won't tickle this change in behavior).
>
> Signed-off-by: Eric Blake
>
> ---
> v2: Rewrite in a different manner (move the comparison to the
> caller)
> Fix free to g_free [Vladimir]
> ---
> nbd/client.c | 89 +++-
> nbd/trace-events | 1 +
> 2 files changed, 58 insertions(+), 32 deletions(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index 1a3a620fb6d..28f5a286cba 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -232,18 +232,24 @@ static int nbd_handle_reply_err(QIOChannel *ioc,
> NBDOptionReply *reply,
> return result;
> }
>
> -/* Process another portion of the NBD_OPT_LIST reply. Set *@match if
> - * the current reply matches @want or if the server does not support
> - * NBD_OPT_LIST, otherwise leave @match alone. Return 0 if iteration
> - * is complete, positive if more replies are expected, or negative
> - * with @errp set if an unrecoverable error occurred. */
> -static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
> +/* nbd_receive_list:
> + * Process another portion of the NBD_OPT_LIST reply, populating any
> + * name received into *@name. If @description is non-NULL, and the
> + * server provided a description, that is also populated. The caller
> + * must eventually call g_free() on success.
> + * Returns 1 if a name was received and iteration must continue,
> + * 0 if iteration is complete,
> + * -1 with @errp set if an unrecoverable error occurred.
> + */
> +static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,
> Error **errp)
> {
> +int ret = -1;
> NBDOptionReply reply;
> uint32_t len;
> uint32_t namelen;
> -char name[NBD_MAX_NAME_SIZE + 1];
> +char *local_name = NULL;
> +char *local_desc = NULL;
> int error;
>
> if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, , errp) < 0) {
> @@ -251,9 +257,6 @@ static int nbd_receive_list(QIOChannel *ioc, const char
> *want, bool *match,
> }
> error = nbd_handle_reply_err(ioc, , errp);
> if (error <= 0) {
> -/* The server did not support NBD_OPT_LIST, so set *match on
> - * the assumption that any name will be accepted. */
> -*match = true;
> return error;
> }
> len = reply.length;
> @@ -290,33 +293,38 @@ static int nbd_receive_list(QIOChannel *ioc, const char
> *want, bool *match,
> nbd_send_opt_abort(ioc);
> return -1;
> }
> -if (namelen != strlen(want)) {
> -if (nbd_drop(ioc, len, errp) < 0) {
> -error_prepend(errp,
> - "failed to skip export name with wrong
Re: [Qemu-devel] [PATCH v2 11/22] nbd/client: Change signature of nbd_negotiate_simple_meta_context()
On Sat, Dec 15, 2018 at 07:53:13AM -0600, Eric Blake wrote:
> Pass 'info' instead of three separate parameters related to info,
> when requesting the server to set the meta context. Update the
> NBDExportInfo struct to rename the received id field to match the
> fact that we are currently overloading the field to match whatever
> context the user supplied through the x-dirty-bitmap hack, as well
> as adding a TODO comment to remind future patches about a desire
> to request two contexts at once.
>
> Signed-off-by: Eric Blake
> ---
> v2: split patch into easier-to-review pieces [Rich, Vladimir]
> rename NBDExportInfo meta_base_allocation_id [Vladimir]
> ---
> include/block/nbd.h | 2 +-
> block/nbd-client.c | 4 ++--
> nbd/client.c| 53 +
> 3 files changed, 28 insertions(+), 31 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 65feff8ba96..ae5fe28f486 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -276,7 +276,7 @@ struct NBDExportInfo {
> uint32_t opt_block;
> uint32_t max_block;
>
> -uint32_t meta_base_allocation_id;
> +uint32_t context_id;
> };
> typedef struct NBDExportInfo NBDExportInfo;
>
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 417971d8b05..608b578e1d3 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -246,11 +246,11 @@ static int
> nbd_parse_blockstatus_payload(NBDClientSession *client,
> }
>
> context_id = payload_advance32();
> -if (client->info.meta_base_allocation_id != context_id) {
> +if (client->info.context_id != context_id) {
> error_setg(errp, "Protocol error: unexpected context id %d for "
> "NBD_REPLY_TYPE_BLOCK_STATUS, when negotiated
> context "
> "id is %d", context_id,
> - client->info.meta_base_allocation_id);
> + client->info.context_id);
> return -EINVAL;
> }
>
> diff --git a/nbd/client.c b/nbd/client.c
> index 7462fa5ae0e..bcccd5f555e 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -628,26 +628,30 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
> }
>
> /* nbd_negotiate_simple_meta_context:
> - * Set one meta context. Simple means that reply must contain zero (not
> - * negotiated) or one (negotiated) contexts. More contexts would be
> considered
> - * as a protocol error. It's also implied that meta-data query equals queried
> - * context name, so, if server replies with something different than
> @context,
> - * it is considered an error too.
> - * return 1 for successful negotiation, context_id is set
> + * Request the server to set the meta context for export @info->name
> + * using @info->x_dirty_bitmap with a fallback to "base:allocation",
> + * setting @info->context_id to the resulting id. Fail if the server
> + * responds with more than one context or with a context different
> + * than the query.
> + * return 1 for successful negotiation,
> *0 if operation is unsupported,
> *-1 with errp set for any other error
> */
> static int nbd_negotiate_simple_meta_context(QIOChannel *ioc,
> - const char *export,
> - const char *context,
> - uint32_t *context_id,
> + NBDExportInfo *info,
> Error **errp)
> {
> +/*
> + * TODO: Removing the x_dirty_bitmap hack will mean refactoring
> + * this function to request and store ids for multiple contexts
> + * (both base:allocation and a dirty bitmap), at which point this
> + * function should lose the term _simple.
> + */
> int ret;
> NBDOptionReply reply;
> -uint32_t received_id = 0;
> +const char *context = info->x_dirty_bitmap ?: "base:allocation";
> bool received = false;
> -uint32_t export_len = strlen(export);
> +uint32_t export_len = strlen(info->name);
> uint32_t context_len = strlen(context);
> uint32_t data_len = sizeof(export_len) + export_len +
> sizeof(uint32_t) + /* number of queries */
> @@ -655,9 +659,9 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> char *data = g_malloc(data_len);
> char *p = data;
>
> -trace_nbd_opt_meta_request(context, export);
> +trace_nbd_opt_meta_request(context, info->name);
> stl_be_p(p, export_len);
> -memcpy(p += sizeof(export_len), export, export_len);
> +memcpy(p += sizeof(export_len), info->name, export_len);
> stl_be_p(p += export_len, 1);
> stl_be_p(p += sizeof(uint32_t), context_len);
> memcpy(p += sizeof(context_len), context, context_len);
> @@ -683,7 +687,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
> *ioc,
> if (reply.type ==
Re: [Qemu-devel] [PATCH v2 07/22] qemu-nbd: Avoid strtol open-coding
On Sat, Dec 15, 2018 at 07:53:09AM -0600, Eric Blake wrote:
> Our copy-and-pasted open-coding of strtol handling forgot to
> handle overflow conditions. Use qemu_strto*() instead.
>
> In the case of --partition, since we insist on a user-supplied
> partition to be non-zero, we can use 0 rather than -1 for our
> initial value to distinguish when a partition is not being
> served, for slightly more optimal code.
>
> The error messages for out-of-bounds values are less specific,
> but should not be a terrible loss in quality.
>
> Signed-off-by: Eric Blake
>
> ---
> v2: Retitle, catch more uses of strtol
> [Hmm - this depends on int64_t and off_t being compatible; if they
> aren't that way on all platforms, I'll need a temporary variable]
> ---
> qemu-nbd.c | 33 ++---
> 1 file changed, 10 insertions(+), 23 deletions(-)
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 2807e132396..e3f739671b5 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -544,9 +544,8 @@ int main(int argc, char **argv)
> };
> int ch;
> int opt_ind = 0;
> -char *end;
> int flags = BDRV_O_RDWR;
> -int partition = -1;
> +int partition = 0;
> int ret = 0;
> bool seen_cache = false;
> bool seen_discard = false;
> @@ -657,13 +656,9 @@ int main(int argc, char **argv)
> port = optarg;
> break;
> case 'o':
> -dev_offset = strtoll (optarg, , 0);
> -if (*end) {
> -error_report("Invalid offset `%s'", optarg);
> -exit(EXIT_FAILURE);
> -}
> -if (dev_offset < 0) {
> -error_report("Offset must be positive `%s'", optarg);
> +if (qemu_strtoi64(optarg, NULL, 0, _offset) < 0 ||
> +dev_offset < 0) {
> +error_report("Invalid offset '%s'", optarg);
> exit(EXIT_FAILURE);
> }
> break;
> @@ -685,13 +680,9 @@ int main(int argc, char **argv)
> flags &= ~BDRV_O_RDWR;
> break;
> case 'P':
> -partition = strtol(optarg, , 0);
> -if (*end) {
> -error_report("Invalid partition `%s'", optarg);
> -exit(EXIT_FAILURE);
> -}
> -if (partition < 1 || partition > 8) {
> -error_report("Invalid partition %d", partition);
> +if (qemu_strtoi(optarg, NULL, 0, ) < 0 ||
> +partition < 1 || partition > 8) {
> +error_report("Invalid partition '%s'", optarg);
> exit(EXIT_FAILURE);
> }
> break;
> @@ -709,15 +700,11 @@ int main(int argc, char **argv)
> device = optarg;
> break;
> case 'e':
> -shared = strtol(optarg, , 0);
> -if (*end) {
> +if (qemu_strtoi(optarg, NULL, 0, ) < 0 ||
> +shared < 1) {
> error_report("Invalid shared device number '%s'", optarg);
> exit(EXIT_FAILURE);
> }
> -if (shared < 1) {
> -error_report("Shared device number must be greater than 0");
> -exit(EXIT_FAILURE);
> -}
> break;
> case 'f':
> fmt = optarg;
> @@ -1006,7 +993,7 @@ int main(int argc, char **argv)
> }
> fd_size -= dev_offset;
>
> -if (partition != -1) {
> +if (partition) {
> ret = find_partition(blk, partition, _offset, _size);
> if (ret < 0) {
> error_report("Could not find partition %d: %s", partition,
Using qemu_strtoi has made the patch slightly bigger that last time
but is still a simplification, so:
Reviewed-by: Richard W.M. Jones
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
Re: [Qemu-devel] [PATCH v2 02/22] nbd: Document timeline of various features
On Sat, Dec 15, 2018 at 07:53:04AM -0600, Eric Blake wrote: > It can be useful to figure out which NBD protocol features are > exposed by a server, as well as what features a client will > take advantage of if available, for a given qemu release. It's > not always precise to base features on version numbers (thanks > to downstream backports), but any documentation is better than > making users search through git logs themselves. > > This patch originally stemmed from a request to document that > pristine 3.0 has a known bug where NBD_OPT_LIST_META_CONTEXT > with 0 queries forgot to advertise an available > "qemu:dirty-bitmap" context, but documenting bugs like this (or > the fact that 3.0 also botched NBD_CMD_CACHE) gets to be too > much details, especially since buggy releases will be less > likely connection targets over time. Instead, I chose to just > remind users to check stable release branches. > > Suggested-by: Vladimir Sementsov-Ogievskiy > Signed-off-by: Eric Blake > > --- > v2: new patch > --- > docs/interop/nbd.txt | 19 ++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt > index 77b5f459111..2b25f871e7c 100644 > --- a/docs/interop/nbd.txt > +++ b/docs/interop/nbd.txt > @@ -15,7 +15,6 @@ Qemu supports the "base:allocation" metadata context as > defined in the > NBD protocol specification, and also defines an additional metadata > namespace "qemu". > > - > == "qemu" namespace == > > The "qemu" namespace currently contains only one type of context, > @@ -36,3 +35,21 @@ in addition to > "qemu:dirty-bitmap:": > namespace. > * "qemu:dirty-bitmap:" - returns list of all available dirty-bitmap > metadata contexts. > + > += Features by version = > + > +The following list documents which qemu version first implemented > +various features (both as a server exposing the feature, and as a > +client taking advantage of the feature when present), to make it > +easier to plan for cross-version interoperability. Note that in > +several cases, the initial release containing a feature may require > +additional patches from the corresponding stable branch to fix bugs in > +the operation of that feature. > + > +* 2.6: NBD_OPT_STARTTLS with TLS X.509 Certificates > +* 2.8: NBD_CMD_WRITE_ZEROES > +* 2.10: NBD_OPT_GO, NBD_INFO_BLOCK > +* 2.11: NBD_OPT_STRUCTURED_READ > +* 2.12: NBD_CMD_BLOCK_STATUS for "base:allocation" > +* 3.0: NBD_OPT_STARTTLS with TLS Pre-Shared Keys (PSK), > +NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:", NBD_CMD_CACHE Sensible documentation change. Reviewed-by: Richard W.M. Jones Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
Re: [Qemu-devel] [PATCH v2 06/22] qemu-nbd: Fail earlier for -c/-d on non-linux
On Sat, Dec 15, 2018 at 07:53:08AM -0600, Eric Blake wrote:
> Connecting to a /dev/nbdN device is a Linux-specific action.
> We were already masking -c and -d from 'qemu-nbd --help' on
> non-linux. However, while -d fails with a sensible error
> message, it took hunting through a couple of files to prove
> that. What's more, the code for -c doesn't fail until after
> it has created a pthread and tried to open a device - possibly
> even printing an error message with %m on a non-Linux platform
> in spite of the comment that %m is glibc-specific. Make the
> failure happen sooner, then get rid of stubs that are no
> longer needed because of the early exits.
>
> While at it: tweak the blank newlines in --help output to be
> consistent, whether or not built on Linux.
>
> Signed-off-by: Eric Blake
>
> ---
> v2: Hoist -c error message to share with -d message [Vladimir]
> Bonus: gets rid of a stray TAB in nbd/client.c
> ---
> nbd/client.c | 18 +-
> qemu-nbd.c | 21 +++--
> 2 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index 5d59d5ba78a..3d9086af39d 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -1029,23 +1029,7 @@ int nbd_disconnect(int fd)
> return 0;
> }
>
> -#else
> -int nbd_init(int fd, QIOChannelSocket *ioc, NBDExportInfo *info,
> - Error **errp)
> -{
> -error_setg(errp, "nbd_init is only supported on Linux");
> -return -ENOTSUP;
> -}
> -
> -int nbd_client(int fd)
> -{
> -return -ENOTSUP;
> -}
> -int nbd_disconnect(int fd)
> -{
> -return -ENOTSUP;
> -}
> -#endif
> +#endif /* __linux__ */
>
> int nbd_send_request(QIOChannel *ioc, NBDRequest *request)
> {
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index e169b839ece..2807e132396 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -43,6 +43,12 @@
> #include "trace/control.h"
> #include "qemu-version.h"
>
> +#ifdef __linux__
> +#define HAVE_NBD_DEVICE 1
> +#else
> +#define HAVE_NBD_DEVICE 0
> +#endif
> +
> #define SOCKET_PATH"/var/lock/qemu-nbd-%s"
> #define QEMU_NBD_OPT_CACHE 256
> #define QEMU_NBD_OPT_AIO 257
> @@ -98,11 +104,11 @@ static void usage(const char *name)
> "specify tracing options\n"
> " --forkfork off the server process and exit the
> parent\n"
> "once the server is running\n"
> -#ifdef __linux__
> +#if HAVE_NBD_DEVICE
> +"\n"
> "Kernel NBD client support:\n"
> " -c, --connect=DEV connect FILE to the local NBD device DEV\n"
> " -d, --disconnect disconnect the specified device\n"
> -"\n"
> #endif
> "\n"
> "Block device options:\n"
> @@ -236,6 +242,7 @@ static void termsig_handler(int signum)
> }
>
>
> +#if HAVE_NBD_DEVICE
> static void *show_parts(void *arg)
> {
> char *device = arg;
> @@ -321,6 +328,7 @@ out:
> kill(getpid(), SIGTERM);
> return (void *) EXIT_FAILURE;
> }
> +#endif /* HAVE_NBD_DEVICE */
>
> static int nbd_can_accept(void)
> {
> @@ -814,6 +822,12 @@ int main(int argc, char **argv)
> }
> }
>
> +#if !HAVE_NBD_DEVICE
> +if (disconnect || device) {
> +error_report("Kernel /dev/nbdN support not available");
> +exit(EXIT_FAILURE);
> +}
> +#else /* HAVE_NBD_DEVICE */
> if (disconnect) {
> int nbdfd = open(argv[optind], O_RDWR);
> if (nbdfd < 0) {
> @@ -829,6 +843,7 @@ int main(int argc, char **argv)
>
> return 0;
> }
> +#endif
>
> if ((device && !verbose) || fork_process) {
> int stderr_fd[2];
> @@ -1006,6 +1021,7 @@ int main(int argc, char **argv)
> nbd_export_set_description(exp, export_description);
>
> if (device) {
> +#if HAVE_NBD_DEVICE
> int ret;
>
> ret = pthread_create(_thread, NULL, nbd_client_thread,
> device);
> @@ -1013,6 +1029,7 @@ int main(int argc, char **argv)
> error_report("Failed to create client thread: %s",
> strerror(ret));
> exit(EXIT_FAILURE);
> }
> +#endif
> } else {
> /* Shut up GCC warnings. */
> memset(_thread, 0, sizeof(client_thread));
> --
An obvious simplification of the last version, so:
Reviewed-by: Richard W.M. Jones
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
Re: [Qemu-devel] [PATCH v2 03/22] maint: Allow for EXAMPLES in texi2pod
On Sat, Dec 15, 2018 at 07:53:05AM -0600, Eric Blake wrote:
> The next commit will add an EXAMPLES section to qemu-nbd.8;
> for that to work, we need to recognize EXAMPLES in texi2pod,
> and we need to make all man pages be regenerated since the
> output of texi2pod can be different.
>
> Signed-off-by: Eric Blake
>
> ---
> v2: new patch
> ---
> Makefile| 18 ++
> scripts/texi2pod.pl | 2 +-
> 2 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index c8b9efdad4b..0bd204eff8a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -824,14 +824,16 @@ docs/interop/qemu-qmp-qapi.texi: qapi/qapi-doc.texi
> docs/interop/qemu-ga-qapi.texi: qga/qapi-generated/qga-qapi-doc.texi
> @cp -p $< $@
>
> -qemu.1: qemu-doc.texi qemu-options.texi qemu-monitor.texi
> qemu-monitor-info.texi
> -qemu.1: qemu-option-trace.texi
> -qemu-img.1: qemu-img.texi qemu-option-trace.texi qemu-img-cmds.texi
> -fsdev/virtfs-proxy-helper.1: fsdev/virtfs-proxy-helper.texi
> -qemu-nbd.8: qemu-nbd.texi qemu-option-trace.texi
> -qemu-ga.8: qemu-ga.texi
> -docs/qemu-block-drivers.7: docs/qemu-block-drivers.texi
> -docs/qemu-cpu-models.7: docs/qemu-cpu-models.texi
> +qemu.1: qemu-doc.texi qemu-options.texi qemu-monitor.texi \
> + qemu-monitor-info.texi scripts/texi2pod.pl
> +qemu.1: qemu-option-trace.texi scripts/texi2pod.pl
> +qemu-img.1: qemu-img.texi qemu-option-trace.texi qemu-img-cmds.texi \
> + scripts/texi2pod.pl
> +fsdev/virtfs-proxy-helper.1: fsdev/virtfs-proxy-helper.texi
> scripts/texi2pod.pl
> +qemu-nbd.8: qemu-nbd.texi qemu-option-trace.texi scripts/texi2pod.pl
> +qemu-ga.8: qemu-ga.texi scripts/texi2pod.pl
> +docs/qemu-block-drivers.7: docs/qemu-block-drivers.texi scripts/texi2pod.pl
> +docs/qemu-cpu-models.7: docs/qemu-cpu-models.texi scripts/texi2pod.pl
>
> html: qemu-doc.html docs/interop/qemu-qmp-ref.html
> docs/interop/qemu-ga-ref.html
> info: qemu-doc.info docs/interop/qemu-qmp-ref.info
> docs/interop/qemu-ga-ref.info
> diff --git a/scripts/texi2pod.pl b/scripts/texi2pod.pl
> index 39ce584a322..839b7917cf7 100755
> --- a/scripts/texi2pod.pl
> +++ b/scripts/texi2pod.pl
> @@ -398,7 +398,7 @@ $sects{NAME} = "$fn \- $tl\n";
> $sects{FOOTNOTES} .= "=back\n" if exists $sects{FOOTNOTES};
>
> for $sect (qw(NAME SYNOPSIS DESCRIPTION OPTIONS ENVIRONMENT FILES
> - BUGS NOTES FOOTNOTES SEEALSO AUTHOR COPYRIGHT)) {
> + BUGS NOTES FOOTNOTES EXAMPLES SEEALSO AUTHOR COPYRIGHT)) {
> if(exists $sects{$sect}) {
> $head = $sect;
> $head =~ s/SEEALSO/SEE ALSO/;
> --
> 2.17.2
Reviewed-by: Richard W.M. Jones
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
Re: [Qemu-devel] [PATCH v2 04/22] qemu-nbd: Enhance man page
On Sat, Dec 15, 2018 at 07:53:06AM -0600, Eric Blake wrote: > Document some useful qemu-nbd command lines. Mention some restrictions > on particular options, like -p being only for MBR images, or -c/-d > being Linux-only. Update some text given the recent change to no > longer serve oldstyle protocol (missed in commit 7f7dfe2a). Also, > consistently use trailing '.' in describing options. > > Signed-off-by: Eric Blake > > --- > v2: new patch > --- > qemu-nbd.texi | 85 +++ > 1 file changed, 66 insertions(+), 19 deletions(-) > > diff --git a/qemu-nbd.texi b/qemu-nbd.texi > index 9a84e81eed9..0e24c2801ee 100644 > --- a/qemu-nbd.texi > +++ b/qemu-nbd.texi > @@ -8,7 +8,10 @@ > > @c man begin DESCRIPTION > > -Export a QEMU disk image using the NBD protocol. > +Provide access to various QEMU NBD features. Most commonly used to > +export a QEMU disk image using the NBD protocol as a server, but can > +also be used (on Linux) to manage kernel bindings of a /dev/nbdX > +block device to a QEMU server. This is only a minor quibble, but I thought the original text was a good summary, and only needs additional paragraphs describing the more minor use cases. Thus the description would become by the end of the patch series: DESCRIPTION Export a QEMU disk image using the NBD protocol. Other uses: * (On Linux) bind /dev/nbdX block device to a QEMU server. * As a client to query exports of a remote NBD server. > +@c man begin EXAMPLES > +Start a server listening on port 10809 that exposes only the > +guest-visible contents of a qcow2 file, with no TLS encryption, and > +with the default export name (an empty string). The command will block > +until the first successful client disconnects: TBH I'd always include the -t option in every example. I don't understand (except for backwards compatibility) why it isn't the default since it's something I always trip over when using qemu-nbd. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
[Qemu-devel] [PATCH v2 20/22] qemu-nbd: Add --list option
We want to be able to detect whether a given qemu NBD server is
exposing the right export(s) and dirty bitmaps, at least for
regression testing. We could use 'nbd-client -l' from the upstream
NBD project to list exports, but it's annoying to rely on
out-of-tree binaries; furthermore, nbd-client doesn't necessarily
know about all of the qemu NBD extensions. Thus, it is time to add
a new mode to qemu-nbd that merely sniffs all possible information
from the server during handshake phase, then disconnects and dumps
the information.
This patch actually implements --list/-L, while reusing other
options such as --tls-creds for now designating how to connect
as the client (rather than their non-list usage of how to operate
as the server).
I debated about adding this functionality to something akin to
'qemu-img info' - but that tool does not readily lend itself
to connecting to an arbitrary NBD server without also tying to
a specific export (I may, however, still add ImageInfoSpecificNBD
for reporting the bitmaps available when connecting to a single
export). And, while it may feel a bit odd that normally
qemu-nbd is a server but 'qemu-nbd -L' is a client, we are not
really making the qemu-nbd binary that much larger, because
'qemu-nbd -c' has to operate as both server and client
simultaneously across two threads when feeding the kernel module
for /dev/nbdN access.
Sample output:
$ qemu-nbd -L
exports available: 1
export: ''
size: 65536
flags: 0x4ed ( flush fua trim zeroes df cache )
min block: 512
opt block: 4096
max block: 33554432
available meta contexts: 1
base:allocation
Note that the output only lists sizes if the server sent
NBD_FLAG_HAS_FLAGS, because a newstyle server does not give
the size otherwise. It has the side effect that for really
old servers that did not send any flags, the size is not
output even though it was available. However, I'm not too
concerned about that - oldstyle servers are (rightfully)
getting less common to encounter (qemu 3.0 was the last
version where we even serve it), and most existing servers
that still even offer oldstyle negotiation (such as nbdkit)
still send flags (since that was added to the NBD protocol
in 2007 to permit read-only connections).
Not done here, but maybe worth future experiments: capture
the meat of NBDExportInfo into a QAPI struct, and use the
generated QAPI pretty-printers instead of hand-rolling our
output loop. It would also permit us to add a JSON output
mode for machine parsing.
Signed-off-by: Eric Blake
---
v2: commit message improvements [Vladimir]
wording tweak to --help output [Vladimir]
update man page documentation
---
qemu-nbd.texi | 30 --
qemu-nbd.c| 155 +-
2 files changed, 166 insertions(+), 19 deletions(-)
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 0e24c2801ee..1e168c10850 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -2,6 +2,8 @@
@c man begin SYNOPSIS
@command{qemu-nbd} [OPTION]... @var{filename}
+@command{qemu-nbd} @option{-L} [OPTION]...
+
@command{qemu-nbd} @option{-d} @var{dev}
@c man end
@end example
@@ -10,8 +12,9 @@
Provide access to various QEMU NBD features. Most commonly used to
export a QEMU disk image using the NBD protocol as a server, but can
-also be used (on Linux) to manage kernel bindings of a /dev/nbdX
-block device to a QEMU server.
+also be used as a client to query properties of a remote server, or
+(on Linux) to manage kernel bindings of a /dev/nbdX block device to a
+QEMU server.
@c man end
@@ -28,13 +31,15 @@ See the @code{qemu(1)} manual page for full details of the
properties
supported. The common object types that it makes sense to define are the
@code{secret} object, which is used to supply passwords and/or encryption
keys, and the @code{tls-creds} object, which is used to supply TLS
-credentials for the qemu-nbd server.
+credentials for the qemu-nbd server or client.
@item -p, --port=@var{port}
-The TCP port to listen on (default @samp{10809}).
+The TCP port to listen on as a server, or connect to as a client
+(default @samp{10809}).
@item -o, --offset=@var{offset}
The offset into the image.
@item -b, --bind=@var{iface}
-The interface to bind to (default @samp{0.0.0.0}).
+The interface to bind to as a server, or connect to as a client
+(default @samp{0.0.0.0}).
@item -k, --socket=@var{path}
Use a unix socket with path @var{path}.
@item --image-opts
@@ -88,10 +93,14 @@ Set the NBD volume export name (default of a zero-length
string).
@item -D, --description=@var{description}
Set the NBD volume export description, as a human-readable
string.
+@item -L, --list
+Connect as a client and list all details about the exports exposed by
+a remote NBD server.
@item --tls-creds=ID
Enable mandatory TLS encryption for the server by setting the ID
of the TLS credentials object previously created with the --object
-option.
+option; or provide the credentials needed for connecting as a client
[Qemu-devel] [PATCH v2 18/22] nbd/client: Add nbd_receive_export_list()
We want to be able to detect whether a given qemu NBD server is
exposing the right export(s) and dirty bitmaps, at least for
regression testing. We could use 'nbd-client -l' from the upstream
NBD project to list exports, but it's annoying to rely on
out-of-tree binaries; furthermore, nbd-client doesn't necessarily
know about all of the qemu NBD extensions. Thus, we plan on adding
a new mode to qemu-nbd that merely sniffs all possible information
from the server during handshake phase, then disconnects and dumps
the information.
This patch adds the low-level client code for grabbing the list
of exports. It benefits from the recent refactoring patches, as
well as a minor tweak of changing nbd_opt_go() to nbd_opt_info_or_go(),
in order to share as much code as possible when it comes to doing
validation of server replies. The resulting information is stored
in an array of NBDExportInfo which has been expanded to any
description string, along with a convenience function for freeing
the list.
Signed-off-by: Eric Blake
---
v2: split out oldstyle size computation into earlier patch [Vladimir]
rename nbd_opt_info_or_go [Rich]
split out collection of meta context collection into later patch
---
include/block/nbd.h | 15 -
nbd/client.c| 138 ++--
nbd/trace-events| 2 +-
3 files changed, 146 insertions(+), 9 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index ae5fe28f486..09d2157efe0 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016-2017 Red Hat, Inc.
+ * Copyright (C) 2016-2018 Red Hat, Inc.
* Copyright (C) 2005 Anthony Liguori
*
* Network Block Device
@@ -262,6 +262,9 @@ struct NBDExportInfo {
/* Set by client before nbd_receive_negotiate() */
bool request_sizes;
char *x_dirty_bitmap;
+
+/* Set by client before nbd_receive_negotiate(), or by server results
+ * during nbd_receive_export_list() */
char *name; /* must be non-NULL */
/* In-out fields, set by client before nbd_receive_negotiate() and
@@ -269,7 +272,8 @@ struct NBDExportInfo {
bool structured_reply;
bool base_allocation; /* base:allocation context for NBD_CMD_BLOCK_STATUS
*/
-/* Set by server results during nbd_receive_negotiate() */
+/* Set by server results during nbd_receive_negotiate() and
+ * nbd_receive_export_list() */
uint64_t size;
uint16_t flags;
uint32_t min_block;
@@ -277,12 +281,19 @@ struct NBDExportInfo {
uint32_t max_block;
uint32_t context_id;
+
+/* Set by server results during nbd_receive_export_list() */
+char *description;
};
typedef struct NBDExportInfo NBDExportInfo;
int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
const char *hostname, QIOChannel **outioc,
NBDExportInfo *info, Error **errp);
+void nbd_free_export_list(NBDExportInfo *info, int count);
+int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
+const char *hostname, NBDExportInfo **info,
+Error **errp);
int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info,
Error **errp);
int nbd_send_request(QIOChannel *ioc, NBDRequest *request);
diff --git a/nbd/client.c b/nbd/client.c
index 4bdfba43068..0e6c575ccad 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -332,7 +332,8 @@ static int nbd_receive_list(QIOChannel *ioc, char **name,
char **description,
* used, 0 if NBD_OPT_GO is unsupported (fall back to NBD_OPT_LIST and
* NBD_OPT_EXPORT_NAME in that case), and > 0 if the export is good to
* go (with the rest of @info populated). */
-static int nbd_opt_go(QIOChannel *ioc, NBDExportInfo *info, Error **errp)
+static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
+ NBDExportInfo *info, Error **errp)
{
NBDOptionReply reply;
uint32_t len = strlen(info->name);
@@ -345,7 +346,8 @@ static int nbd_opt_go(QIOChannel *ioc, NBDExportInfo *info,
Error **errp)
* flags still 0 is a witness of a broken server. */
info->flags = 0;
-trace_nbd_opt_go_start(info->name);
+assert(opt == NBD_OPT_GO || opt == NBD_OPT_INFO);
+trace_nbd_opt_go_start(nbd_opt_lookup(opt), info->name);
buf = g_malloc(4 + len + 2 + 2 * info->request_sizes + 1);
stl_be_p(buf, len);
memcpy(buf + 4, info->name, len);
@@ -354,7 +356,7 @@ static int nbd_opt_go(QIOChannel *ioc, NBDExportInfo *info,
Error **errp)
if (info->request_sizes) {
stw_be_p(buf + 4 + len + 2, NBD_INFO_BLOCK_SIZE);
}
-error = nbd_send_option_request(ioc, NBD_OPT_GO,
+error = nbd_send_option_request(ioc, opt,
4 + len + 2 + 2 * info->request_sizes,
buf, errp);
g_free(buf);
@@ -363,7 +365,7 @@ static int nbd_opt_go(QIOChannel *ioc, NBDExportInfo
[Qemu-devel] [PATCH v2 09/22] nbd/client: Refactor nbd_receive_list()
Right now, nbd_receive_list() is only called by
nbd_receive_query_exports(), which in turn is only called if the
server lacks NBD_OPT_GO but has working option negotiation, and is
merely used as a quality-of-implementation trick since servers
can't give decent errors for NBD_OPT_EXPORT_NAME. However, servers
that lack NBD_OPT_GO are becoming increasingly rare (nbdkit was a
latecomer, in Aug 2018, but qemu has been such a server since commit
f37708f6 in July 2017 and released in 2.10), so it no longer makes
sense to micro-optimize that function for performance.
Furthermore, when debugging a server's implementation, tracing the
full reply (both names and descriptions) is useful, not to mention
that upcoming patches adding 'qemu-nbd --list' will want to collect
that data. And when you consider that a server can send an export
name up to the NBD protocol length limit of 4k; but our current
NBD_MAX_NAME_SIZE is only 256, we can't trace all valid server
names without more storage, but 4k is large enough that the heap
is better than the stack for long names.
Thus, I'm changing the division of labor, with nbd_receive_list()
now always malloc'ing a result on success (the malloc is bounded
by the fact that we reject servers with a reply length larger
than 32M), and moving the comparison to 'wantname' to the caller.
There is a minor change in behavior where a server with 0 exports
(an immediate NBD_REP_ACK reply) is now no longer distinguished
from a server without LIST support (NBD_REP_ERR_UNSUP); this
information could be preserved with a complication to the calling
contract to provide a bit more information, but I didn't see the
point. After all, the worst that can happen if our guess at a
match is wrong is that the caller will get a cryptic disconnect
when NBD_OPT_EXPORT_NAME fails (which is no different from what
would happen if we had not tried LIST), while treating an empty
list as immediate failure would prevent connecting to really old
servers that really did lack LIST. Besides, NBD servers with 0
exports are rare (qemu can do it when using QMP nbd-server-start
without nbd-server-add - but qemu understands NBD_OPT_GO and
thus won't tickle this change in behavior).
Signed-off-by: Eric Blake
---
v2: Rewrite in a different manner (move the comparison to the
caller)
Fix free to g_free [Vladimir]
---
nbd/client.c | 89 +++-
nbd/trace-events | 1 +
2 files changed, 58 insertions(+), 32 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index 1a3a620fb6d..28f5a286cba 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -232,18 +232,24 @@ static int nbd_handle_reply_err(QIOChannel *ioc,
NBDOptionReply *reply,
return result;
}
-/* Process another portion of the NBD_OPT_LIST reply. Set *@match if
- * the current reply matches @want or if the server does not support
- * NBD_OPT_LIST, otherwise leave @match alone. Return 0 if iteration
- * is complete, positive if more replies are expected, or negative
- * with @errp set if an unrecoverable error occurred. */
-static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,
+/* nbd_receive_list:
+ * Process another portion of the NBD_OPT_LIST reply, populating any
+ * name received into *@name. If @description is non-NULL, and the
+ * server provided a description, that is also populated. The caller
+ * must eventually call g_free() on success.
+ * Returns 1 if a name was received and iteration must continue,
+ * 0 if iteration is complete,
+ * -1 with @errp set if an unrecoverable error occurred.
+ */
+static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,
Error **errp)
{
+int ret = -1;
NBDOptionReply reply;
uint32_t len;
uint32_t namelen;
-char name[NBD_MAX_NAME_SIZE + 1];
+char *local_name = NULL;
+char *local_desc = NULL;
int error;
if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, , errp) < 0) {
@@ -251,9 +257,6 @@ static int nbd_receive_list(QIOChannel *ioc, const char
*want, bool *match,
}
error = nbd_handle_reply_err(ioc, , errp);
if (error <= 0) {
-/* The server did not support NBD_OPT_LIST, so set *match on
- * the assumption that any name will be accepted. */
-*match = true;
return error;
}
len = reply.length;
@@ -290,33 +293,38 @@ static int nbd_receive_list(QIOChannel *ioc, const char
*want, bool *match,
nbd_send_opt_abort(ioc);
return -1;
}
-if (namelen != strlen(want)) {
-if (nbd_drop(ioc, len, errp) < 0) {
-error_prepend(errp,
- "failed to skip export name with wrong length: ");
-nbd_send_opt_abort(ioc);
-return -1;
-}
-return 1;
-}
-assert(namelen < sizeof(name));
-if (nbd_read(ioc, name, namelen, errp) < 0) {
+local_name = g_malloc(namelen + 1);
+if (nbd_read(ioc, local_name,
[Qemu-devel] [PATCH v2 21/22] nbd/client: Work around 3.0 bug for listing meta contexts
Commit 3d068aff forgot to advertise available qemu: contexts
when the client requests a list with 0 queries. Furthermore,
3.0 shipped with a qemu-img hack of x-dirty-bitmap (commit
216ee365) that _silently_ acts as though the entire image is
clean if a requested bitmap is not present. Both bugs have
been recently fixed to give full output from the start, and
to refuse a connection if a requested x-dirty-bitmap was not
found.
Still, it is likely that there will be users that have to
work with a mix of old and new qemu versions, depending on
which features get backported where, at which point being
able to rely on 'qemu-img --list' output to know for sure
whether a given NBD export has the desired dirty bitmap is
much nicer than blindly connecting and risking that the
entire image may appear clean. We can make our --list code
smart enough to work around buggy servers by tracking
whether we've seen any qemu: replies in the original 0-query
list; if not, recurse to a single query on "qemu:" (which
may still have no replies, but then we know for sure we
didn't trip up on the server bug).
Signed-off-by: Eric Blake
---
Done as a separate patch to make it easier to revert when we no
longer care about 3.0 servers
v2: rebase on top of earlier patch splits
---
nbd/client.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/nbd/client.c b/nbd/client.c
index d392b5e8bee..48fa6e10b92 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -21,6 +21,7 @@
#include "qapi/error.h"
#include "trace.h"
#include "nbd-internal.h"
+#include "qemu/cutils.h"
/* Definitions for opaque data types */
@@ -819,6 +820,8 @@ static int nbd_list_meta_contexts(QIOChannel *ioc,
Error **errp)
{
int ret;
+int seen_any = false;
+int seen_qemu = false;
if (nbd_send_one_meta_context(ioc, NBD_OPT_LIST_META_CONTEXT,
info->name, NULL, errp) < 0) {
@@ -830,9 +833,25 @@ static int nbd_list_meta_contexts(QIOChannel *ioc,
ret = nbd_receive_one_meta_context(ioc, NBD_OPT_LIST_META_CONTEXT,
, NULL, errp);
+if (ret == 0 && seen_any && !seen_qemu) {
+/*
+ * Work around qemu 3.0 bug: the server forgot to send
+ * "qemu:" replies to 0 queries. If we saw at least one
+ * reply (probably base:allocation), but none of them were
+ * qemu:, then run a more specific query to make sure.
+ */
+seen_qemu = true;
+if (nbd_send_one_meta_context(ioc, NBD_OPT_LIST_META_CONTEXT,
+ info->name, "qemu:", errp) < 0) {
+return -1;
+}
+continue;
+}
if (ret <= 0) {
return ret;
}
+seen_any = true;
+seen_qemu |= strstart(context, "qemu:", NULL);
info->contexts = g_renew(char *, info->contexts, ++info->n_contexts);
info->contexts[info->n_contexts - 1] = context;
}
--
2.17.2
[Qemu-devel] [PATCH v2 15/22] nbd/client: Refactor return of nbd_receive_negotiate()
The function could only ever return 0 or -EINVAL; make this
clearer by dropping a useless 'fail:' label.
Signed-off-by: Eric Blake
Reviewed-by: Richard W.M. Jones
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
nbd/client.c | 51 +++
1 file changed, 23 insertions(+), 28 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index 0e5a9d59dbd..64a0e5760c3 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -811,7 +811,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds
*tlscreds,
NBDExportInfo *info, Error **errp)
{
uint64_t magic;
-int rc;
bool zeroes = true;
bool structured_reply = info->structured_reply;
bool base_allocation = info->base_allocation;
@@ -822,31 +821,30 @@ int nbd_receive_negotiate(QIOChannel *ioc,
QCryptoTLSCreds *tlscreds,
trace_nbd_receive_negotiate_name(info->name);
info->structured_reply = false;
info->base_allocation = false;
-rc = -EINVAL;
if (outioc) {
*outioc = NULL;
}
if (tlscreds && !outioc) {
error_setg(errp, "Output I/O channel required for TLS");
-goto fail;
+return -EINVAL;
}
if (nbd_read(ioc, , sizeof(magic), errp) < 0) {
error_prepend(errp, "Failed to read initial magic: ");
-goto fail;
+return -EINVAL;
}
magic = be64_to_cpu(magic);
trace_nbd_receive_negotiate_magic(magic);
if (magic != NBD_INIT_MAGIC) {
error_setg(errp, "Bad initial magic received: 0x%" PRIx64, magic);
-goto fail;
+return -EINVAL;
}
if (nbd_read(ioc, , sizeof(magic), errp) < 0) {
error_prepend(errp, "Failed to read server magic: ");
-goto fail;
+return -EINVAL;
}
magic = be64_to_cpu(magic);
trace_nbd_receive_negotiate_magic(magic);
@@ -858,7 +856,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds
*tlscreds,
if (nbd_read(ioc, , sizeof(globalflags), errp) < 0) {
error_prepend(errp, "Failed to read server flags: ");
-goto fail;
+return -EINVAL;
}
globalflags = be16_to_cpu(globalflags);
trace_nbd_receive_negotiate_server_flags(globalflags);
@@ -874,18 +872,18 @@ int nbd_receive_negotiate(QIOChannel *ioc,
QCryptoTLSCreds *tlscreds,
clientflags = cpu_to_be32(clientflags);
if (nbd_write(ioc, , sizeof(clientflags), errp) < 0) {
error_prepend(errp, "Failed to send clientflags field: ");
-goto fail;
+return -EINVAL;
}
if (tlscreds) {
if (fixedNewStyle) {
*outioc = nbd_receive_starttls(ioc, tlscreds, hostname, errp);
if (!*outioc) {
-goto fail;
+return -EINVAL;
}
ioc = *outioc;
} else {
error_setg(errp, "Server does not support STARTTLS");
-goto fail;
+return -EINVAL;
}
}
if (fixedNewStyle) {
@@ -896,7 +894,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds
*tlscreds,
NBD_OPT_STRUCTURED_REPLY,
errp);
if (result < 0) {
-goto fail;
+return -EINVAL;
}
info->structured_reply = result == 1;
}
@@ -904,7 +902,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds
*tlscreds,
if (info->structured_reply && base_allocation) {
result = nbd_negotiate_simple_meta_context(ioc, info, errp);
if (result < 0) {
-goto fail;
+return -EINVAL;
}
info->base_allocation = result == 1;
}
@@ -916,7 +914,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds
*tlscreds,
* export, then use NBD_OPT_EXPORT_NAME. */
result = nbd_opt_go(ioc, info, errp);
if (result < 0) {
-goto fail;
+return -EINVAL;
}
if (result > 0) {
return 0;
@@ -928,25 +926,25 @@ int nbd_receive_negotiate(QIOChannel *ioc,
QCryptoTLSCreds *tlscreds,
* export name is not available.
*/
if (nbd_receive_query_exports(ioc, info->name, errp) < 0) {
-goto fail;
+return -EINVAL;
}
}
/* write the export name request */
if (nbd_send_option_request(ioc, NBD_OPT_EXPORT_NAME, -1, info->name,
errp) < 0) {
-goto fail;
+return -EINVAL;
}
/* Read the response */
if (nbd_read(ioc, >size, sizeof(info->size), errp) < 0) {
[Qemu-devel] [PATCH v2 19/22] nbd/client: Add meta contexts to nbd_receive_export_list()
We want to be able to detect whether a given qemu NBD server is
exposing the right export(s) and dirty bitmaps, at least for
regression testing. We could use 'nbd-client -l' from the upstream
NBD project to list exports, but it's annoying to rely on
out-of-tree binaries; furthermore, nbd-client doesn't necessarily
know about all of the qemu NBD extensions. Thus, we plan on adding
a new mode to qemu-nbd that merely sniffs all possible information
from the server during handshake phase, then disconnects and dumps
the information.
This patch continues the work of the previous patch, by adding the
ability to track the list of available meta contexts into
NBDExportInfo. It benefits from the recent refactoring patches
with a new nbd_list_meta_contexts() that reuses much of the same
framework as setting a meta context.
Signed-off-by: Eric Blake
---
v2: new patch to split out collection of meta context collection
s/free/g_free/ [Vladimir]
---
include/block/nbd.h | 2 ++
nbd/client.c| 41 +++--
2 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 09d2157efe0..6d9fbb941d7 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -284,6 +284,8 @@ struct NBDExportInfo {
/* Set by server results during nbd_receive_export_list() */
char *description;
+int n_contexts;
+char **contexts;
};
typedef struct NBDExportInfo NBDExportInfo;
diff --git a/nbd/client.c b/nbd/client.c
index 0e6c575ccad..d392b5e8bee 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -808,6 +808,36 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
*ioc,
}
}
+/*
+ * nbd_list_meta_contexts:
+ * Request the server to list all meta contexts for export @info->name.
+ * return 0 if list is complete (even if empty),
+ *-1 with errp set for any other error
+ */
+static int nbd_list_meta_contexts(QIOChannel *ioc,
+ NBDExportInfo *info,
+ Error **errp)
+{
+int ret;
+
+if (nbd_send_one_meta_context(ioc, NBD_OPT_LIST_META_CONTEXT,
+ info->name, NULL, errp) < 0) {
+return -1;
+}
+
+while (1) {
+char *context;
+
+ret = nbd_receive_one_meta_context(ioc, NBD_OPT_LIST_META_CONTEXT,
+ , NULL, errp);
+if (ret <= 0) {
+return ret;
+}
+info->contexts = g_renew(char *, info->contexts, ++info->n_contexts);
+info->contexts[info->n_contexts - 1] = context;
+}
+}
+
/*
* nbd_start_negotiate:
* Start the handshake to the server. After a positive return, the server
@@ -1054,7 +1084,7 @@ int nbd_receive_negotiate(QIOChannel *ioc,
QCryptoTLSCreds *tlscreds,
/* Clean up result of nbd_receive_export_list */
void nbd_free_export_list(NBDExportInfo *info, int count)
{
-int i;
+int i, j;
if (!info) {
return;
@@ -1063,6 +1093,10 @@ void nbd_free_export_list(NBDExportInfo *info, int count)
for (i = 0; i < count; i++) {
g_free(info[i].name);
g_free(info[i].description);
+for (j = 0; j < info[i].n_contexts; j++) {
+g_free(info[i].contexts[j]);
+}
+g_free(info[i].contexts);
}
g_free(info);
}
@@ -1130,7 +1164,10 @@ int nbd_receive_export_list(QIOChannel *ioc,
QCryptoTLSCreds *tlscreds,
break;
}
-/* TODO: Grab meta contexts */
+if (result == 3 &&
+nbd_list_meta_contexts(ioc, [i], errp) < 0) {
+goto out;
+}
}
/* Send NBD_OPT_ABORT as a courtesy before hanging up */
--
2.17.2
[Qemu-devel] [PATCH v2 22/22] iotests: Enhance 223, 233 to cover 'qemu-nbd --list'
Any good new feature deserves some regression testing :)
Coverage includes:
- 223: what happens when there are 0 or more than 1 export,
proof that we can see multiple contexts including qemu:dirty-bitmap
- 233: proof that we can list over TLS, and that mix-and-match of
plain/TLS listings will behave sanely
Signed-off-by: Eric Blake
Reviewed-by: Richard W.M. Jones
Tested-by: Richard W.M. Jones
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
v2: Factor out repeated --object text [Vladimir]
---
tests/qemu-iotests/223 | 2 ++
tests/qemu-iotests/223.out | 20
tests/qemu-iotests/233 | 19 +--
tests/qemu-iotests/233.out | 15 +++
4 files changed, 50 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index 397b865d347..e64747a9a61 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -119,6 +119,7 @@ _send_qemu_cmd $QEMU_HANDLE
'{"execute":"x-block-dirty-bitmap-disable",
_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
"arguments":{"addr":{"type":"unix",
"data":{"path":"'"$TEST_DIR/nbd"'"' "return"
+$QEMU_NBD_PROG -L -k "$TEST_DIR/nbd"
_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
"arguments":{"device":"n"}}' "return"
_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
@@ -127,6 +128,7 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
"arguments":{"device":"n", "name":"n2"}}' "return"
_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
"arguments":{"name":"n2", "bitmap":"b2"}}' "return"
+$QEMU_NBD_PROG -L -k "$TEST_DIR/nbd"
echo
echo "=== Contrast normal status to large granularity dirty-bitmap ==="
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index de417477de0..3342bff3447 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -29,10 +29,30 @@ wrote 2097152/2097152 bytes at offset 2097152
{"return": {}}
{"return": {}}
{"return": {}}
+exports available: 0
{"return": {}}
{"return": {}}
{"return": {}}
{"return": {}}
+exports available: 2
+ export: 'n'
+ size: 4194304
+ flags: 0x4ef ( readonly flush fua trim zeroes df cache )
+ min block: 512
+ opt block: 4096
+ max block: 33554432
+ available meta contexts: 2
+ base:allocation
+ qemu:dirty-bitmap:b
+ export: 'n2'
+ size: 4194304
+ flags: 0x4ef ( readonly flush fua trim zeroes df cache )
+ min block: 512
+ opt block: 4096
+ max block: 33554432
+ available meta contexts: 2
+ base:allocation
+ qemu:dirty-bitmap:b2
=== Contrast normal status to large granularity dirty-bitmap ===
diff --git a/tests/qemu-iotests/233 b/tests/qemu-iotests/233
index 1814efe..a6ef7b20fb4 100755
--- a/tests/qemu-iotests/233
+++ b/tests/qemu-iotests/233
@@ -68,10 +68,12 @@ echo
echo "== check TLS client to plain server fails =="
nbd_server_start_tcp_socket -f $IMGFMT "$TEST_IMG"
-$QEMU_IMG info --image-opts \
---object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \
+obj=tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0
+$QEMU_IMG info --image-opts --object $obj \
driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 \
2>&1 | sed "s/$nbd_tcp_port/PORT/g"
+$QEMU_NBD_PROG -L -b $nbd_tcp_addr -p $nbd_tcp_port --object $obj \
+--tls-creds=tls0
nbd_server_stop
@@ -84,20 +86,25 @@ nbd_server_start_tcp_socket \
-f $IMGFMT "$TEST_IMG"
$QEMU_IMG info nbd://localhost:$nbd_tcp_port 2>&1 | sed
"s/$nbd_tcp_port/PORT/g"
+$QEMU_NBD_PROG -L -b $nbd_tcp_addr -p $nbd_tcp_port
echo
echo "== check TLS works =="
-$QEMU_IMG info --image-opts \
---object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \
+obj=tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0
+$QEMU_IMG info --image-opts --object $obj \
driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 \
2>&1 | sed "s/$nbd_tcp_port/PORT/g"
+$QEMU_NBD_PROG -L -b $nbd_tcp_addr -p $nbd_tcp_port --object $obj \
+--tls-creds=tls0
echo
echo "== check TLS with different CA fails =="
-$QEMU_IMG info --image-opts \
---object tls-creds-x509,dir=${tls_dir}/client2,endpoint=client,id=tls0 \
+obj=tls-creds-x509,dir=${tls_dir}/client2,endpoint=client,id=tls0
+$QEMU_IMG info --image-opts --object $obj \
driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 \
2>&1 | sed "s/$nbd_tcp_port/PORT/g"
+$QEMU_NBD_PROG -L -b $nbd_tcp_addr -p $nbd_tcp_port --object $obj \
+--tls-creds=tls0
echo
echo "== perform I/O over TLS =="
diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out
index 5f416721b03..ab669488669 100644
--- a/tests/qemu-iotests/233.out
+++ b/tests/qemu-iotests/233.out
@@ -15,20 +15,35 @@ wrote 1048576/1048576 bytes at offset 1048576
== check TLS client to plain server fails ==
qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0':
Denied by server for option 5 (starttls)
server reported:
[Qemu-devel] [PATCH v2 08/22] nbd/client: Drop pointless buf variable
There's no need to read into a temporary buffer (oversized
since commit 7d3123e1) followed by a byteswap into a uint64_t
to check for a magic number via memcmp(), when the code
immediately below demonstrates reading into the uint64_t then
byteswapping in place and checking for a magic number via
integer math. What's more, having a different error message
when the server's first reply byte is 0 is unusual - it's no
different from any other wrong magic number, and we already
detected short reads. That whole strlen() issue has been
present and useless since commit 1d45f8b5 in 2010; perhaps it
was leftover debugging (since the correct magic number happens
to be ASCII)? Make the error messages more consistent and
detailed while touching things.
Signed-off-by: Eric Blake
Reviewed-by: Richard W.M. Jones
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
v2: improve commit message based on git archaeology [Rich]
added strategic comments, and improve error messages [Vladimir]
---
nbd/nbd-internal.h | 3 ++-
nbd/client.c | 22 +++---
2 files changed, 9 insertions(+), 16 deletions(-)
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index eeff78d3c98..443e177d44a 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -46,8 +46,9 @@
/* Size of oldstyle negotiation */
#define NBD_OLDSTYLE_NEGOTIATE_SIZE (8 + 8 + 8 + 4 + 124)
+#define NBD_INIT_MAGIC 0x4e42444d41474943LL /* ASCII "NBDMAGIC" */
#define NBD_REQUEST_MAGIC 0x25609513
-#define NBD_OPTS_MAGIC 0x49484156454F5054LL
+#define NBD_OPTS_MAGIC 0x49484156454F5054LL /* ASCII "IHAVEOPT" */
#define NBD_CLIENT_MAGIC0x420281861253LL
#define NBD_REP_MAGIC 0x0003e889045565a9LL
diff --git a/nbd/client.c b/nbd/client.c
index 3d9086af39d..1a3a620fb6d 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -731,7 +731,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
QIOChannel **outioc, NBDExportInfo *info,
Error **errp)
{
-char buf[256];
uint64_t magic;
int rc;
bool zeroes = true;
@@ -752,27 +751,20 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char
*name,
goto fail;
}
-if (nbd_read(ioc, buf, 8, errp) < 0) {
-error_prepend(errp, "Failed to read data: ");
+if (nbd_read(ioc, , sizeof(magic), errp) < 0) {
+error_prepend(errp, "Failed to read initial magic: ");
goto fail;
}
-
-buf[8] = '\0';
-if (strlen(buf) == 0) {
-error_setg(errp, "Server connection closed unexpectedly");
-goto fail;
-}
-
-magic = ldq_be_p(buf);
+magic = be64_to_cpu(magic);
trace_nbd_receive_negotiate_magic(magic);
-if (memcmp(buf, "NBDMAGIC", 8) != 0) {
-error_setg(errp, "Invalid magic received");
+if (magic != NBD_INIT_MAGIC) {
+error_setg(errp, "Bad initial magic received: 0x%" PRIx64, magic);
goto fail;
}
if (nbd_read(ioc, , sizeof(magic), errp) < 0) {
-error_prepend(errp, "Failed to read magic: ");
+error_prepend(errp, "Failed to read server magic: ");
goto fail;
}
magic = be64_to_cpu(magic);
@@ -911,7 +903,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
}
info->flags = oldflags;
} else {
-error_setg(errp, "Bad magic received");
+error_setg(errp, "Bad server magic received: 0x%" PRIx64, magic);
goto fail;
}
--
2.17.2
[Qemu-devel] [PATCH v2 13/22] nbd/client: Split out nbd_send_one_meta_context()
Refactor nbd_negotiate_simple_meta_context() to pull out the
code that can be reused to send a LIST request for 0 or 1 query.
No semantic change.
Signed-off-by: Eric Blake
---
v2: split patch into easier-to-review pieces [Rich, Vladimir]
---
nbd/client.c | 64 ++--
nbd/trace-events | 2 +-
2 files changed, 46 insertions(+), 20 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index b6a85fc3ef8..5b6b9964097 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -627,6 +627,48 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
return QIO_CHANNEL(tioc);
}
+/* nbd_send_one_meta_context:
+ * Send 0 or 1 set/list meta context queries.
+ * return 0 on success, -1 with errp set for any error
+ */
+static int nbd_send_one_meta_context(QIOChannel *ioc,
+ uint32_t opt,
+ const char *export,
+ const char *query,
+ Error **errp)
+{
+int ret;
+uint32_t export_len = strlen(export);
+uint32_t queries = !!query;
+uint32_t context_len = 0;
+uint32_t data_len;
+char *data;
+char *p;
+
+data_len = sizeof(export_len) + export_len + sizeof(queries);
+if (query) {
+context_len = strlen(query);
+data_len += sizeof(context_len) + context_len;
+} else {
+assert(opt == NBD_OPT_LIST_META_CONTEXT);
+}
+data = g_malloc(data_len);
+p = data;
+
+trace_nbd_opt_meta_request(nbd_opt_lookup(opt), query ?: "(all)", export);
+stl_be_p(p, export_len);
+memcpy(p += sizeof(export_len), export, export_len);
+stl_be_p(p += export_len, queries);
+if (query) {
+stl_be_p(p += sizeof(uint32_t), context_len);
+memcpy(p += sizeof(context_len), query, context_len);
+}
+
+ret = nbd_send_option_request(ioc, opt, data_len, data, errp);
+g_free(data);
+return ret;
+}
+
/* nbd_negotiate_simple_meta_context:
* Request the server to set the meta context for export @info->name
* using @info->x_dirty_bitmap with a fallback to "base:allocation",
@@ -651,26 +693,10 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
*ioc,
NBDOptionReply reply;
const char *context = info->x_dirty_bitmap ?: "base:allocation";
bool received = false;
-uint32_t export_len = strlen(info->name);
-uint32_t context_len = strlen(context);
-uint32_t data_len = sizeof(export_len) + export_len +
-sizeof(uint32_t) + /* number of queries */
-sizeof(context_len) + context_len;
-char *data = g_malloc(data_len);
-char *p = data;
-trace_nbd_opt_meta_request(context, info->name);
-stl_be_p(p, export_len);
-memcpy(p += sizeof(export_len), info->name, export_len);
-stl_be_p(p += export_len, 1);
-stl_be_p(p += sizeof(uint32_t), context_len);
-memcpy(p += sizeof(context_len), context, context_len);
-
-ret = nbd_send_option_request(ioc, NBD_OPT_SET_META_CONTEXT, data_len,
data,
- errp);
-g_free(data);
-if (ret < 0) {
-return ret;
+if (nbd_send_one_meta_context(ioc, NBD_OPT_SET_META_CONTEXT,
+ info->name, context, errp) < 0) {
+return -1;
}
if (nbd_receive_option_reply(ioc, NBD_OPT_SET_META_CONTEXT, ,
diff --git a/nbd/trace-events b/nbd/trace-events
index 446d10b8603..00872a6f9d4 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -11,7 +11,7 @@ nbd_receive_query_exports_start(const char *wantname)
"Querying export list for
nbd_receive_query_exports_success(const char *wantname) "Found desired export
name '%s'"
nbd_receive_starttls_new_client(void) "Setting up TLS"
nbd_receive_starttls_tls_handshake(void) "Starting TLS handshake"
-nbd_opt_meta_request(const char *context, const char *export) "Requesting to
set meta context %s for export %s"
+nbd_opt_meta_request(const char *optname, const char *context, const char
*export) "Requesting %s %s for export %s"
nbd_opt_meta_reply(const char *context, uint32_t id) "Received mapping of
context %s to id %" PRIu32
nbd_receive_negotiate(void *tlscreds, const char *hostname) "Receiving
negotiation tlscreds=%p hostname=%s"
nbd_receive_negotiate_magic(uint64_t magic) "Magic is 0x%" PRIx64
--
2.17.2
[Qemu-devel] [PATCH v2 17/22] nbd/client: Pull out oldstyle size determination
Another refactoring creating nbd_negotiate_finish_oldstyle()
for further reuse during 'qemu-nbd --list'.
Signed-off-by: Eric Blake
---
v2: new patch [Vladimir]
---
nbd/client.c | 49 -
1 file changed, 32 insertions(+), 17 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index 8b0ae20fae8..4bdfba43068 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -811,7 +811,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
*ioc,
* Start the handshake to the server. After a positive return, the server
* is ready to accept additional NBD_OPT requests.
* Returns: negative errno: failure talking to server
- * 0: server is oldstyle, client must still parse export size
+ * 0: server is oldstyle, must call nbd_negotiate_finish_oldstyle
* 1: server is newstyle, but can only accept EXPORT_NAME
* 2: server is newstyle, but lacks structured replies
* 3: server is newstyle and set up for structured replies
@@ -916,6 +916,36 @@ static int nbd_start_negotiate(QIOChannel *ioc,
QCryptoTLSCreds *tlscreds,
}
}
+/*
+ * nbd_negotiate_finish_oldstyle:
+ * Populate @info with the size and export flags from an oldstyle server,
+ * but does not consume 124 bytes of reserved zero padding.
+ * Returns 0 on success, -1 with @errp set on failure
+ */
+static int nbd_negotiate_finish_oldstyle(QIOChannel *ioc, NBDExportInfo *info,
+ Error **errp)
+{
+uint32_t oldflags;
+
+if (nbd_read(ioc, >size, sizeof(info->size), errp) < 0) {
+error_prepend(errp, "Failed to read export length: ");
+return -EINVAL;
+}
+info->size = be64_to_cpu(info->size);
+
+if (nbd_read(ioc, , sizeof(oldflags), errp) < 0) {
+error_prepend(errp, "Failed to read export flags: ");
+return -EINVAL;
+}
+oldflags = be32_to_cpu(oldflags);
+if (oldflags & ~0x) {
+error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags);
+return -EINVAL;
+}
+info->flags = oldflags;
+return 0;
+}
+
/*
* nbd_receive_negotiate:
* Connect to server, complete negotiation, and move into transmission phase.
@@ -929,7 +959,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds
*tlscreds,
int result;
bool zeroes = true;
bool base_allocation = info->base_allocation;
-uint32_t oldflags;
assert(info->name);
trace_nbd_receive_negotiate_name(info->name);
@@ -1002,23 +1031,9 @@ int nbd_receive_negotiate(QIOChannel *ioc,
QCryptoTLSCreds *tlscreds,
error_setg(errp, "Server does not support non-empty export names");
return -EINVAL;
}
-
-if (nbd_read(ioc, >size, sizeof(info->size), errp) < 0) {
-error_prepend(errp, "Failed to read export length: ");
+if (nbd_negotiate_finish_oldstyle(ioc, info, errp) < 0) {
return -EINVAL;
}
-info->size = be64_to_cpu(info->size);
-
-if (nbd_read(ioc, , sizeof(oldflags), errp) < 0) {
-error_prepend(errp, "Failed to read export flags: ");
-return -EINVAL;
-}
-oldflags = be32_to_cpu(oldflags);
-if (oldflags & ~0x) {
-error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags);
-return -EINVAL;
-}
-info->flags = oldflags;
break;
default:
return result;
--
2.17.2
[Qemu-devel] [PATCH v2 04/22] qemu-nbd: Enhance man page
Document some useful qemu-nbd command lines. Mention some restrictions
on particular options, like -p being only for MBR images, or -c/-d
being Linux-only. Update some text given the recent change to no
longer serve oldstyle protocol (missed in commit 7f7dfe2a). Also,
consistently use trailing '.' in describing options.
Signed-off-by: Eric Blake
---
v2: new patch
---
qemu-nbd.texi | 85 +++
1 file changed, 66 insertions(+), 19 deletions(-)
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 9a84e81eed9..0e24c2801ee 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -8,7 +8,10 @@
@c man begin DESCRIPTION
-Export a QEMU disk image using the NBD protocol.
+Provide access to various QEMU NBD features. Most commonly used to
+export a QEMU disk image using the NBD protocol as a server, but can
+also be used (on Linux) to manage kernel bindings of a /dev/nbdX
+block device to a QEMU server.
@c man end
@@ -27,28 +30,29 @@ supported. The common object types that it makes sense to
define are the
keys, and the @code{tls-creds} object, which is used to supply TLS
credentials for the qemu-nbd server.
@item -p, --port=@var{port}
-The TCP port to listen on (default @samp{10809})
+The TCP port to listen on (default @samp{10809}).
@item -o, --offset=@var{offset}
-The offset into the image
+The offset into the image.
@item -b, --bind=@var{iface}
-The interface to bind to (default @samp{0.0.0.0})
+The interface to bind to (default @samp{0.0.0.0}).
@item -k, --socket=@var{path}
-Use a unix socket with path @var{path}
+Use a unix socket with path @var{path}.
@item --image-opts
Treat @var{filename} as a set of image options, instead of a plain
filename. If this flag is specified, the @var{-f} flag should
not be used, instead the '@code{format=}' option should be set.
@item -f, --format=@var{fmt}
Force the use of the block driver for format @var{fmt} instead of
-auto-detecting
+auto-detecting.
@item -r, --read-only
-Export the disk as read-only
+Export the disk as read-only.
@item -P, --partition=@var{num}
-Only expose partition @var{num}
+Only expose MBR partition @var{num}. Understands physical partitions
+1-4 and logical partitions 5-8.
@item -s, --snapshot
Use @var{filename} as an external snapshot, create a temporary
file with backing_file=@var{filename}, redirect the write to
-the temporary one
+the temporary one.
@item -l, --load-snapshot=@var{snapshot_param}
Load an internal snapshot inside @var{filename} and export it
as an read-only device, @var{snapshot_param} format is
@@ -72,19 +76,18 @@ driver-specific optimized zero write commands.
@var{detect-zeroes} is one of
converts a zero write to an unmap operation and can only be used if
@var{discard} is set to @samp{unmap}. The default is @samp{off}.
@item -c, --connect=@var{dev}
-Connect @var{filename} to NBD device @var{dev}
+Connect @var{filename} to NBD device @var{dev} (Linux only).
@item -d, --disconnect
-Disconnect the device @var{dev}
+Disconnect the device @var{dev} (Linux only).
@item -e, --shared=@var{num}
-Allow up to @var{num} clients to share the device (default @samp{1})
+Allow up to @var{num} clients to share the device (default @samp{1}).
@item -t, --persistent
-Don't exit on the last connection
+Don't exit on the last connection.
@item -x, --export-name=@var{name}
-Set the NBD volume export name. This switches the server to use
-the new style NBD protocol negotiation
+Set the NBD volume export name (default of a zero-length string).
@item -D, --description=@var{description}
Set the NBD volume export description, as a human-readable
-string. Requires the use of @option{-x}
+string.
@item --tls-creds=ID
Enable mandatory TLS encryption for the server by setting the ID
of the TLS credentials object previously created with the --object
@@ -92,11 +95,11 @@ option.
@item --fork
Fork off the server process and exit the parent once the server is running.
@item -v, --verbose
-Display extra debugging information
+Display extra debugging information.
@item -h, --help
-Display this help and exit
+Display this help and exit.
@item -V, --version
-Display version information and exit
+Display version information and exit.
@item -T, --trace
[[enable=]@var{pattern}][,events=@var{file}][,file=@var{file}]
@findex --trace
@include qemu-option-trace.texi
@@ -104,6 +107,50 @@ Display version information and exit
@c man end
+@c man begin EXAMPLES
+Start a server listening on port 10809 that exposes only the
+guest-visible contents of a qcow2 file, with no TLS encryption, and
+with the default export name (an empty string). The command will block
+until the first successful client disconnects:
+
+@example
+qemu-nbd -f qcow2 file.qcow2
+@end example
+
+Start a server listening with encryption on port 10810, and require
+the client to have a correct X.509 certificate to connect to a 1
+megabyte subset of a raw file, using the export name 'subset':
+
+@example
+qemu-nbd \
+
[Qemu-devel] [PATCH v2 14/22] nbd/client: Split out nbd_receive_one_meta_context()
Refactor nbd_negotiate_simple_meta_context() to more closely
resemble the pattern of nbd_receive_list(), separating the
argument validation for one pass from the caller making a loop
over passes. No major semantic change (although one error
message loses the original query). The diff may be a bit hard
to follow due to indentation changes and handling ACK first
rather than last.
Signed-off-by: Eric Blake
---
v2: split patch into easier-to-review pieces [Rich, Vladimir]
---
nbd/client.c | 144 +++
nbd/trace-events | 2 +-
2 files changed, 84 insertions(+), 62 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index 5b6b9964097..0e5a9d59dbd 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -669,6 +669,83 @@ static int nbd_send_one_meta_context(QIOChannel *ioc,
return ret;
}
+/* nbd_receive_one_meta_context:
+ * Called in a loop to receive and trace one set/list meta context reply.
+ * Pass non-NULL @name or @id to collect results back to the caller, which
+ * must eventually call g_free().
+ * return 1 if more contexts are expected,
+ *0 if operation is complete,
+ *-1 with errp set for any error
+ */
+static int nbd_receive_one_meta_context(QIOChannel *ioc,
+uint32_t opt,
+char **name,
+uint32_t *id,
+Error **errp)
+{
+int ret;
+NBDOptionReply reply;
+char *local_name = NULL;
+uint32_t local_id;
+
+if (nbd_receive_option_reply(ioc, opt, , errp) < 0) {
+return -1;
+}
+
+ret = nbd_handle_reply_err(ioc, , errp);
+if (ret <= 0) {
+return ret;
+}
+
+if (reply.type == NBD_REP_ACK) {
+if (reply.length != 0) {
+error_setg(errp, "Unexpected length to ACK response");
+nbd_send_opt_abort(ioc);
+return -1;
+}
+return 0;
+} else if (reply.type != NBD_REP_META_CONTEXT) {
+error_setg(errp, "Unexpected reply type %u (%s), expected %u (%s)",
+ reply.type, nbd_rep_lookup(reply.type),
+ NBD_REP_META_CONTEXT, nbd_rep_lookup(NBD_REP_META_CONTEXT));
+nbd_send_opt_abort(ioc);
+return -1;
+}
+
+if (reply.length <= sizeof(local_id) ||
+reply.length > NBD_MAX_BUFFER_SIZE) {
+error_setg(errp, "Failed to negotiate meta context, server "
+ "answered with unexpected length %" PRIu32,
+ reply.length);
+nbd_send_opt_abort(ioc);
+return -1;
+}
+
+if (nbd_read(ioc, _id, sizeof(local_id), errp) < 0) {
+return -1;
+}
+local_id = be32_to_cpu(local_id);
+
+reply.length -= sizeof(local_id);
+local_name = g_malloc(reply.length + 1);
+if (nbd_read(ioc, local_name, reply.length, errp) < 0) {
+g_free(local_name);
+return -1;
+}
+local_name[reply.length] = '\0';
+trace_nbd_opt_meta_reply(nbd_opt_lookup(opt), local_name, local_id);
+
+if (name) {
+*name = local_name;
+} else {
+g_free(local_name);
+}
+if (id) {
+*id = local_id;
+}
+return 1;
+}
+
/* nbd_negotiate_simple_meta_context:
* Request the server to set the meta context for export @info->name
* using @info->x_dirty_bitmap with a fallback to "base:allocation",
@@ -690,7 +767,6 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
*ioc,
* function should lose the term _simple.
*/
int ret;
-NBDOptionReply reply;
const char *context = info->x_dirty_bitmap ?: "base:allocation";
bool received = false;
@@ -699,44 +775,17 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
*ioc,
return -1;
}
-if (nbd_receive_option_reply(ioc, NBD_OPT_SET_META_CONTEXT, ,
- errp) < 0)
-{
-return -1;
-}
-
-ret = nbd_handle_reply_err(ioc, , errp);
-if (ret <= 0) {
-return ret;
-}
-
-while (reply.type == NBD_REP_META_CONTEXT) {
+while (1) {
char *name;
-if (reply.length <= sizeof(info->context_id) ||
-reply.length > NBD_MAX_BUFFER_SIZE) {
-error_setg(errp, "Failed to negotiate meta context '%s', server "
- "answered with unexpected length %" PRIu32, context,
- reply.length);
-nbd_send_opt_abort(ioc);
+ret = nbd_receive_one_meta_context(ioc, NBD_OPT_SET_META_CONTEXT,
+ , >context_id, errp);
+if (ret < 0) {
return -1;
+} else if (ret == 0) {
+return received;
}
-if (nbd_read(ioc, >context_id, sizeof(info->context_id),
- errp) < 0) {
-return -1;
-}
-info->context_id = be32_to_cpu(info->context_id);
-
-reply.length -=
[Qemu-devel] [PATCH v2 07/22] qemu-nbd: Avoid strtol open-coding
Our copy-and-pasted open-coding of strtol handling forgot to
handle overflow conditions. Use qemu_strto*() instead.
In the case of --partition, since we insist on a user-supplied
partition to be non-zero, we can use 0 rather than -1 for our
initial value to distinguish when a partition is not being
served, for slightly more optimal code.
The error messages for out-of-bounds values are less specific,
but should not be a terrible loss in quality.
Signed-off-by: Eric Blake
---
v2: Retitle, catch more uses of strtol
[Hmm - this depends on int64_t and off_t being compatible; if they
aren't that way on all platforms, I'll need a temporary variable]
---
qemu-nbd.c | 33 ++---
1 file changed, 10 insertions(+), 23 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 2807e132396..e3f739671b5 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -544,9 +544,8 @@ int main(int argc, char **argv)
};
int ch;
int opt_ind = 0;
-char *end;
int flags = BDRV_O_RDWR;
-int partition = -1;
+int partition = 0;
int ret = 0;
bool seen_cache = false;
bool seen_discard = false;
@@ -657,13 +656,9 @@ int main(int argc, char **argv)
port = optarg;
break;
case 'o':
-dev_offset = strtoll (optarg, , 0);
-if (*end) {
-error_report("Invalid offset `%s'", optarg);
-exit(EXIT_FAILURE);
-}
-if (dev_offset < 0) {
-error_report("Offset must be positive `%s'", optarg);
+if (qemu_strtoi64(optarg, NULL, 0, _offset) < 0 ||
+dev_offset < 0) {
+error_report("Invalid offset '%s'", optarg);
exit(EXIT_FAILURE);
}
break;
@@ -685,13 +680,9 @@ int main(int argc, char **argv)
flags &= ~BDRV_O_RDWR;
break;
case 'P':
-partition = strtol(optarg, , 0);
-if (*end) {
-error_report("Invalid partition `%s'", optarg);
-exit(EXIT_FAILURE);
-}
-if (partition < 1 || partition > 8) {
-error_report("Invalid partition %d", partition);
+if (qemu_strtoi(optarg, NULL, 0, ) < 0 ||
+partition < 1 || partition > 8) {
+error_report("Invalid partition '%s'", optarg);
exit(EXIT_FAILURE);
}
break;
@@ -709,15 +700,11 @@ int main(int argc, char **argv)
device = optarg;
break;
case 'e':
-shared = strtol(optarg, , 0);
-if (*end) {
+if (qemu_strtoi(optarg, NULL, 0, ) < 0 ||
+shared < 1) {
error_report("Invalid shared device number '%s'", optarg);
exit(EXIT_FAILURE);
}
-if (shared < 1) {
-error_report("Shared device number must be greater than 0");
-exit(EXIT_FAILURE);
-}
break;
case 'f':
fmt = optarg;
@@ -1006,7 +993,7 @@ int main(int argc, char **argv)
}
fd_size -= dev_offset;
-if (partition != -1) {
+if (partition) {
ret = find_partition(blk, partition, _offset, _size);
if (ret < 0) {
error_report("Could not find partition %d: %s", partition,
--
2.17.2
[Qemu-devel] [PATCH v2 16/22] nbd/client: Split handshake into two functions
An upcoming patch will add the ability for qemu-nbd to list
the services provided by an NBD server. Share the common
code of the TLS handshake by splitting the initial exchange
into a separate function, leaving only the export handling
in the original function. Functionally, there should be no
change in behavior in this patch, although some of the code
motion may be difficult to follow due to indentation changes
(view with 'git diff -w' for a smaller changeset).
I considered an enum for the return code coordinating state
between the two functions, but in the end just settled with
ample comments.
Signed-off-by: Eric Blake
Reviewed-by: Richard W.M. Jones
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
v2: improve commit message, tweak code comment formatting
---
nbd/client.c | 144 +++
nbd/trace-events | 2 +-
2 files changed, 95 insertions(+), 51 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index 64a0e5760c3..8b0ae20fae8 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -806,21 +806,24 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
*ioc,
}
}
-int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
- const char *hostname, QIOChannel **outioc,
- NBDExportInfo *info, Error **errp)
+/*
+ * nbd_start_negotiate:
+ * Start the handshake to the server. After a positive return, the server
+ * is ready to accept additional NBD_OPT requests.
+ * Returns: negative errno: failure talking to server
+ * 0: server is oldstyle, client must still parse export size
+ * 1: server is newstyle, but can only accept EXPORT_NAME
+ * 2: server is newstyle, but lacks structured replies
+ * 3: server is newstyle and set up for structured replies
+ */
+static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
+ const char *hostname, QIOChannel **outioc,
+ bool structured_reply, bool *zeroes,
+ Error **errp)
{
uint64_t magic;
-bool zeroes = true;
-bool structured_reply = info->structured_reply;
-bool base_allocation = info->base_allocation;
-trace_nbd_receive_negotiate(tlscreds, hostname ? hostname : "");
-
-assert(info->name);
-trace_nbd_receive_negotiate_name(info->name);
-info->structured_reply = false;
-info->base_allocation = false;
+trace_nbd_start_negotiate(tlscreds, hostname ? hostname : "");
if (outioc) {
*outioc = NULL;
@@ -865,7 +868,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds
*tlscreds,
clientflags |= NBD_FLAG_C_FIXED_NEWSTYLE;
}
if (globalflags & NBD_FLAG_NO_ZEROES) {
-zeroes = false;
+*zeroes = false;
clientflags |= NBD_FLAG_C_NO_ZEROES;
}
/* client requested flags */
@@ -887,7 +890,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds
*tlscreds,
}
}
if (fixedNewStyle) {
-int result;
+int result = 0;
if (structured_reply) {
result = nbd_request_simple_option(ioc,
@@ -896,39 +899,85 @@ int nbd_receive_negotiate(QIOChannel *ioc,
QCryptoTLSCreds *tlscreds,
if (result < 0) {
return -EINVAL;
}
-info->structured_reply = result == 1;
}
+return 2 + result;
+} else {
+return 1;
+}
+} else if (magic == NBD_CLIENT_MAGIC) {
+if (tlscreds) {
+error_setg(errp, "Server does not support STARTTLS");
+return -EINVAL;
+}
+return 0;
+} else {
+error_setg(errp, "Bad server magic received: 0x%" PRIx64, magic);
+return -EINVAL;
+}
+}
-if (info->structured_reply && base_allocation) {
-result = nbd_negotiate_simple_meta_context(ioc, info, errp);
-if (result < 0) {
-return -EINVAL;
-}
-info->base_allocation = result == 1;
-}
+/*
+ * nbd_receive_negotiate:
+ * Connect to server, complete negotiation, and move into transmission phase.
+ * Returns: negative errno: failure talking to server
+ * 0: server is connected
+ */
+int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
+ const char *hostname, QIOChannel **outioc,
+ NBDExportInfo *info, Error **errp)
+{
+int result;
+bool zeroes = true;
+bool base_allocation = info->base_allocation;
+uint32_t oldflags;
+
+assert(info->name);
+trace_nbd_receive_negotiate_name(info->name);
+
+result = nbd_start_negotiate(ioc, tlscreds, hostname, outioc,
+ info->structured_reply, , errp);
+
+info->structured_reply = false;
+
[Qemu-devel] [PATCH v2 06/22] qemu-nbd: Fail earlier for -c/-d on non-linux
Connecting to a /dev/nbdN device is a Linux-specific action.
We were already masking -c and -d from 'qemu-nbd --help' on
non-linux. However, while -d fails with a sensible error
message, it took hunting through a couple of files to prove
that. What's more, the code for -c doesn't fail until after
it has created a pthread and tried to open a device - possibly
even printing an error message with %m on a non-Linux platform
in spite of the comment that %m is glibc-specific. Make the
failure happen sooner, then get rid of stubs that are no
longer needed because of the early exits.
While at it: tweak the blank newlines in --help output to be
consistent, whether or not built on Linux.
Signed-off-by: Eric Blake
---
v2: Hoist -c error message to share with -d message [Vladimir]
Bonus: gets rid of a stray TAB in nbd/client.c
---
nbd/client.c | 18 +-
qemu-nbd.c | 21 +++--
2 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index 5d59d5ba78a..3d9086af39d 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -1029,23 +1029,7 @@ int nbd_disconnect(int fd)
return 0;
}
-#else
-int nbd_init(int fd, QIOChannelSocket *ioc, NBDExportInfo *info,
-Error **errp)
-{
-error_setg(errp, "nbd_init is only supported on Linux");
-return -ENOTSUP;
-}
-
-int nbd_client(int fd)
-{
-return -ENOTSUP;
-}
-int nbd_disconnect(int fd)
-{
-return -ENOTSUP;
-}
-#endif
+#endif /* __linux__ */
int nbd_send_request(QIOChannel *ioc, NBDRequest *request)
{
diff --git a/qemu-nbd.c b/qemu-nbd.c
index e169b839ece..2807e132396 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -43,6 +43,12 @@
#include "trace/control.h"
#include "qemu-version.h"
+#ifdef __linux__
+#define HAVE_NBD_DEVICE 1
+#else
+#define HAVE_NBD_DEVICE 0
+#endif
+
#define SOCKET_PATH"/var/lock/qemu-nbd-%s"
#define QEMU_NBD_OPT_CACHE 256
#define QEMU_NBD_OPT_AIO 257
@@ -98,11 +104,11 @@ static void usage(const char *name)
"specify tracing options\n"
" --forkfork off the server process and exit the parent\n"
"once the server is running\n"
-#ifdef __linux__
+#if HAVE_NBD_DEVICE
+"\n"
"Kernel NBD client support:\n"
" -c, --connect=DEV connect FILE to the local NBD device DEV\n"
" -d, --disconnect disconnect the specified device\n"
-"\n"
#endif
"\n"
"Block device options:\n"
@@ -236,6 +242,7 @@ static void termsig_handler(int signum)
}
+#if HAVE_NBD_DEVICE
static void *show_parts(void *arg)
{
char *device = arg;
@@ -321,6 +328,7 @@ out:
kill(getpid(), SIGTERM);
return (void *) EXIT_FAILURE;
}
+#endif /* HAVE_NBD_DEVICE */
static int nbd_can_accept(void)
{
@@ -814,6 +822,12 @@ int main(int argc, char **argv)
}
}
+#if !HAVE_NBD_DEVICE
+if (disconnect || device) {
+error_report("Kernel /dev/nbdN support not available");
+exit(EXIT_FAILURE);
+}
+#else /* HAVE_NBD_DEVICE */
if (disconnect) {
int nbdfd = open(argv[optind], O_RDWR);
if (nbdfd < 0) {
@@ -829,6 +843,7 @@ int main(int argc, char **argv)
return 0;
}
+#endif
if ((device && !verbose) || fork_process) {
int stderr_fd[2];
@@ -1006,6 +1021,7 @@ int main(int argc, char **argv)
nbd_export_set_description(exp, export_description);
if (device) {
+#if HAVE_NBD_DEVICE
int ret;
ret = pthread_create(_thread, NULL, nbd_client_thread, device);
@@ -1013,6 +1029,7 @@ int main(int argc, char **argv)
error_report("Failed to create client thread: %s", strerror(ret));
exit(EXIT_FAILURE);
}
+#endif
} else {
/* Shut up GCC warnings. */
memset(_thread, 0, sizeof(client_thread));
--
2.17.2
[Qemu-devel] [PATCH v2 11/22] nbd/client: Change signature of nbd_negotiate_simple_meta_context()
Pass 'info' instead of three separate parameters related to info,
when requesting the server to set the meta context. Update the
NBDExportInfo struct to rename the received id field to match the
fact that we are currently overloading the field to match whatever
context the user supplied through the x-dirty-bitmap hack, as well
as adding a TODO comment to remind future patches about a desire
to request two contexts at once.
Signed-off-by: Eric Blake
---
v2: split patch into easier-to-review pieces [Rich, Vladimir]
rename NBDExportInfo meta_base_allocation_id [Vladimir]
---
include/block/nbd.h | 2 +-
block/nbd-client.c | 4 ++--
nbd/client.c| 53 +
3 files changed, 28 insertions(+), 31 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 65feff8ba96..ae5fe28f486 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -276,7 +276,7 @@ struct NBDExportInfo {
uint32_t opt_block;
uint32_t max_block;
-uint32_t meta_base_allocation_id;
+uint32_t context_id;
};
typedef struct NBDExportInfo NBDExportInfo;
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 417971d8b05..608b578e1d3 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -246,11 +246,11 @@ static int nbd_parse_blockstatus_payload(NBDClientSession
*client,
}
context_id = payload_advance32();
-if (client->info.meta_base_allocation_id != context_id) {
+if (client->info.context_id != context_id) {
error_setg(errp, "Protocol error: unexpected context id %d for "
"NBD_REPLY_TYPE_BLOCK_STATUS, when negotiated context
"
"id is %d", context_id,
- client->info.meta_base_allocation_id);
+ client->info.context_id);
return -EINVAL;
}
diff --git a/nbd/client.c b/nbd/client.c
index 7462fa5ae0e..bcccd5f555e 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -628,26 +628,30 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
}
/* nbd_negotiate_simple_meta_context:
- * Set one meta context. Simple means that reply must contain zero (not
- * negotiated) or one (negotiated) contexts. More contexts would be considered
- * as a protocol error. It's also implied that meta-data query equals queried
- * context name, so, if server replies with something different than @context,
- * it is considered an error too.
- * return 1 for successful negotiation, context_id is set
+ * Request the server to set the meta context for export @info->name
+ * using @info->x_dirty_bitmap with a fallback to "base:allocation",
+ * setting @info->context_id to the resulting id. Fail if the server
+ * responds with more than one context or with a context different
+ * than the query.
+ * return 1 for successful negotiation,
*0 if operation is unsupported,
*-1 with errp set for any other error
*/
static int nbd_negotiate_simple_meta_context(QIOChannel *ioc,
- const char *export,
- const char *context,
- uint32_t *context_id,
+ NBDExportInfo *info,
Error **errp)
{
+/*
+ * TODO: Removing the x_dirty_bitmap hack will mean refactoring
+ * this function to request and store ids for multiple contexts
+ * (both base:allocation and a dirty bitmap), at which point this
+ * function should lose the term _simple.
+ */
int ret;
NBDOptionReply reply;
-uint32_t received_id = 0;
+const char *context = info->x_dirty_bitmap ?: "base:allocation";
bool received = false;
-uint32_t export_len = strlen(export);
+uint32_t export_len = strlen(info->name);
uint32_t context_len = strlen(context);
uint32_t data_len = sizeof(export_len) + export_len +
sizeof(uint32_t) + /* number of queries */
@@ -655,9 +659,9 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
*ioc,
char *data = g_malloc(data_len);
char *p = data;
-trace_nbd_opt_meta_request(context, export);
+trace_nbd_opt_meta_request(context, info->name);
stl_be_p(p, export_len);
-memcpy(p += sizeof(export_len), export, export_len);
+memcpy(p += sizeof(export_len), info->name, export_len);
stl_be_p(p += export_len, 1);
stl_be_p(p += sizeof(uint32_t), context_len);
memcpy(p += sizeof(context_len), context, context_len);
@@ -683,7 +687,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
*ioc,
if (reply.type == NBD_REP_META_CONTEXT) {
char *name;
-if (reply.length != sizeof(received_id) + context_len) {
+if (reply.length != sizeof(info->context_id) + context_len) {
error_setg(errp, "Failed to negotiate meta context '%s', server "
"answered with
[Qemu-devel] [PATCH v2 03/22] maint: Allow for EXAMPLES in texi2pod
The next commit will add an EXAMPLES section to qemu-nbd.8;
for that to work, we need to recognize EXAMPLES in texi2pod,
and we need to make all man pages be regenerated since the
output of texi2pod can be different.
Signed-off-by: Eric Blake
---
v2: new patch
---
Makefile| 18 ++
scripts/texi2pod.pl | 2 +-
2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/Makefile b/Makefile
index c8b9efdad4b..0bd204eff8a 100644
--- a/Makefile
+++ b/Makefile
@@ -824,14 +824,16 @@ docs/interop/qemu-qmp-qapi.texi: qapi/qapi-doc.texi
docs/interop/qemu-ga-qapi.texi: qga/qapi-generated/qga-qapi-doc.texi
@cp -p $< $@
-qemu.1: qemu-doc.texi qemu-options.texi qemu-monitor.texi
qemu-monitor-info.texi
-qemu.1: qemu-option-trace.texi
-qemu-img.1: qemu-img.texi qemu-option-trace.texi qemu-img-cmds.texi
-fsdev/virtfs-proxy-helper.1: fsdev/virtfs-proxy-helper.texi
-qemu-nbd.8: qemu-nbd.texi qemu-option-trace.texi
-qemu-ga.8: qemu-ga.texi
-docs/qemu-block-drivers.7: docs/qemu-block-drivers.texi
-docs/qemu-cpu-models.7: docs/qemu-cpu-models.texi
+qemu.1: qemu-doc.texi qemu-options.texi qemu-monitor.texi \
+ qemu-monitor-info.texi scripts/texi2pod.pl
+qemu.1: qemu-option-trace.texi scripts/texi2pod.pl
+qemu-img.1: qemu-img.texi qemu-option-trace.texi qemu-img-cmds.texi \
+scripts/texi2pod.pl
+fsdev/virtfs-proxy-helper.1: fsdev/virtfs-proxy-helper.texi scripts/texi2pod.pl
+qemu-nbd.8: qemu-nbd.texi qemu-option-trace.texi scripts/texi2pod.pl
+qemu-ga.8: qemu-ga.texi scripts/texi2pod.pl
+docs/qemu-block-drivers.7: docs/qemu-block-drivers.texi scripts/texi2pod.pl
+docs/qemu-cpu-models.7: docs/qemu-cpu-models.texi scripts/texi2pod.pl
html: qemu-doc.html docs/interop/qemu-qmp-ref.html
docs/interop/qemu-ga-ref.html
info: qemu-doc.info docs/interop/qemu-qmp-ref.info
docs/interop/qemu-ga-ref.info
diff --git a/scripts/texi2pod.pl b/scripts/texi2pod.pl
index 39ce584a322..839b7917cf7 100755
--- a/scripts/texi2pod.pl
+++ b/scripts/texi2pod.pl
@@ -398,7 +398,7 @@ $sects{NAME} = "$fn \- $tl\n";
$sects{FOOTNOTES} .= "=back\n" if exists $sects{FOOTNOTES};
for $sect (qw(NAME SYNOPSIS DESCRIPTION OPTIONS ENVIRONMENT FILES
- BUGS NOTES FOOTNOTES SEEALSO AUTHOR COPYRIGHT)) {
+ BUGS NOTES FOOTNOTES EXAMPLES SEEALSO AUTHOR COPYRIGHT)) {
if(exists $sects{$sect}) {
$head = $sect;
$head =~ s/SEEALSO/SEE ALSO/;
--
2.17.2
[Qemu-devel] [PATCH v2 10/22] nbd/client: Move export name into NBDExportInfo
Refactor the 'name' parameter of nbd_receive_negotiate() from
being a separate parameter into being part of the in-out 'info'.
This also spills over to a simplification of nbd_opt_go().
The main driver for this refactoring is that an upcoming patch
would like to add support to qemu-nbd to list information about
all exports available on a server, where the name(s) will be
provided by the server instead of the client. But another benefit
is that we can now allow the client to explicitly specify the
empty export name "" even when connecting to an oldstyle server
(even if qemu is no longer such a server after commit 7f7dfe2a).
Signed-off-by: Eric Blake
Reviewed-by: Richard W.M. Jones
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
v2: Fix g_free usage [Vladimir]
---
include/block/nbd.h | 8
block/nbd-client.c | 5 +++--
nbd/client.c| 39 ++-
qemu-nbd.c | 6 --
nbd/trace-events| 2 +-
5 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 6a5bfe5d559..65feff8ba96 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -262,6 +262,7 @@ struct NBDExportInfo {
/* Set by client before nbd_receive_negotiate() */
bool request_sizes;
char *x_dirty_bitmap;
+char *name; /* must be non-NULL */
/* In-out fields, set by client before nbd_receive_negotiate() and
* updated by server results during nbd_receive_negotiate() */
@@ -279,10 +280,9 @@ struct NBDExportInfo {
};
typedef struct NBDExportInfo NBDExportInfo;
-int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
- QCryptoTLSCreds *tlscreds, const char *hostname,
- QIOChannel **outioc, NBDExportInfo *info,
- Error **errp);
+int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
+ const char *hostname, QIOChannel **outioc,
+ NBDExportInfo *info, Error **errp);
int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info,
Error **errp);
int nbd_send_request(QIOChannel *ioc, NBDRequest *request);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index fc5b7eda8ee..417971d8b05 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -984,10 +984,11 @@ int nbd_client_init(BlockDriverState *bs,
client->info.structured_reply = true;
client->info.base_allocation = true;
client->info.x_dirty_bitmap = g_strdup(x_dirty_bitmap);
-ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
-tlscreds, hostname,
+client->info.name = g_strdup(export ?: "");
+ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), tlscreds, hostname,
>ioc, >info, errp);
g_free(client->info.x_dirty_bitmap);
+g_free(client->info.name);
if (ret < 0) {
logout("Failed to negotiate with the NBD server\n");
return ret;
diff --git a/nbd/client.c b/nbd/client.c
index 28f5a286cba..7462fa5ae0e 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -328,15 +328,14 @@ static int nbd_receive_list(QIOChannel *ioc, char **name,
char **description,
}
-/* Returns -1 if NBD_OPT_GO proves the export @wantname cannot be
+/* Returns -1 if NBD_OPT_GO proves the export @info->name cannot be
* used, 0 if NBD_OPT_GO is unsupported (fall back to NBD_OPT_LIST and
* NBD_OPT_EXPORT_NAME in that case), and > 0 if the export is good to
- * go (with @info populated). */
-static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
- NBDExportInfo *info, Error **errp)
+ * go (with the rest of @info populated). */
+static int nbd_opt_go(QIOChannel *ioc, NBDExportInfo *info, Error **errp)
{
NBDOptionReply reply;
-uint32_t len = strlen(wantname);
+uint32_t len = strlen(info->name);
uint16_t type;
int error;
char *buf;
@@ -346,10 +345,10 @@ static int nbd_opt_go(QIOChannel *ioc, const char
*wantname,
* flags still 0 is a witness of a broken server. */
info->flags = 0;
-trace_nbd_opt_go_start(wantname);
+trace_nbd_opt_go_start(info->name);
buf = g_malloc(4 + len + 2 + 2 * info->request_sizes + 1);
stl_be_p(buf, len);
-memcpy(buf + 4, wantname, len);
+memcpy(buf + 4, info->name, len);
/* At most one request, everything else up to server */
stw_be_p(buf + 4 + len, info->request_sizes);
if (info->request_sizes) {
@@ -751,10 +750,9 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
*ioc,
return 0;
}
-int nbd_receive_negotiate(QIOChannel *ioc, const char *name,
- QCryptoTLSCreds *tlscreds, const char *hostname,
- QIOChannel **outioc, NBDExportInfo *info,
- Error **errp)
+int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
+ const char *hostname, QIOChannel
[Qemu-devel] [PATCH v2 05/22] nbd/client: More consistent error messages
Consolidate on using decimal (not hex), on outputting the
option reply name (not just value), and a consistent comma between
clauses, when the client reports protocol discrepancies from the
server. While it won't affect normal operation, it makes
debugging additions easier.
Signed-off-by: Eric Blake
Reviewed-by: Richard W.M. Jones
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
v2: consistent use of comma [Vladimir]
---
nbd/client.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index b4d457a19ad..5d59d5ba78a 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -132,8 +132,9 @@ static int nbd_receive_option_reply(QIOChannel *ioc,
uint32_t opt,
return -1;
}
if (reply->option != opt) {
-error_setg(errp, "Unexpected option type %x expected %x",
- reply->option, opt);
+error_setg(errp, "Unexpected option type %u (%s), expected %u (%s)",
+ reply->option, nbd_opt_lookup(reply->option),
+ opt, nbd_opt_lookup(opt));
nbd_send_opt_abort(ioc);
return -1;
}
@@ -265,8 +266,9 @@ static int nbd_receive_list(QIOChannel *ioc, const char
*want, bool *match,
}
return 0;
} else if (reply.type != NBD_REP_SERVER) {
-error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
- reply.type, NBD_REP_SERVER);
+error_setg(errp, "Unexpected reply type %u (%s), expected %u (%s)",
+ reply.type, nbd_rep_lookup(reply.type),
+ NBD_REP_SERVER, nbd_rep_lookup(NBD_REP_SERVER));
nbd_send_opt_abort(ioc);
return -1;
}
@@ -378,9 +380,9 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,
return 1;
}
if (reply.type != NBD_REP_INFO) {
-error_setg(errp, "unexpected reply type %" PRIu32
- " (%s), expected %u",
- reply.type, nbd_rep_lookup(reply.type), NBD_REP_INFO);
+error_setg(errp, "unexpected reply type %u (%s), expected %u (%s)",
+ reply.type, nbd_rep_lookup(reply.type),
+ NBD_REP_INFO, nbd_rep_lookup(NBD_REP_INFO));
nbd_send_opt_abort(ioc);
return -1;
}
@@ -704,8 +706,9 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
*ioc,
}
if (reply.type != NBD_REP_ACK) {
-error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
- reply.type, NBD_REP_ACK);
+error_setg(errp, "Unexpected reply type %u (%s), expected %u (%s)",
+ reply.type, nbd_rep_lookup(reply.type),
+ NBD_REP_ACK, nbd_rep_lookup(NBD_REP_ACK));
nbd_send_opt_abort(ioc);
return -1;
}
--
2.17.2
[Qemu-devel] [PATCH v2 12/22] nbd/client: Improve error handling in nbd_negotiate_simple_meta_context()
Always allocate space for the reply returned by the server and
hoist the trace earlier, as it is more interesting to trace the
server's reply (even if it is unexpected) than parroting our
request only on success. After all, skipping the allocation
for a wrong size was merely a micro-optimization that only
benefitted a broken server, rather than the common case of a
compliant server that meets our expectations.
Then turn the reply handling into a loop (even though we still
never iterate more than once), to make this code easier to use
when later patches do support multiple server replies. This
changes the error message for a server with two replies (a
corner case we are unlikely to hit in practice) from:
Unexpected reply type 4 (meta context), expected 0 (ack)
to:
Server replied with more than one context
Signed-off-by: Eric Blake
---
v2: split patch into easier-to-review pieces [Rich, Vladimir]
---
nbd/client.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index bcccd5f555e..b6a85fc3ef8 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -684,10 +684,11 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
*ioc,
return ret;
}
-if (reply.type == NBD_REP_META_CONTEXT) {
+while (reply.type == NBD_REP_META_CONTEXT) {
char *name;
-if (reply.length != sizeof(info->context_id) + context_len) {
+if (reply.length <= sizeof(info->context_id) ||
+reply.length > NBD_MAX_BUFFER_SIZE) {
error_setg(errp, "Failed to negotiate meta context '%s', server "
"answered with unexpected length %" PRIu32, context,
reply.length);
@@ -708,6 +709,15 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
*ioc,
return -1;
}
name[reply.length] = '\0';
+trace_nbd_opt_meta_reply(name, info->context_id);
+
+if (received) {
+error_setg(errp, "Server replied with more than one context");
+g_free(name);
+nbd_send_opt_abort(ioc);
+return -1;
+}
+
if (strcmp(context, name)) {
error_setg(errp, "Failed to negotiate meta context '%s', server "
"answered with different context '%s'", context,
@@ -717,8 +727,6 @@ static int nbd_negotiate_simple_meta_context(QIOChannel
*ioc,
return -1;
}
g_free(name);
-
-trace_nbd_opt_meta_reply(context, info->context_id);
received = true;
/* receive NBD_REP_ACK */
--
2.17.2
[Qemu-devel] [PATCH v2 01/22] qemu-nbd: Use program name in error messages
This changes output from: $ qemu-nbd nosuch Failed to blk_new_open 'nosuch': Could not open 'nosuch': No such file or directory to something more consistent with qemu-img and qemu: $ qemu-nbd nosuch qemu-nbd: Failed to blk_new_open 'nosuch': Could not open 'nosuch': No such file or directory Update the lone affected test to match. (Hmm - is it sad that we don't do much testing of expected failures?) Signed-off-by: Eric Blake Reviewed-by: Richard W.M. Jones Reviewed-by: Vladimir Sementsov-Ogievskiy --- qemu-nbd.c | 1 + tests/qemu-iotests/233.out | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/qemu-nbd.c b/qemu-nbd.c index ca7109652e5..e169b839ece 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -571,6 +571,7 @@ int main(int argc, char **argv) #endif module_call_init(MODULE_INIT_TRACE); +error_set_progname(argv[0]); qcrypto_init(_fatal); module_call_init(MODULE_INIT_QOM); diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out index 94acd9b9479..5f416721b03 100644 --- a/tests/qemu-iotests/233.out +++ b/tests/qemu-iotests/233.out @@ -27,7 +27,7 @@ virtual size: 64M (67108864 bytes) disk size: unavailable == check TLS with different CA fails == -option negotiation failed: Verify failed: No certificate was found. +qemu-nbd: option negotiation failed: Verify failed: No certificate was found. qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': The certificate hasn't got a known issuer == perform I/O over TLS == -- 2.17.2
[Qemu-devel] [PATCH v2 00/22] nbd: add qemu-nbd --list
I got tired of debugging whether a server was advertising the correct things during negotiation by inspecting the trace logs of qemu-io as client - not to mention that without SOME sort of client tracing particular commands, we can't easily regression test the server for correct behavior. The final straw was at KVM Forum, when Nir asked me to make sure there was a way to easily determine if an NBD server is exposing what we really want (and fixing x-dirty-bitmap to behave saner fell out as a result of answering that question). I note that upstream NBD has 'nbd-client -l $host' for querying just export names (with no quoting, so you have to know that a blank line means the default export), but it wasn't powerful enough, so I implemented 'qemu-nbd -L' to document everything. Upstream NBD has separate 'nbd-client' and 'nbd-server' binaries, while we only have 'qemu-nbd' (which is normally just a server, but 'qemu-nbd -c' also operates a second thread as a client). Our other uses of qemu as NBD client are for consuming a block device (as in qemu-io, qemu-img, or a drive to qemu) - but those binaries are less suited to something so specific to the NBD protocol. Bonus: As a result of my work on this series, nbdkit now supports NBD_OPT_INFO (my interoperability testing between server implementations has been paying off, both at fixing server bugs, and at making this code more reliable across difference in valid servers). Also available at: https://repo.or.cz/qemu/ericb.git qemu-nbd-list-v2 v1->v2: Lots of rework: several new patches added, several split into more managable pieces, a couple reordered. Use g_free consistently. Add R-b where the patch didn't change too much beyond the review. 001/22:[] [--] 'qemu-nbd: Use program name in error messages' 002/22:[down] 'nbd: Document timeline of various features' 003/22:[down] 'maint: Allow for EXAMPLES in texi2pod' 004/22:[down] 'qemu-nbd: Enhance man page' 005/22:[0006] [FC] 'nbd/client: More consistent error messages' 006/22:[0009] [FC] 'qemu-nbd: Fail earlier for -c/-d on non-linux' 007/22:[down] 'qemu-nbd: Avoid strtol open-coding' 008/22:[0012] [FC] 'nbd/client: Drop pointless buf variable' 009/22:[0114] [FC] 'nbd/client: Refactor nbd_receive_list()' 010/22:[0006] [FC] 'nbd/client: Move export name into NBDExportInfo' 011/22:[down] 'nbd/client: Change signature of nbd_negotiate_simple_meta_context()' 012/22:[down] 'nbd/client: Improve error handling in nbd_negotiate_simple_meta_context()' 013/22:[down] 'nbd/client: Split out nbd_send_one_meta_context()' 014/22:[down] 'nbd/client: Split out nbd_receive_one_meta_context()' 015/22:[] [-C] 'nbd/client: Refactor return of nbd_receive_negotiate()' 016/22:[0036] [FC] 'nbd/client: Split handshake into two functions' 017/22:[down] 'nbd/client: Pull out oldstyle size determination' 018/22:[0078] [FC] 'nbd/client: Add nbd_receive_export_list()' 019/22:[down] 'nbd/client: Add meta contexts to nbd_receive_export_list()' 020/22:[0034] [FC] 'qemu-nbd: Add --list option' 021/22:[0028] [FC] 'nbd/client: Work around 3.0 bug for listing meta contexts' 022/22:[0021] [FC] 'iotests: Enhance 223, 233 to cover 'qemu-nbd --list'' Eric Blake (22): qemu-nbd: Use program name in error messages nbd: Document timeline of various features maint: Allow for EXAMPLES in texi2pod qemu-nbd: Enhance man page nbd/client: More consistent error messages qemu-nbd: Fail earlier for -c/-d on non-linux qemu-nbd: Avoid strtol open-coding nbd/client: Drop pointless buf variable nbd/client: Refactor nbd_receive_list() nbd/client: Move export name into NBDExportInfo nbd/client: Change signature of nbd_negotiate_simple_meta_context() nbd/client: Improve error handling in nbd_negotiate_simple_meta_context() nbd/client: Split out nbd_send_one_meta_context() nbd/client: Split out nbd_receive_one_meta_context() nbd/client: Refactor return of nbd_receive_negotiate() nbd/client: Split handshake into two functions nbd/client: Pull out oldstyle size determination nbd/client: Add nbd_receive_export_list() nbd/client: Add meta contexts to nbd_receive_export_list() qemu-nbd: Add --list option nbd/client: Work around 3.0 bug for listing meta contexts iotests: Enhance 223, 233 to cover 'qemu-nbd --list' docs/interop/nbd.txt | 19 +- qemu-nbd.texi | 107 - Makefile | 18 +- include/block/nbd.h| 27 +- nbd/nbd-internal.h | 3 +- block/nbd-client.c | 9 +- nbd/client.c | 796 + qemu-nbd.c | 216 -- nbd/trace-events | 11 +- scripts/texi2pod.pl| 2 +- tests/qemu-iotests/223 | 2 + tests/qemu-iotests/223.out | 20 + tests/qemu-iotests/233 | 19 +- tests/qemu-iotests/233.out | 17 +- 14 files changed, 918 insertions(+), 348 deletions(-) -- 2.17.2
[Qemu-devel] [PATCH v2 02/22] nbd: Document timeline of various features
It can be useful to figure out which NBD protocol features are exposed by a server, as well as what features a client will take advantage of if available, for a given qemu release. It's not always precise to base features on version numbers (thanks to downstream backports), but any documentation is better than making users search through git logs themselves. This patch originally stemmed from a request to document that pristine 3.0 has a known bug where NBD_OPT_LIST_META_CONTEXT with 0 queries forgot to advertise an available "qemu:dirty-bitmap" context, but documenting bugs like this (or the fact that 3.0 also botched NBD_CMD_CACHE) gets to be too much details, especially since buggy releases will be less likely connection targets over time. Instead, I chose to just remind users to check stable release branches. Suggested-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Eric Blake --- v2: new patch --- docs/interop/nbd.txt | 19 ++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt index 77b5f459111..2b25f871e7c 100644 --- a/docs/interop/nbd.txt +++ b/docs/interop/nbd.txt @@ -15,7 +15,6 @@ Qemu supports the "base:allocation" metadata context as defined in the NBD protocol specification, and also defines an additional metadata namespace "qemu". - == "qemu" namespace == The "qemu" namespace currently contains only one type of context, @@ -36,3 +35,21 @@ in addition to "qemu:dirty-bitmap:": namespace. * "qemu:dirty-bitmap:" - returns list of all available dirty-bitmap metadata contexts. + += Features by version = + +The following list documents which qemu version first implemented +various features (both as a server exposing the feature, and as a +client taking advantage of the feature when present), to make it +easier to plan for cross-version interoperability. Note that in +several cases, the initial release containing a feature may require +additional patches from the corresponding stable branch to fix bugs in +the operation of that feature. + +* 2.6: NBD_OPT_STARTTLS with TLS X.509 Certificates +* 2.8: NBD_CMD_WRITE_ZEROES +* 2.10: NBD_OPT_GO, NBD_INFO_BLOCK +* 2.11: NBD_OPT_STRUCTURED_READ +* 2.12: NBD_CMD_BLOCK_STATUS for "base:allocation" +* 3.0: NBD_OPT_STARTTLS with TLS Pre-Shared Keys (PSK), +NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:", NBD_CMD_CACHE -- 2.17.2
[Qemu-devel] [PATCH v2 3/3] util: check the return value of fcntl in qemu_set_{block, nonblock}
Assert that the return value is not an error. This is like commit
7e6478e7d4f for qemu_set_cloexec.
Signed-off-by: Li Qiang
---
util/oslib-posix.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index c1bee2a581..4ce1ba9ca4 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -233,14 +233,18 @@ void qemu_set_block(int fd)
{
int f;
f = fcntl(fd, F_GETFL);
-fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
+assert(f != -1);
+f = fcntl(fd, F_SETFL, f & ~O_NONBLOCK);
+assert(f != -1);
}
void qemu_set_nonblock(int fd)
{
int f;
f = fcntl(fd, F_GETFL);
-fcntl(fd, F_SETFL, f | O_NONBLOCK);
+assert(f != -1);
+f = fcntl(fd, F_SETFL, f | O_NONBLOCK);
+assert(f != -1);
}
int socket_set_fast_reuse(int fd)
--
2.17.1
[Qemu-devel] [PATCH v2 2/3] vhost-user: fix ioeventfd_enabled
Currently, the vhost-user-test assumes the eventfd is available.
However it's not true because the accel is qtest. So the
'vhost_set_vring_file' will not add fds to the msg and the server
side of vhost-user-test will be broken. The bug is in 'ioeventfd_enabled'.
We should make this function return true if not using kvm accel.
Signed-off-by: Li Qiang
---
v2: change the fix in 'ioeventfd_enabled' per Paolo's review
hw/virtio/vhost-user.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index e09bed0e4a..564a31d12c 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -207,7 +207,7 @@ struct vhost_user {
static bool ioeventfd_enabled(void)
{
-return kvm_enabled() && kvm_eventfds_enabled();
+return !kvm_enabled() || kvm_eventfds_enabled();
}
static int vhost_user_read(struct vhost_dev *dev, VhostUserMsg *msg)
--
2.17.1
[Qemu-devel] [PATCH v2 0/3] vhost-user-test fix
Currently, the vhost-user-test is not correct.
When in qtest mode, the accel is qtest, not kvm.
So when the client side of vhost-user-test send
'VHOST_USER_SET_VRING_CALL' msg, the 'fd' will
no be added in 'fds' in 'vhost_set_vring_file'.
In 'chr_read' of the server side in the
vhost-user-test, it calls 'qemu_chr_fe_get_msgfds'
to get the fd in 'VHOST_USER_SET_VRING_CALL'. Though
there is no fd returned, but as the 'fd' is not initialized
so 'fd' maybe valid, and 'qemu_set_nonblock' will be success.
Even worse, 'qemu_set_nonblock' doesn't check the return value
of fcntl.
So this cause the interesting bug here: there are three issues,
but they combined and will bypass the qtest.
This patchset tries to address these issue.
v2: Change the second patch per Paolo's review
Li Qiang (3):
tests: vhost-user-test: initialize 'fd' in chr_read
vhost-user: fix ioeventfd_enabled
util: check the return value of fcntl in qemu_set_{block,nonblock}
hw/virtio/vhost-user.c | 2 +-
tests/vhost-user-test.c | 2 +-
util/oslib-posix.c | 8 ++--
3 files changed, 8 insertions(+), 4 deletions(-)
--
2.17.1
[Qemu-devel] [PATCH v2 1/3] tests: vhost-user-test: initialize 'fd' in chr_read
Currently when processing VHOST_USER_SET_VRING_CALL
if 'qemu_chr_fe_get_msgfds' get no fd, the 'fd' will
be a stack uninitialized value.
Signed-off-by: Li Qiang
---
tests/vhost-user-test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 45d58d8ea2..86039e61e0 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -309,7 +309,7 @@ static void chr_read(void *opaque, const uint8_t *buf, int
size)
CharBackend *chr = >chr;
VhostUserMsg msg;
uint8_t *p = (uint8_t *)
-int fd;
+int fd = -1;
if (s->test_fail) {
qemu_chr_fe_disconnect(chr);
--
2.17.1
Re: [Qemu-devel] [PATCH for-4.0 0/6] vhost-user-blk: Add support for backend reconnecting
On Sat, 15 Dec 2018 at 05:23, Michael S. Tsirkin wrote: > > On Fri, Dec 14, 2018 at 10:33:54AM +0800, Yongji Xie wrote: > > On Fri, 14 Dec 2018 at 10:20, Michael S. Tsirkin wrote: > > > > > > On Fri, Dec 14, 2018 at 09:56:41AM +0800, Yongji Xie wrote: > > > > On Thu, 13 Dec 2018 at 22:45, Michael S. Tsirkin > > > > wrote: > > > > > > > > > > On Thu, Dec 06, 2018 at 02:35:46PM +0800, elohi...@gmail.com wrote: > > > > > > From: Xie Yongji > > > > > > > > > > > > This patchset is aimed at supporting qemu to reconnect > > > > > > vhost-user-blk backend after vhost-user-blk backend crash or > > > > > > restart. > > > > > > > > > > > > The patch 1 tries to implenment the sync connection for > > > > > > "reconnect socket". > > > > > > > > > > > > The patch 2 introduces a new message VHOST_USER_SET_VRING_INFLIGHT > > > > > > to support offering shared memory to backend to record > > > > > > its inflight I/O. > > > > > > > > > > > > The patch 3,4 are the corresponding libvhost-user patches of > > > > > > patch 2. Make libvhost-user support VHOST_USER_SET_VRING_INFLIGHT. > > > > > > > > > > > > The patch 5 supports vhost-user-blk to reconnect backend when > > > > > > connection closed. > > > > > > > > > > > > The patch 6 tells qemu that we support reconnecting now. > > > > > > > > > > > > To use it, we could start qemu with: > > > > > > > > > > > > qemu-system-x86_64 \ > > > > > > -chardev > > > > > > socket,id=char0,path=/path/vhost.socket,reconnect=1,wait \ > > > > > > -device vhost-user-blk-pci,chardev=char0 \ > > > > > > > > > > > > and start vhost-user-blk backend with: > > > > > > > > > > > > vhost-user-blk -b /path/file -s /path/vhost.socket > > > > > > > > > > > > Then we can restart vhost-user-blk at any time during VM running. > > > > > > > > > > > > Xie Yongji (6): > > > > > > char-socket: Enable "wait" option for client mode > > > > > > vhost-user: Add shared memory to record inflight I/O > > > > > > libvhost-user: Introduce vu_queue_map_desc() > > > > > > libvhost-user: Support recording inflight I/O in shared memory > > > > > > vhost-user-blk: Add support for reconnecting backend > > > > > > contrib/vhost-user-blk: enable inflight I/O recording > > > > > > > > > > What is missing in all this is documentation. > > > > > Specifically docs/interop/vhost-user.txt. > > > > > > > > > > At a high level the design is IMO a good one. > > > > > > > > > > However I would prefer reading the protocol first before > > > > > the code. > > > > > > > > > > So here's what I managed to figure out, and it matches > > > > > how I imagined it would work when I was still > > > > > thinking about out of order for net: > > > > > > > > > > - backend allocates memory to keep its stuff around > > > > > - sends it to qemu so it can maintain it > > > > > - gets it back on reconnect > > > > > > > > > > format and size etc are all up to the backend, > > > > > a good implementation would probably implement some > > > > > kind of versioning. > > > > > > > > > > Is this what this implements? > > > > > > > > > > > > > Definitely, yes. And the comments looks good to me. Qemu get size and > > > > version from backend, then allocate memory and send it back with > > > > version. Backend knows how to use the memory according to the version. > > > > If we do that, should we allocate the memory per device rather than > > > > per virtqueue? > > > > > > > > Thanks, > > > > Yongji > > > > > > It's up to you. Maybe both. > > > > > > > OK. I think I may still keep it in virtqueue level in v2. Thank you. > > > > Thanks, > > Yongji > > I'd actually add options for both, and backend can set size 0 if it > wants to. > OK, let me try it in v2. Thanks, Yongji
Re: [Qemu-devel] [PATCH 2/3] vhost-user: add fds inf 'vhost_set_vring_file' in qtest
On 15/12/18 02:26, Li Qiang wrote:
> Currently, the vhost-user-test assumes the eventfd is available.
> However it's not true because the accel is qtest. So the
> 'vhost_set_vring_file' will not add fds to the msg and the server
> side of vhost-user-test will be broken. This patch avoid this.
>
> Signed-off-by: Li Qiang
> ---
> hw/virtio/vhost-user.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index e09bed0e4a..3b666f093c 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -23,6 +23,7 @@
> #include "migration/migration.h"
> #include "migration/postcopy-ram.h"
> #include "trace.h"
> +#include "sysemu/qtest.h"
>
> #include
> #include
> @@ -742,7 +743,7 @@ static int vhost_set_vring_file(struct vhost_dev *dev,
> .hdr.size = sizeof(msg.payload.u64),
> };
>
> -if (ioeventfd_enabled() && file->fd > 0) {
The bug is in ioeventfd_enabled. It should be !kvm_enabled() ||
kvm_eventfds_enabled().
Paolo
> +if ((qtest_enabled() || ioeventfd_enabled()) && file->fd > 0) {
> fds[fd_num++] = file->fd;
> } else {
> msg.payload.u64 |= VHOST_USER_VRING_NOFD_MASK;
>
Re: [Qemu-devel] [PATCH v7 16/19] spapr: introduce a new sPAPR IRQ backend supporting XIVE and XICS
On Wed, Dec 12, 2018 at 10:13:29AM +0100, Cédric Le Goater wrote:
> [ ... ]
>
>
> +
> +static qemu_irq spapr_qirq_dual(sPAPRMachineState *spapr, int irq)
> +{
> +return spapr_irq_current(spapr)->qirq(spapr, irq);
> >>>
> >>> Urgh... I don't think this is going to work. IIRC the various devices
> >>> (PHB, VIO, etc.) are wired up to their qirqs at realize() time, so if
> >>> you reboot from a XIVE guest to XICS guest (or maybe the other way
> >>> around) the peripherals won't be able to signal irqs in the new
> >>> scheme.
> >>
> >> It does. The IRQ numbers are claimed in both backends.
> >
> > Yes, I realize that, but the two backends still have their own set of
> > qirqs, which have their own set_irq routines associated with them.
> >
> >> This is the problem since the very beginning. For reset and migration
> >> to work, we need to keep in sync the IRQ number space of the machine
> >> and the different interrupt controllers.
> >
> > Sure, we have the numbers in sync, but that won't help if when the
> > peripherals do a qemu_irq_pulse() it goes to the wrong backend's
> > trigger routine.
>
> Ah. You mean that the devices could bypass the sPAPR IRQ layer and
> trigger the IRQ directly from the IRQ controller model ? it's not
> the case today.
Ah, right, sorry. We're saved by the fact that basically the only
root level devices for spapr are the PHB and VIO devices, and it so
happens that those look up the relevant qirqs from spapr at interrupt
time, rather than in advance.
I think it's fragile to rely on that - qirqs are supposed to be stable
objects that devices can keep a reference to and use later.
> I agree that having a common set of qirqs and a qemu_irq handler
> doing the dispatch on the selected interrupt contoller model is
> good idea. I didn't go in that direction because KVM brings additional
> head aches.
>
> Where would put the qirqs ? at the machine level I suppose.
Yes.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v7 18/19] spapr: add a 'pseries-4.0-xive' machine type
On Tue, Dec 11, 2018 at 05:44:26PM +0100, Cédric Le Goater wrote:
> On 12/11/18 11:42 AM, Cédric Le Goater wrote:
> > On 12/11/18 3:06 AM, David Gibson wrote:
> >> On Mon, Dec 10, 2018 at 11:17:33PM +0100, Cédric Le Goater wrote:
> >>> On 12/9/18 8:46 PM, Cédric Le Goater wrote:
> This pseries machine makes use of a new sPAPR IRQ backend supporting
> the XIVE interrupt mode.
>
> The guest OS is required to have support for the XIVE exploitation
> mode of the POWER9 interrupt controller.
>
> Signed-off-by: Cédric Le Goater
> ---
> hw/ppc/spapr.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4012ebd794a4..3cc134a0b673 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3985,6 +3985,21 @@ static void
> spapr_machine_4_0_class_options(MachineClass *mc)
>
> DEFINE_SPAPR_MACHINE(4_0, "4.0", true);
>
> +static void spapr_machine_4_0_xive_instance_options(MachineState
> *machine)
> +{
> +spapr_machine_4_0_instance_options(machine);
> +}
> +
> +static void spapr_machine_4_0_xive_class_options(MachineClass *mc)
> +{
> +sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> +
> +spapr_machine_4_0_class_options(mc);> +smc->irq =
> _irq_xive;
> >>>
> >>> I have been adding checks on the CPU model to export the XIVE capability
> >>> only on POWER9 processors but it breaks some of the tests.
> >>>
> >>> I was wondering if we could add a default POWER9 CPU to the -xive machine
> >>> :
> >>>
> >>> + mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
> >>>
> >>> and if we could change tests/cpu-plug-test.c with :
> >>>
> >>> @@ -198,8 +198,13 @@ static void add_pseries_test_case(const
> >>>}
> >>>data = g_new(PlugTestData, 1);
> >>>data->machine = g_strdup(mname);
> >>> -data->cpu_model = "power8_v2.0";
> >>> -data->device_model = g_strdup("power8_v2.0-spapr-cpu-core");
> >>> +if (g_str_has_suffix(mname, "xive")) {
> >>> +data->cpu_model = "power9_v2.0";
> >>> +data->device_model = g_strdup("power9_v2.0-spapr-cpu-core");
> >>> +} else {
> >>> +data->cpu_model = "power8_v2.0";
> >>> +data->device_model = g_strdup("power8_v2.0-spapr-cpu-core");
> >>> +}
> >>>data->sockets = 2;
> >>>data->cores = 3;
> >>>data->threads = 1;
> >>>
> >>> or if there is a better way ?
> >>
> >> So, I'd actually prefer a machine option, rather than wholly separate
> >> machine types to select xics/xive/dual. Machine types was fine while
> >> prototyping this, but I don't think we want to actually merge new
> >> machine types for it.
> >
> > I agree.
> >
> >> So, instead I think we want a machine option which can be set to
> >> xics/xive/dual, with xics being the default for earlier machine types
> >> and dual the default for 4.0 onwards.
> >
> > I will revive an old patch doing just that.
> >
> > The question now is how to link the sPAPRMachineState instance to
> > the selected sPAPR IRQ backend.
>
> Would something like below be acceptable ? If so I will change the
> remaining patches to use 'spapr->irq' and not 'smc->irq' anymore.
Yeah, I think that looks ok.
>
> Thanks,
>
> C.
>
> Index: qemu-xive.git/include/hw/ppc/spapr.h
> ===
> --- qemu-xive.git.orig/include/hw/ppc/spapr.h
> +++ qemu-xive.git/include/hw/ppc/spapr.h
> @@ -177,6 +177,7 @@ struct sPAPRMachineState {
> int32_t irq_map_nr;
> unsigned long *irq_map;
> sPAPRXive *xive;
> +sPAPRIrq *irq;
>
> bool cmd_line_caps[SPAPR_CAP_NUM];
> sPAPRCapabilities def, eff, mig;
> Index: qemu-xive.git/hw/ppc/spapr.c
> ===
> --- qemu-xive.git.orig/hw/ppc/spapr.c
> +++ qemu-xive.git/hw/ppc/spapr.c
> @@ -1302,7 +1301,7 @@ static void *spapr_build_fdt(sPAPRMachin
> }
>
> QLIST_FOREACH(phb, >phbs, list) {
> -ret = spapr_populate_pci_dt(phb, PHANDLE_XICP, fdt,
> smc->irq->nr_msis);
> +ret = spapr_populate_pci_dt(phb, PHANDLE_XICP, fdt,
> spapr->irq->nr_msis);
> if (ret < 0) {
> error_report("couldn't setup PCI devices in fdt");
> exit(1);
> @@ -3056,9 +3055,38 @@ static void spapr_set_vsmt(Object *obj,
> visit_type_uint32(v, name, (uint32_t *)opaque, errp);
> }
>
> +static char *spapr_get_irq(Object *obj, Error **errp)
> +{
> +sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +
> +if (spapr->irq == _irq_xics_legacy) {
> +return g_strdup("legacy");
> +} else if (spapr->irq == _irq_xics) {
> +return g_strdup("xics");
> +} else if (spapr->irq == _irq_xive) {
> +return g_strdup("xive");
> +}
> +g_assert_not_reached();
> +}
> +
>
