Re: [Qemu-devel] [PATCH v7 12/24] virtio-blk: Functions for op blocker management
On Wed, Nov 25, 2015 at 05:26:02PM +0100, Max Reitz wrote: > On 25.11.2015 17:18, Kevin Wolf wrote: > > Am 25.11.2015 um 17:03 hat Max Reitz geschrieben: > >> On 25.11.2015 16:57, Kevin Wolf wrote: > >>> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben: > >>> This makes me wonder: What do we even block here any more? If I didn't > >>> miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure > >>> why this needs to be blocked, or if we simply forgot to enable it. > >> > >> Well, even though in practice this wall of code doesn't make much sense, > >> of course it will be safe for potential additions of new op blockers. > >> > >> And of course we actually don't want these blockers at all anymore... > > > > Yes, but dataplane shouldn't really be special enough any more that we > > want to disable features for it initially. By now it sounds more like an > > easy way to forget unblocking a new feature even though it would work. > > > > So perhaps we should really just remove the blockers from dataplane. > > Then we don't have to answer the question above... > > Well, maybe. I guess this is up to Stefan. At this point blockdev.c and block jobs acquire/release AioContext, hence all these op blockers are being unblocked. I think we can switch from whitelisting (unblocking) nearly everything to blacklisting (blocking) only things that aren't supported yet. Stefan signature.asc Description: PGP signature
Re: [Qemu-devel] [SeaBIOS] bug: incorrect uuid in seabios output
On Di, 2015-11-24 at 11:55 -0500, Cole Robinson wrote:
> Hi,
>
> The UUID seabios reports in its boot output doesn't match what is passed via
> qemu -uuid option. An example is reported here:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1284259
>
> This is due to:
>
> commit caad057bb6ce86a9cb71520af395fd0bd04a659f
> Author: Eduardo Habkost
> Date: Wed Oct 29 11:26:08 2014 -0200
>
> smbios: Encode UUID according to SMBIOS specification
>
> Differently from older versions, SMBIOS version 2.6 is explicit about
> the encoding of UUID fields:
>
> > Although RFC 4122 recommends network byte order for all fields, the PC
> > industry (including the ACPI, UEFI, and Microsoft specifications) has
> > consistently used little-endian byte encoding for the first three
> fields:
> > time_low, time_mid, time_hi_and_version. The same encoding, also known
> as
> > wire format, should also be used for the SMBIOS representation of the
> UUID.
> >
> > The UUID {00112233-4455-6677-8899-AABBCCDDEEFF} would thus be
> represented
> > as 33 22 11 00 55 44 77 66 88 99 AA BB CC DD EE FF.
>
> The dmidecode tool implements this and decodes the above "wire format"
> when SMBIOS version >= 2.6. We moved from SMBIOS version 2.4 to 2.8 when
> we started building the SMBIOS entry point inside QEMU, on commit
> c97294ec1b9e36887e119589d456557d72ab37b5.
>
>
> seabios doesn't seem to handle this special UUID format when reading from
> smbios.
Hmm. Changing the ordering in display_uuid() is easy. There seems to
be no easy way to figure which format to use though. Checking the
version like dmidecode doesn't fly as there are qemu versions with old
format but version smbios 2.8 in the wild ...
cheers,
Gerd
Re: [Qemu-devel] [PATCH] Avoid memory leak
dongxingshui writes: > monitor.c: Avoid memory leak > > When send a wrong qmp command, a memory leak occurs. Fix it. > > Signed-off-by: dongxingshui Functionally the same as http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06862.html Message-Id: <1446117309-15322-1-git-send-email-arm...@redhat.com> The delay getting that merged is entirely my fault. Thanks!
Re: [Qemu-devel] [PATCH COLO-Frame v11 10/39] COLO: Implement colo checkpoint protocol
On 2015/11/25 3:00, Dr. David Alan Gilbert wrote:
* zhanghailiang (zhang.zhanghaili...@huawei.com) wrote:
We need communications protocol of user-defined to control the checkpoint
process.
The new checkpoint request is started by Primary VM, and the interactive process
like below:
Checkpoint synchronizing points,
Primary Secondary
initial work
'checkpoint-ready' <-- @
'checkpoint-request' @ ->
Suspend (Only in hybrid
mode)
'checkpoint-reply' <-- @
Suspend&Save state
'vmstate-send' @ ->
Send state Receive state
'vmstate-received' <-- @
Release packets Load state
'vmstate-load' <-- @
Resume Resume (Only in hybrid
mode)
Start Comparing (Only in hybrid mode)
NOTE:
1) '@' who sends the message
2) Every sync-point is synchronized by two sides with only
one handshake(single direction) for low-latency.
If more strict synchronization is required, a opposite direction
sync-point should be added.
3) Since sync-points are single direction, the remote side may
go forward a lot when this side just receives the sync-point.
4) For now, we only support 'periodic' checkpoint, for which
the Secondary VM is not running, later we will support 'hybrid' mode.
Signed-off-by: zhanghailiang
Signed-off-by: Li Zhijian
Signed-off-by: Gonglei
Cc: Eric Blake
---
v11:
- Add missing 'checkpoint-ready' communication in comment.
- Use parameter to return 'value' for colo_ctl_get() (Dave's suggestion)
- Fix trace for colo_ctl_get() to trace command and value both
v10:
- Rename enum COLOCmd to COLOCommand (Eric's suggestion).
- Remove unused 'ram-steal'
---
migration/colo.c | 198 ++-
qapi-schema.json | 27
trace-events | 2 +
3 files changed, 225 insertions(+), 2 deletions(-)
diff --git a/migration/colo.c b/migration/colo.c
index 0ab9618..c045d61 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -10,10 +10,12 @@
* later. See the COPYING file in the top-level directory.
*/
+#include
#include "sysemu/sysemu.h"
#include "migration/colo.h"
#include "trace.h"
#include "qemu/error-report.h"
+#include "qemu/sockets.h"
bool colo_supported(void)
{
@@ -34,9 +36,107 @@ bool migration_incoming_in_colo_state(void)
return mis && (mis->state == MIGRATION_STATUS_COLO);
}
+/* colo checkpoint control helper */
+static int colo_ctl_put(QEMUFile *f, uint32_t cmd, uint64_t value)
+{
+int ret = 0;
+
+qemu_put_be32(f, cmd);
+qemu_put_be64(f, value);
+qemu_fflush(f);
+
+ret = qemu_file_get_error(f);
+trace_colo_ctl_put(COLOCommand_lookup[cmd], value);
+
+return ret;
+}
+
+static int colo_ctl_get_cmd(QEMUFile *f, uint32_t *cmd)
+{
+int ret = 0;
+
+*cmd = qemu_get_be32(f);
+ret = qemu_file_get_error(f);
+if (ret < 0) {
+return ret;
+}
+if (*cmd >= COLO_COMMAND_MAX) {
+error_report("Invalid colo command, got cmd:%d", *cmd);
+return -EINVAL;
+}
+
+return 0;
+}
+
+static int colo_ctl_get(QEMUFile *f, uint32_t require, uint64_t *value)
+{
+int ret;
+uint32_t cmd;
+
+ret = colo_ctl_get_cmd(f, &cmd);
+if (ret < 0) {
+return ret;
+}
+if (cmd != require) {
+error_report("Unexpect colo command, expect:%d, but got cmd:%d",
+ require, cmd);
I think you need to use PRIu32 rather than %d since they are uint32_t
(I doubt it will break on anything, but it's correct).
+return -EINVAL;
+}
+
+*value = qemu_get_be64(f);
+trace_colo_ctl_get(COLOCommand_lookup[cmd], *value);
+ret = qemu_file_get_error(f);
+
+return ret;
+}
+
+static int colo_do_checkpoint_transaction(MigrationState *s)
+{
+int ret;
+uint64_t value;
+
+ret = colo_ctl_put(s->to_dst_file, COLO_COMMAND_CHECKPOINT_REQUEST, 0);
+if (ret < 0) {
+goto out;
+}
+
+ret = colo_ctl_get(s->rp_state.from_dst_file,
+ COLO_COMMAND_CHECKPOINT_REPLY, &value);
+if (ret < 0) {
+goto out;
+}
+
+/* TODO: suspend and save vm state to colo buffer */
+
+ret = colo_ctl_put(s->to_dst_file, COLO_COMMAND_VMSTATE_SEND, 0);
+if (ret < 0) {
+goto out;
+}
+
+/* TODO: send vmstate to Secondary */
+
+ret = colo_ctl_get(s->rp_state.from_dst_file,
+ COLO_COMMAND_VMSTATE_RECEIVED, &value);
+if (ret < 0) {
+goto out;
+}
+
+ret = colo_ctl_get(s->rp_state.from_d
Re: [Qemu-devel] [PATCH] error: Document how to accumulate multiple errors
Fam Zheng writes:
> On Tue, 11/17 17:05, Markus Armbruster wrote:
>> Suggested-by: Eric Blake
>> Signed-off-by: Markus Armbruster
>> Reviewed-by: Eric Blake
>> ---
>> include/qapi/error.h | 17 +
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 4d42cdc..b2362a5 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -76,6 +76,23 @@
>> * But when all you do with the error is pass it on, please use
>> * foo(arg, errp);
>> * for readability.
>> + *
>> + * Receive and accumulate multiple errors (first one wins):
>> + * Error *err = NULL, *local_err = NULL;
>> + * foo(arg, &err);
>> + * bar(arg, &local_err);
>> + * error_propagate(&err, local_err);
>> + * if (err) {
>> + * handle the error...
>> + * }
>> + *
>> + * Do *not* "optimize" this to
>> + * foo(arg, &err);
>> + * bar(arg, &err); // WRONG!
>> + * if (err) {
>> + * handle the error...
>> + * }
>> + * because this may pass a non-null err to bar().
>> */
>>
>> #ifndef ERROR_H
>> --
>> 2.4.3
>>
>>
>
> Curious if there is a recommended way to report both error messages to caller,
> rather than overwriting the first with the second?
The documentation above is about keeping the first and throwing away the
second, i.e. first one wins. I haven't seen in "last one wins" the
code, yet. How to do it is thus left as exercise to the reader.
You can only return one *error*. You can still combine multiple
*messages* into one error. There is no recommended way, at least not
yet.
You could use the hint mechanism to append additional messages:
foo(arg, &err);
bar(arg, &err2);
if (err2) {
error_append_hint(err, "additionally, %s", error_get_pretty(err2));
error_free(err2);
}
Just an example, the wording of the hint is probably not a good idea.
You may want to modify the first message, too. The obvious technique
for receiving, modifying and passing on an error:
frob(arg, &err)
error_setg(errp, "Could not frobnicate: %s",
error_get_pretty(local_err));
[Qemu-devel] [PATCH v4 3/3] i.MX: Add an i.MX25 specific CCM class/instance.
Signed-off-by: Jean-Christophe Dubois
---
Changes since v1:
* rework loging to match other i.MX drivers
Changes since v2:
* We moved to an inheritance QOM scheme
Changes since v3:
* Rework logging based on comments.
hw/arm/fsl-imx25.c | 2 +-
hw/misc/Makefile.objs | 1 +
hw/misc/imx25_ccm.c | 276
include/hw/arm/fsl-imx25.h | 4 +-
include/hw/misc/imx25_ccm.h | 59 ++
5 files changed, 339 insertions(+), 3 deletions(-)
create mode 100644 hw/misc/imx25_ccm.c
create mode 100644 include/hw/misc/imx25_ccm.h
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 9f302ed..36818ee 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -38,7 +38,7 @@ static void fsl_imx25_init(Object *obj)
object_initialize(&s->avic, sizeof(s->avic), TYPE_IMX_AVIC);
qdev_set_parent_bus(DEVICE(&s->avic), sysbus_get_default());
-object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX31_CCM);
+object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX25_CCM);
qdev_set_parent_bus(DEVICE(&s->ccm), sysbus_get_default());
for (i = 0; i < FSL_IMX25_NUM_UARTS; i++) {
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index c77f3e3..8a235df 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -27,6 +27,7 @@ obj-$(CONFIG_ECCMEMCTL) += eccmemctl.o
obj-$(CONFIG_EXYNOS4) += exynos4210_pmu.o
obj-$(CONFIG_IMX) += imx_ccm.o
obj-$(CONFIG_IMX) += imx31_ccm.o
+obj-$(CONFIG_IMX) += imx25_ccm.o
obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
obj-$(CONFIG_MAINSTONE) += mst_fpga.o
diff --git a/hw/misc/imx25_ccm.c b/hw/misc/imx25_ccm.c
new file mode 100644
index 000..d0fe017
--- /dev/null
+++ b/hw/misc/imx25_ccm.c
@@ -0,0 +1,276 @@
+/*
+ * IMX25 Clock Control Module
+ *
+ * Copyright (C) 2012 NICTA
+ * Updated by Jean-Christophe Dubois
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * To get the timer frequencies right, we need to emulate at least part of
+ * the CCM.
+ */
+
+#include "hw/misc/imx25_ccm.h"
+
+#ifndef DEBUG_IMX25_CCM
+#define DEBUG_IMX25_CCM 0
+#endif
+
+#define DPRINTF(fmt, args...) \
+do { \
+if (DEBUG_IMX25_CCM) { \
+fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX25_CCM, \
+ __func__, ##args); \
+} \
+} while (0)
+
+#define CKIH_FREQ 2400 /* 24MHz crystal input */
+
+static const VMStateDescription vmstate_imx25_ccm = {
+.name = TYPE_IMX25_CCM,
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(mpctl, IMX25CCMState),
+VMSTATE_UINT32(upctl, IMX25CCMState),
+VMSTATE_UINT32(cctl, IMX25CCMState),
+VMSTATE_UINT32_ARRAY(cgcr, IMX25CCMState, 3),
+VMSTATE_UINT32_ARRAY(pcdr, IMX25CCMState, 4),
+VMSTATE_UINT32(rcsr, IMX25CCMState),
+VMSTATE_UINT32(crdr, IMX25CCMState),
+VMSTATE_UINT32_ARRAY(dcvr, IMX25CCMState, 4),
+VMSTATE_UINT32_ARRAY(ltr, IMX25CCMState, 4),
+VMSTATE_UINT32_ARRAY(ltbr, IMX25CCMState, 2),
+VMSTATE_UINT32_ARRAY(pmcr, IMX25CCMState, 3),
+VMSTATE_UINT32(mcr, IMX25CCMState),
+VMSTATE_UINT32_ARRAY(lpimr, IMX25CCMState, 2),
+VMSTATE_END_OF_LIST()
+},
+};
+
+static uint32_t imx25_ccm_get_mpll_clk(IMXCCMState *dev)
+{
+IMX25CCMState *s = IMX25_CCM(dev);
+
+DPRINTF("()\n");
+
+if (EXTRACT(s->cctl, MPLL_BYPASS)) {
+return CKIH_FREQ;
+} else {
+return imx_ccm_calc_pll(s->mpctl, CKIH_FREQ);
+}
+}
+
+static uint32_t imx25_ccm_get_upll_clk(IMXCCMState *dev)
+{
+IMX25CCMState *s = IMX25_CCM(dev);
+
+DPRINTF("()\n");
+
+return imx_ccm_calc_pll(s->upctl, CKIH_FREQ);
+}
+
+static uint32_t imx25_ccm_get_mcu_clk(IMXCCMState *dev)
+{
+IMX25CCMState *s = IMX25_CCM(dev);
+
+DPRINTF("()\n");
+
+if (EXTRACT(s->cctl, ARM_SRC)) {
+return (imx25_ccm_get_mpll_clk(dev) * 3 / 4) /
+ (1 + EXTRACT(s->cctl, ARM_CLK_DIV));
+} else {
+return imx25_ccm_get_mpll_clk(dev) /
+ (1 + EXTRACT(s->cctl, ARM_CLK_DIV));
+}
+}
+
+static uint32_t imx25_ccm_get_ahb_clk(IMXCCMState *dev)
+{
+IMX25CCMState *s = IMX25_CCM(dev);
+
+DPRINTF("()\n");
+
+return imx25_ccm_get_mcu_clk(dev) / (1 + EXTRACT(s->cctl, AHB_CLK_DIV));
+}
+
+static uint32_t imx25_ccm_get_ipg_clk(IMXCCMState *dev)
+{
+DPRINTF("()\n");
+
+return imx25_ccm_get_ahb_clk(dev) / 2;
+}
+
+static uint32_t imx25_ccm_get_clock_frequency(IMXCCMState *dev, IMXClk clock)
+{
+uint32_t freq = 0;
+DPRINTF("Clock = %d)\n", clock);
+
+switch (clock) {
+case NOCLK:
+break;
+case CLK_MPLL:
+freq = imx25_ccm_get_mpll_clk(dev);
+break;
+case CLK_UPLL:
+freq = imx25_ccm_get_upll_clk(dev);
+break;
+case CLK_MC
[Qemu-devel] [PATCH v4 2/3] i.MX: Split the CCM class into an abstact base class and a concrete class.
The IMX_CCM class is now the base abstract class that is used by EPIT and GPT
timer implementation.
IMX31_CCM class is the concrete class implementing CCM for i.MX31 SOC.
For now the i.MX25 continues to use the i.MX31 CCM implementation.
An i.MX25 specific CCM will be introduced in a later patch.
We also rework initialization to stop using deprecated sysbus device init
Signed-off-by: Jean-Christophe Dubois
Reviewed-by: Peter Crosthwaite
---
Changes since v1:
* None
Changes since v2:
* We moved to an inheritance QOM scheme
Changes since v3:
* Rework logging based on comment on i.MX25 CCM patch.
* Change abstract class function parameter to abstract class type instead
of DEVICE type.
* EPIT and GPT timers use abstract class instead of DEVICE class.
hw/arm/fsl-imx25.c | 6 +-
hw/arm/fsl-imx31.c | 6 +-
hw/misc/Makefile.objs | 1 +
hw/misc/imx31_ccm.c | 306
hw/misc/imx_ccm.c | 226 +++-
include/hw/arm/fsl-imx25.h | 4 +-
include/hw/arm/fsl-imx31.h | 4 +-
include/hw/misc/imx31_ccm.h | 64 +
include/hw/misc/imx_ccm.h | 69 --
include/hw/timer/imx_epit.h | 5 +-
include/hw/timer/imx_gpt.h | 5 +-
11 files changed, 428 insertions(+), 268 deletions(-)
create mode 100644 hw/misc/imx31_ccm.c
create mode 100644 include/hw/misc/imx31_ccm.h
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index e1cadac..9f302ed 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -38,7 +38,7 @@ static void fsl_imx25_init(Object *obj)
object_initialize(&s->avic, sizeof(s->avic), TYPE_IMX_AVIC);
qdev_set_parent_bus(DEVICE(&s->avic), sysbus_get_default());
-object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX_CCM);
+object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX31_CCM);
qdev_set_parent_bus(DEVICE(&s->ccm), sysbus_get_default());
for (i = 0; i < FSL_IMX25_NUM_UARTS; i++) {
@@ -150,7 +150,7 @@ static void fsl_imx25_realize(DeviceState *dev, Error
**errp)
{ FSL_IMX25_GPT4_ADDR, FSL_IMX25_GPT4_IRQ }
};
-s->gpt[i].ccm = DEVICE(&s->ccm);
+s->gpt[i].ccm = IMX_CCM(&s->ccm);
object_property_set_bool(OBJECT(&s->gpt[i]), true, "realized", &err);
if (err) {
@@ -173,7 +173,7 @@ static void fsl_imx25_realize(DeviceState *dev, Error
**errp)
{ FSL_IMX25_EPIT2_ADDR, FSL_IMX25_EPIT2_IRQ }
};
-s->epit[i].ccm = DEVICE(&s->ccm);
+s->epit[i].ccm = IMX_CCM(&s->ccm);
object_property_set_bool(OBJECT(&s->epit[i]), true, "realized", &err);
if (err) {
diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
index 53d4473..abdea06 100644
--- a/hw/arm/fsl-imx31.c
+++ b/hw/arm/fsl-imx31.c
@@ -35,7 +35,7 @@ static void fsl_imx31_init(Object *obj)
object_initialize(&s->avic, sizeof(s->avic), TYPE_IMX_AVIC);
qdev_set_parent_bus(DEVICE(&s->avic), sysbus_get_default());
-object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX_CCM);
+object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX31_CCM);
qdev_set_parent_bus(DEVICE(&s->ccm), sysbus_get_default());
for (i = 0; i < FSL_IMX31_NUM_UARTS; i++) {
@@ -128,7 +128,7 @@ static void fsl_imx31_realize(DeviceState *dev, Error
**errp)
serial_table[i].irq));
}
-s->gpt.ccm = DEVICE(&s->ccm);
+s->gpt.ccm = IMX_CCM(&s->ccm);
object_property_set_bool(OBJECT(&s->gpt), true, "realized", &err);
if (err) {
@@ -150,7 +150,7 @@ static void fsl_imx31_realize(DeviceState *dev, Error
**errp)
{ FSL_IMX31_EPIT2_ADDR, FSL_IMX31_EPIT2_IRQ },
};
-s->epit[i].ccm = DEVICE(&s->ccm);
+s->epit[i].ccm = IMX_CCM(&s->ccm);
object_property_set_bool(OBJECT(&s->epit[i]), true, "realized", &err);
if (err) {
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index aeb6b7d..c77f3e3 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -26,6 +26,7 @@ obj-$(CONFIG_NSERIES) += cbus.o
obj-$(CONFIG_ECCMEMCTL) += eccmemctl.o
obj-$(CONFIG_EXYNOS4) += exynos4210_pmu.o
obj-$(CONFIG_IMX) += imx_ccm.o
+obj-$(CONFIG_IMX) += imx31_ccm.o
obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
obj-$(CONFIG_MAINSTONE) += mst_fpga.o
diff --git a/hw/misc/imx31_ccm.c b/hw/misc/imx31_ccm.c
new file mode 100644
index 000..81245d4
--- /dev/null
+++ b/hw/misc/imx31_ccm.c
@@ -0,0 +1,306 @@
+/*
+ * IMX31 Clock Control Module
+ *
+ * Copyright (C) 2012 NICTA
+ * Updated by Jean-Christophe Dubois
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * To get the timer frequencies right, we need to emulate at least part of
+ * the i.MX31 CCM.
+ */
+
+#include "hw/misc/imx31_ccm.h"
+
+#define CKIH_FREQ 2600 /* 26MHz crystal input */
+
+#ifndef DEBUG
[Qemu-devel] [PATCH v4 1/3] i.MX: rename i.MX CCM get_clock() function and CLK ID enum names
This is to prepare for CCM code refactoring.
This is just a bit of function and enum values renaming.
We also remove some useless intermediate variables.
Signed-off-by: Jean-Christophe Dubois
Reviewed-by: Peter Crosthwaite
---
Changes since v1:
* Not present
Changes since v2:
* Not present
Changes since v3:
* None
hw/misc/imx_ccm.c | 8
hw/timer/imx_epit.c | 20 +---
hw/timer/imx_gpt.c| 16
include/hw/misc/imx_ccm.h | 8
4 files changed, 25 insertions(+), 27 deletions(-)
diff --git a/hw/misc/imx_ccm.c b/hw/misc/imx_ccm.c
index 4cc2bbc..415937f 100644
--- a/hw/misc/imx_ccm.c
+++ b/hw/misc/imx_ccm.c
@@ -49,18 +49,18 @@ static const VMStateDescription vmstate_imx_ccm = {
.post_load = imx_ccm_post_load,
};
-uint32_t imx_clock_frequency(DeviceState *dev, IMXClk clock)
+uint32_t imx_ccm_get_clock_frequency(DeviceState *dev, IMXClk clock)
{
IMXCCMState *s = IMX_CCM(dev);
switch (clock) {
case NOCLK:
return 0;
-case MCU:
+case CLK_MCU:
return s->mcu_clk_freq;
-case HSP:
+case CLK_HSP:
return s->hsp_clk_freq;
-case IPG:
+case CLK_IPG:
return s->ipg_clk_freq;
case CLK_32k:
return CKIL_FREQ;
diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index 967be4a..50bf83c 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -51,9 +51,9 @@ static char const *imx_epit_reg_name(uint32_t reg)
* These are typical.
*/
static const IMXClk imx_epit_clocks[] = {
-0,/* 00 disabled */
-IPG, /* 01 ipg_clk, ~532MHz */
-IPG, /* 10 ipg_clk_highfreq */
+NOCLK,/* 00 disabled */
+CLK_IPG, /* 01 ipg_clk, ~532MHz */
+CLK_IPG, /* 10 ipg_clk_highfreq */
CLK_32k, /* 11 ipg_clk_32k -- ~32kHz */
};
@@ -73,20 +73,18 @@ static void imx_epit_set_freq(IMXEPITState *s)
{
uint32_t clksrc;
uint32_t prescaler;
-uint32_t freq;
clksrc = extract32(s->cr, CR_CLKSRC_SHIFT, 2);
prescaler = 1 + extract32(s->cr, CR_PRESCALE_SHIFT, 12);
-freq = imx_clock_frequency(s->ccm, imx_epit_clocks[clksrc]) / prescaler;
+s->freq = imx_ccm_get_clock_frequency(s->ccm,
+imx_epit_clocks[clksrc]) / prescaler;
-s->freq = freq;
+DPRINTF("Setting ptimer frequency to %u\n", s->freq);
-DPRINTF("Setting ptimer frequency to %u\n", freq);
-
-if (freq) {
-ptimer_set_freq(s->timer_reload, freq);
-ptimer_set_freq(s->timer_cmp, freq);
+if (s->freq) {
+ptimer_set_freq(s->timer_reload, s->freq);
+ptimer_set_freq(s->timer_cmp, s->freq);
}
}
diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c
index 7257f42..b1893b8 100644
--- a/hw/timer/imx_gpt.c
+++ b/hw/timer/imx_gpt.c
@@ -81,8 +81,8 @@ static const VMStateDescription vmstate_imx_timer_gpt = {
static const IMXClk imx_gpt_clocks[] = {
NOCLK,/* 000 No clock source */
-IPG, /* 001 ipg_clk, 532MHz*/
-IPG, /* 010 ipg_clk_highfreq */
+CLK_IPG, /* 001 ipg_clk, 532MHz*/
+CLK_IPG, /* 010 ipg_clk_highfreq */
NOCLK,/* 011 not defined */
CLK_32k, /* 100 ipg_clk_32k */
NOCLK,/* 101 not defined */
@@ -93,14 +93,14 @@ static const IMXClk imx_gpt_clocks[] = {
static void imx_gpt_set_freq(IMXGPTState *s)
{
uint32_t clksrc = extract32(s->cr, GPT_CR_CLKSRC_SHIFT, 3);
-uint32_t freq = imx_clock_frequency(s->ccm, imx_gpt_clocks[clksrc])
-/ (1 + s->pr);
-s->freq = freq;
-DPRINTF("Setting clksrc %d to frequency %d\n", clksrc, freq);
+s->freq = imx_ccm_get_clock_frequency(s->ccm,
+imx_gpt_clocks[clksrc]) / (1 + s->pr);
-if (freq) {
-ptimer_set_freq(s->timer, freq);
+DPRINTF("Setting clksrc %d to frequency %d\n", clksrc, s->freq);
+
+if (s->freq) {
+ptimer_set_freq(s->timer, s->freq);
}
}
diff --git a/include/hw/misc/imx_ccm.h b/include/hw/misc/imx_ccm.h
index 0f2e469..09f6248 100644
--- a/include/hw/misc/imx_ccm.h
+++ b/include/hw/misc/imx_ccm.h
@@ -80,12 +80,12 @@ typedef struct IMXCCMState {
typedef enum {
NOCLK,
-MCU,
-HSP,
-IPG,
+CLK_MCU,
+CLK_HSP,
+CLK_IPG,
CLK_32k
} IMXClk;
-uint32_t imx_clock_frequency(DeviceState *s, IMXClk clock);
+uint32_t imx_ccm_get_clock_frequency(DeviceState *s, IMXClk clock);
#endif /* IMX_CCM_H */
--
2.5.0
[Qemu-devel] [PATCH v4 0/3] Add an i.MX25 specific CCM driver
i.MX25 SOC has a different CCM device than i.MX31. Qemu i.MX25 emulation was built with i.MX31 CCM driver. This allows Linux to work on top of the i.MX25 emultion but this is not correct. Furthermore, other SOC we could emulate like i.MX6 have yet a different implementation of the CCM device. So we split the i.MX31 into a generic base class and a specific i.MX31 class. We then add an i.MX25 specific CCM Device and have the i.MX25 SOC use it. Jean-Christophe Dubois (3): i.MX: rename i.MX CCM get_clock() function and CLK ID enum names i.MX: Split the CCM class into an abstact base class and a concrete class. i.MX: Add an i.MX25 specific CCM class/instance. hw/arm/fsl-imx25.c | 6 +- hw/arm/fsl-imx31.c | 6 +- hw/misc/Makefile.objs | 2 + hw/misc/imx25_ccm.c | 276 +++ hw/misc/imx31_ccm.c | 306 hw/misc/imx_ccm.c | 226 +++- hw/timer/imx_epit.c | 20 ++- hw/timer/imx_gpt.c | 16 +-- include/hw/arm/fsl-imx25.h | 4 +- include/hw/arm/fsl-imx31.h | 4 +- include/hw/misc/imx25_ccm.h | 59 + include/hw/misc/imx31_ccm.h | 64 + include/hw/misc/imx_ccm.h | 75 --- include/hw/timer/imx_epit.h | 5 +- include/hw/timer/imx_gpt.h | 5 +- 15 files changed, 784 insertions(+), 290 deletions(-) create mode 100644 hw/misc/imx25_ccm.c create mode 100644 hw/misc/imx31_ccm.c create mode 100644 include/hw/misc/imx25_ccm.h create mode 100644 include/hw/misc/imx31_ccm.h -- 2.5.0
Re: [Qemu-devel] [PATCH v3 3/3] i.MX: Add an i.MX25 specific CCM class/instance.
Le 25/11/2015 06:51, Peter Crosthwaite a écrit :
On Thu, Nov 19, 2015 at 12:40 PM, Jean-Christophe Dubois
wrote:
Signed-off-by: Jean-Christophe Dubois
---
Changes since v1:
* rework loging to match other i.MX drivers
Changes since v2:
* We moved to an inheritance QOM scheme
hw/arm/fsl-imx25.c | 2 +-
hw/misc/Makefile.objs | 1 +
hw/misc/imx25_ccm.c | 243
include/hw/arm/fsl-imx25.h | 4 +-
include/hw/misc/imx25_ccm.h | 59 +++
5 files changed, 306 insertions(+), 3 deletions(-)
create mode 100644 hw/misc/imx25_ccm.c
create mode 100644 include/hw/misc/imx25_ccm.h
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 5526c22..0aacc91 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -38,7 +38,7 @@ static void fsl_imx25_init(Object *obj)
object_initialize(&s->avic, sizeof(s->avic), TYPE_IMX_AVIC);
qdev_set_parent_bus(DEVICE(&s->avic), sysbus_get_default());
-object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX31_CCM);
+object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX25_CCM);
qdev_set_parent_bus(DEVICE(&s->ccm), sysbus_get_default());
for (i = 0; i < FSL_IMX25_NUM_UARTS; i++) {
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index c77f3e3..8a235df 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -27,6 +27,7 @@ obj-$(CONFIG_ECCMEMCTL) += eccmemctl.o
obj-$(CONFIG_EXYNOS4) += exynos4210_pmu.o
obj-$(CONFIG_IMX) += imx_ccm.o
obj-$(CONFIG_IMX) += imx31_ccm.o
+obj-$(CONFIG_IMX) += imx25_ccm.o
obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
obj-$(CONFIG_MAINSTONE) += mst_fpga.o
diff --git a/hw/misc/imx25_ccm.c b/hw/misc/imx25_ccm.c
new file mode 100644
index 000..a6c9bff
--- /dev/null
+++ b/hw/misc/imx25_ccm.c
@@ -0,0 +1,243 @@
+/*
+ * IMX25 Clock Control Module
+ *
+ * Copyright (C) 2012 NICTA
+ * Updated by Jean-Christophe Dubois
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * To get the timer frequencies right, we need to emulate at least part of
+ * the CCM.
+ */
+
+#include "hw/misc/imx25_ccm.h"
+
+#ifndef DEBUG_IMX25_CCM
+#define DEBUG_IMX25_CCM 0
+#endif
+
+#define DPRINTF(fmt, args...) \
+do { \
+if (DEBUG_IMX25_CCM) { \
+fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX25_CCM, \
+ __func__, ##args); \
+} \
+} while (0)
+
+#define CKIH_FREQ 2400 /* 24MHz crystal input */
+
+static const VMStateDescription vmstate_imx25_ccm = {
+.name = TYPE_IMX25_CCM,
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(mpctl, IMX25CCMState),
+VMSTATE_UINT32(upctl, IMX25CCMState),
+VMSTATE_UINT32(cctl, IMX25CCMState),
+VMSTATE_UINT32_ARRAY(cgcr, IMX25CCMState, 3),
+VMSTATE_UINT32_ARRAY(pcdr, IMX25CCMState, 4),
+VMSTATE_UINT32(rcsr, IMX25CCMState),
+VMSTATE_UINT32(crdr, IMX25CCMState),
+VMSTATE_UINT32_ARRAY(dcvr, IMX25CCMState, 4),
+VMSTATE_UINT32_ARRAY(ltr, IMX25CCMState, 4),
+VMSTATE_UINT32_ARRAY(ltbr, IMX25CCMState, 2),
+VMSTATE_UINT32_ARRAY(pmcr, IMX25CCMState, 3),
+VMSTATE_UINT32(mcr, IMX25CCMState),
+VMSTATE_UINT32_ARRAY(lpimr, IMX25CCMState, 2),
+VMSTATE_END_OF_LIST()
+},
+};
+
+static uint32_t imx25_ccm_get_mpll_clk(DeviceState *dev)
+{
+IMX25CCMState *s = IMX25_CCM(dev);
+
+DPRINTF("()\n");
+
+if (EXTRACT(s->cctl, MPLL_BYPASS)) {
+return CKIH_FREQ;
+} else {
+return imx_ccm_calc_pll(s->mpctl, CKIH_FREQ);
+}
+}
+
+static uint32_t imx25_ccm_get_upll_clk(DeviceState *dev)
+{
+IMX25CCMState *s = IMX25_CCM(dev);
+
+DPRINTF("()\n");
+
+return imx_ccm_calc_pll(s->upctl, CKIH_FREQ);
+}
+
+static uint32_t imx25_ccm_get_mcu_clk(DeviceState *dev)
+{
+IMX25CCMState *s = IMX25_CCM(dev);
+
+DPRINTF("()\n");
+
+if (EXTRACT(s->cctl, ARM_SRC)) {
+return (imx25_ccm_get_mpll_clk(dev) * 3 / 4) /
+ (1 + EXTRACT(s->cctl, ARM_CLK_DIV));
+} else {
+return imx25_ccm_get_mpll_clk(dev) /
+ (1 + EXTRACT(s->cctl, ARM_CLK_DIV));
+}
+}
+
+static uint32_t imx25_ccm_get_ahb_clk(DeviceState *dev)
+{
+IMX25CCMState *s = IMX25_CCM(dev);
+
+DPRINTF("()\n");
+
+return imx25_ccm_get_mcu_clk(dev) / (1 + EXTRACT(s->cctl, AHB_CLK_DIV));
+}
+
+static uint32_t imx25_ccm_get_ipg_clk(DeviceState *dev)
+{
+DPRINTF("()\n");
+
+return imx25_ccm_get_ahb_clk(dev) / 2;
+}
+
+static uint32_t imx25_ccm_get_clock_frequency(DeviceState *dev, IMXClk clock)
+{
+DPRINTF("Clock = %d)\n", clock);
This would be more useful if it was at the end and grabbed the return
value too (similar to what's commonly done for an MMIO read function).
OK
+
+switch (clock) {
+
Re: [Qemu-devel] [PATCH COLO-Frame v11 10/39] COLO: Implement colo checkpoint protocol
On 2015/11/25 22:01, Eric Blake wrote:
On 11/24/2015 12:00 PM, Dr. David Alan Gilbert wrote:
* zhanghailiang (zhang.zhanghaili...@huawei.com) wrote:
We need communications protocol of user-defined to control the checkpoint
process.
+static int colo_ctl_get(QEMUFile *f, uint32_t require, uint64_t *value)
+{
+int ret;
+uint32_t cmd;
+
+ret = colo_ctl_get_cmd(f, &cmd);
+if (ret < 0) {
+return ret;
+}
+if (cmd != require) {
+error_report("Unexpect colo command, expect:%d, but got cmd:%d",
+ require, cmd);
I think you need to use PRIu32 rather than %d since they are uint32_t
(I doubt it will break on anything, but it's correct).
32-bit cygwin uses 'long' for uint32_t, so there ARE platforms where it
is absolutely necessary to use PRIu32 for printing (even if qemu isn't
ported to cygwin).
Got it, i will fix it.
+++ b/qapi-schema.json
@@ -722,6 +722,33 @@
{ 'command': 'migrate-start-postcopy' }
##
+# @COLOCommand
+#
+# The commands for COLO fault tolerance
+#
+# @invalid: unknown command
+#
+# @checkpoint-ready: SVM is ready for checkpointing
+#
+# @checkpoint-request: PVM tells SVM to prepare for new checkpointing
+#
+# @checkpoint-reply: SVM gets PVM's checkpoint request
+#
+# @vmstate-send: VM's state will be sent by PVM.
+#
+# @vmstate-size: The total size of VMstate.
+#
+# @vmstate-received: VM's state has been received by SVM
+#
+# @vmstate-loaded: VM's state has been loaded by SVM
Inconsistent use of trailing '.'
Also, do we really need 'invalid'?
No, we can remove it. Thanks.
+#
+# Since: 2.6
+##
+{ 'enum': 'COLOCommand',
+ 'data': [ 'invalid', 'checkpoint-ready', 'checkpoint-request',
+'checkpoint-reply', 'vmstate-send', 'vmstate-size',
+'vmstate-received', 'vmstate-loaded' ] }
+
Re: [Qemu-devel] [PATCH v3 0/4] Bitmap clean-up patches for 2.6
On Wed, 11/25 09:57, Vladimir Sementsov-Ogievskiy wrote:
> Hmm, stop. Very bad thing (sorry, that I didn't realize it before):
>
> This breaks my dirty bitmap migration series with its meta bitmaps.
> Meta bitmap is an additional HBitmap in BdrvDirtyBitmap, which
> tracks dirtiness of this BdrvDirtyBitmap. And it (meta bitmap) have
> its own granularity of course. Or we will have to maintain
> additional meta_gran_shift and lots of duplicated code..
>
> If we speak about splitting granularity out of HBitmap, then the
> most true way should be
>
> struct HBitmapGranuled {
> struct HBitmap *hb;
> uint32_t gran_shift;
> }
>
> with all supporting functions and iterators.
Then it's not worth making this change, let's drop patch 3 & 4 for now.
Fam
Re: [Qemu-devel] [PATCH v2 01/14] block: Add "file" output parameter to block status query functions
On Wed, 11/25 22:03, Eric Blake wrote: > On 11/25/2015 12:39 AM, Fam Zheng wrote: > > The added parameter can be used to return the BDS pointer which the > > valid offset is referring to. It's value should be ignored unless > > s/It's/Its/ (remember, "It's" is valid only if "It is" also works in > the same place) Right, it's just easy to typo. Thanks :) Fam > > > BDRV_BLOCK_OFFSET_VALID in ret is set. > > > > Until block drivers fill in the right value, let's clear it explicitly > > right before calling .bdrv_get_block_status. > > > > Signed-off-by: Fam Zheng > > --- > > With that fix, > Reviewed-by: Eric Blake > > -- > Eric Blake eblake redhat com+1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
Re: [Qemu-devel] [PATCH v1 0/7] KVM: Hyper-V SynIC timers
2015-11-25 23:20 GMT+08:00 Andrey Smetanin : > Per Hyper-V specification (and as required by Hyper-V-aware guests), > SynIC provides 4 per-vCPU timers. Each timer is programmed via a pair > of MSRs, and signals expiration by delivering a special format message > to the configured SynIC message slot and triggering the corresponding > synthetic interrupt. Could you post a link for this specification? Regards, Wanpeng Li
Re: [Qemu-devel] [PATCH v3 12/15] qemu-img: In "map", use the returned "file" from bdrv_get_block_status
On 11/25/2015 10:05 PM, Fam Zheng wrote: > Now all drivers should return a correct "file", we can make use of it, > even with the recursion into backing chain above. > > Signed-off-by: Fam Zheng > --- > qemu-img.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Eric Blake > > diff --git a/qemu-img.c b/qemu-img.c > index 7954242..a7fa794 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -,7 +,7 @@ static int get_block_status(BlockDriverState *bs, > int64_t sector_num, > e->flags = ret & ~BDRV_BLOCK_OFFSET_MASK; > e->offset = ret & BDRV_BLOCK_OFFSET_MASK; > e->depth = depth; > -e->bs = bs; > +e->bs = file; > return 0; > } > > -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 13/15] qemu-img: Make MapEntry a QAPI struct
On 11/25/2015 10:05 PM, Fam Zheng wrote: > The "flags" bit mask is expanded to two booleans, "data" and "zero"; > "bs" is replaced with "filename" string. > > Refactor the merge conditions in img_map() into entry_mergable(). s/mergable/mergeable/ here and in the patch body > > Signed-off-by: Fam Zheng > --- > qapi/block-core.json | 27 > qemu-img.c | 71 > +++- > 2 files changed, 69 insertions(+), 29 deletions(-) > With the spelling fix, Reviewed-by: Eric Blake -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH v3 15/15] iotests: Add "qemu-img map" test for VMDK extents
Reviewed-by: Eric Blake Signed-off-by: Fam Zheng --- tests/qemu-iotests/059 | 10 ++ tests/qemu-iotests/059.out | 38 ++ 2 files changed, 48 insertions(+) diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059 index 0ded0c3..261d8b0 100755 --- a/tests/qemu-iotests/059 +++ b/tests/qemu-iotests/059 @@ -133,6 +133,16 @@ $QEMU_IO -c "write -P 0xa 900G 512" "$TEST_IMG" | _filter_qemu_io $QEMU_IO -c "read -v 900G 1024" "$TEST_IMG" | _filter_qemu_io echo +echo "=== Testing qemu-img map on extents ===" +for fmt in twoGbMaxExtentSparse twoGbMaxExtentFlat; do +IMGOPTS="subformat=$fmt" _make_test_img 31G +$QEMU_IO -c "write 65024 1k" "$TEST_IMG" | _filter_qemu_io +$QEMU_IO -c "write 2147483136 1k" "$TEST_IMG" | _filter_qemu_io +$QEMU_IO -c "write 5G 1k" "$TEST_IMG" | _filter_qemu_io +$QEMU_IMG map "$TEST_IMG" | _filter_testdir +done + +echo echo "=== Testing afl image with a very large capacity ===" _use_sample_img afl9.vmdk.bz2 _img_info diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out index 00057fe..54eb530 100644 --- a/tests/qemu-iotests/059.out +++ b/tests/qemu-iotests/059.out @@ -2337,6 +2337,44 @@ e103f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 read 1024/1024 bytes at offset 966367641600 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +=== Testing qemu-img map on extents === +Formatting 'TEST_DIR/iotest-version3.IMGFMT', fmt=IMGFMT size=33285996544 subformat=twoGbMaxExtentSparse +wrote 1024/1024 bytes at offset 65024 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 1024/1024 bytes at offset 2147483136 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 1024/1024 bytes at offset 5368709120 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Offset Length Mapped to File +0 0x2 0x5 TEST_DIR/iotest-version3-s001.vmdk +0x7fff 0x1 0x7 TEST_DIR/iotest-version3-s001.vmdk +0x8000 0x1 0x5 TEST_DIR/iotest-version3-s002.vmdk +0x14000 0x1 0x5 TEST_DIR/iotest-version3-s003.vmdk +Formatting 'TEST_DIR/iotest-version3.IMGFMT', fmt=IMGFMT size=33285996544 subformat=twoGbMaxExtentFlat +wrote 1024/1024 bytes at offset 65024 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 1024/1024 bytes at offset 2147483136 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 1024/1024 bytes at offset 5368709120 +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Offset Length Mapped to File +0 0x8000 0 TEST_DIR/iotest-version3-f001.vmdk +0x8000 0x8000 0 TEST_DIR/iotest-version3-f002.vmdk +0x1 0x8000 0 TEST_DIR/iotest-version3-f003.vmdk +0x18000 0x8000 0 TEST_DIR/iotest-version3-f004.vmdk +0x2 0x8000 0 TEST_DIR/iotest-version3-f005.vmdk +0x28000 0x8000 0 TEST_DIR/iotest-version3-f006.vmdk +0x3 0x8000 0 TEST_DIR/iotest-version3-f007.vmdk +0x38000 0x8000 0 TEST_DIR/iotest-version3-f008.vmdk +0x4 0x8000 0 TEST_DIR/iotest-version3-f009.vmdk +0x48000 0x8000 0 TEST_DIR/iotest-version3-f010.vmdk +0x5 0x8000 0 TEST_DIR/iotest-version3-f011.vmdk +0x58000 0x8000 0 TEST_DIR/iotest-version3-f012.vmdk +0x6 0x8000 0 TEST_DIR/iotest-version3-f013.vmdk +0x68000 0x8000 0 TEST_DIR/iotest-version3-f014.vmdk +0x7 0x8000 0 TEST_DIR/iotest-version3-f015.vmdk +0x78000 0x4000 0 TEST_DIR/iotest-version3-f016.vmdk + === Testing afl image with a very large capacity === qemu-img: Can't get size of device 'image': File too large *** done -- 2.4.3
[Qemu-devel] [PATCH v3 14/15] qemu-img: Use QAPI visitor to generate JSON
A visible improvement is that "filename" is now included in the output
if it's valid.
Reviewed-by: Eric Blake
Signed-off-by: Fam Zheng
---
qemu-img.c | 34 ++--
tests/qemu-iotests/122.out | 96 ++
2 files changed, 77 insertions(+), 53 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 1d19c86..fccddc0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -25,6 +25,8 @@
#include "qapi/qmp-output-visitor.h"
#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qint.h"
+#include "qapi/qmp/qbool.h"
#include "qemu-common.h"
#include "qemu/option.h"
#include "qemu/error-report.h"
@@ -2136,10 +2138,14 @@ static int img_info(int argc, char **argv)
static void dump_map_entry(OutputFormat output_format, MapEntry *e,
MapEntry *next)
{
+QString *str;
+QObject *obj;
+QmpOutputVisitor *ov;
+
switch (output_format) {
case OFORMAT_HUMAN:
if (e->data && !e->has_offset) {
-error_report("File contains external, encrypted or compressed
clusters.");
+error_report("File contains encrypted or compressed clusters.");
exit(1);
}
if (e->data && !e->zero) {
@@ -2157,20 +2163,26 @@ static void dump_map_entry(OutputFormat output_format,
MapEntry *e,
}
break;
case OFORMAT_JSON:
-printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
- " \"depth\": %"PRId64", \"zero\": %s, \"data\": %s",
- (e->start == 0 ? "[" : ",\n"),
- e->start, e->length, e->depth,
- e->zero ? "true" : "false",
- e->data ? "true" : "false");
-if (e->has_offset) {
-printf(", \"offset\": %"PRId64"", e->offset);
-}
-putchar('}');
+ov = qmp_output_visitor_new();
+visit_type_MapEntry(qmp_output_get_visitor(ov),
+&e, NULL, &error_abort);
+obj = qmp_output_get_qobject(ov);
+str = qobject_to_json(obj);
+assert(str != NULL);
+if (e->start == 0) {
+printf("[");
+} else {
+printf(",");
+}
+printf("%s\n", qstring_get_str(str));
if (!next) {
printf("]\n");
}
+
+qobject_decref(obj);
+qmp_output_visitor_cleanup(ov);
+QDECREF(str);
break;
}
}
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 0068e96..760759e 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -49,12 +49,13 @@ read 65536/65536 bytes at offset 4194304
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
read 65536/65536 bytes at offset 8388608
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 65536, "length": 4128768, "depth": 0, "zero": true, "data": false},
-{ "start": 4194304, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 4259840, "length": 4128768, "depth": 0, "zero": true, "data":
false},
-{ "start": 8388608, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 8454144, "length": 4128768, "depth": 0, "zero": true, "data":
false}]
+[{"length": 65536, "start": 0, "zero": false, "depth": 0, "data": true}
+,{"length": 4128768, "start": 65536, "zero": true, "depth": 0, "data": false}
+,{"length": 65536, "start": 4194304, "zero": false, "depth": 0, "data": true}
+,{"length": 4128768, "start": 4259840, "zero": true, "depth": 0, "data": false}
+,{"length": 65536, "start": 8388608, "zero": false, "depth": 0, "data": true}
+,{"length": 4128768, "start": 8454144, "zero": true, "depth": 0, "data": false}
+]
read 65536/65536 bytes at offset 0
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
read 65536/65536 bytes at offset 4194304
@@ -76,12 +77,13 @@ wrote 1024/1024 bytes at offset 1046528
1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
wrote 1024/1024 bytes at offset 0
1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 65536, "length": 65536, "depth": 0, "zero": true, "data": false},
-{ "start": 131072, "length": 196608, "depth": 0, "zero": false, "data": true},
-{ "start": 327680, "length": 655360, "depth": 0, "zero": true, "data": false},
-{ "start": 983040, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 1048576, "length": 1046528, "depth": 0, "zero": true, "data":
false}]
+[{"length": 65536, "start": 0, "zero": false, "depth": 0, "data": true}
+,{"length": 65536, "start": 65536, "zero": true, "depth": 0, "data": false}
+,{"length": 196608, "start": 131072, "zero": false, "depth": 0, "data": true}
+,{"length": 655360, "start": 327680, "zero": true, "depth": 0, "data": false}
+,{"length": 65536, "start": 983040, "zero": false, "depth": 0, "data": true}
+,{
[Qemu-devel] [PATCH v3 13/15] qemu-img: Make MapEntry a QAPI struct
The "flags" bit mask is expanded to two booleans, "data" and "zero";
"bs" is replaced with "filename" string.
Refactor the merge conditions in img_map() into entry_mergable().
Signed-off-by: Fam Zheng
---
qapi/block-core.json | 27
qemu-img.c | 71 +++-
2 files changed, 69 insertions(+), 29 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a07b13f..ee2294f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -186,6 +186,33 @@
'*fragmented-clusters': 'int', '*compressed-clusters': 'int' } }
##
+# @MapEntry:
+#
+# Mapping information from a virtual block range to a host file range
+#
+# @start: the start byte of the mapped virtual range
+#
+# @length: the number of bytes of the mapped virtual range
+#
+# @data: whether the mapped range has data
+#
+# @zero: whether the virtual blocks are zeroed
+#
+# @depth: the depth of the mapping
+#
+# @offset: #optional the offset in file that the virtual sectors are mapped to
+#
+# @filename: #optional filename that is referred to by @offset
+#
+# Since: 2.6
+#
+##
+{ 'struct': 'MapEntry',
+ 'data': {'start': 'int', 'length': 'int', 'data': 'bool',
+ 'zero': 'bool', 'depth': 'int', '*offset': 'int',
+ '*filename': 'str' } }
+
+##
# @BlockdevCacheInfo
#
# Cache mode information for a block device
diff --git a/qemu-img.c b/qemu-img.c
index a7fa794..1d19c86 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2133,47 +2133,37 @@ static int img_info(int argc, char **argv)
return 0;
}
-
-typedef struct MapEntry {
-int flags;
-int depth;
-int64_t start;
-int64_t length;
-int64_t offset;
-BlockDriverState *bs;
-} MapEntry;
-
static void dump_map_entry(OutputFormat output_format, MapEntry *e,
MapEntry *next)
{
switch (output_format) {
case OFORMAT_HUMAN:
-if ((e->flags & BDRV_BLOCK_DATA) &&
-!(e->flags & BDRV_BLOCK_OFFSET_VALID)) {
+if (e->data && !e->has_offset) {
error_report("File contains external, encrypted or compressed
clusters.");
exit(1);
}
-if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == BDRV_BLOCK_DATA)
{
+if (e->data && !e->zero) {
printf("%#-16"PRIx64"%#-16"PRIx64"%#-16"PRIx64"%s\n",
- e->start, e->length, e->offset, e->bs->filename);
+ e->start, e->length,
+ e->has_offset ? e->offset : 0,
+ e->has_filename ? e->filename : "");
}
/* This format ignores the distinction between 0, ZERO and ZERO|DATA.
* Modify the flags here to allow more coalescing.
*/
-if (next &&
-(next->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) !=
BDRV_BLOCK_DATA) {
-next->flags &= ~BDRV_BLOCK_DATA;
-next->flags |= BDRV_BLOCK_ZERO;
+if (next && (!next->data || next->zero)) {
+next->data = false;
+next->zero = true;
}
break;
case OFORMAT_JSON:
-printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\":
%d,"
- " \"zero\": %s, \"data\": %s",
+printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
+ " \"depth\": %"PRId64", \"zero\": %s, \"data\": %s",
(e->start == 0 ? "[" : ",\n"),
e->start, e->length, e->depth,
- (e->flags & BDRV_BLOCK_ZERO) ? "true" : "false",
- (e->flags & BDRV_BLOCK_DATA) ? "true" : "false");
-if (e->flags & BDRV_BLOCK_OFFSET_VALID) {
+ e->zero ? "true" : "false",
+ e->data ? "true" : "false");
+if (e->has_offset) {
printf(", \"offset\": %"PRId64"", e->offset);
}
putchar('}');
@@ -2219,13 +2209,39 @@ static int get_block_status(BlockDriverState *bs,
int64_t sector_num,
e->start = sector_num * BDRV_SECTOR_SIZE;
e->length = nb_sectors * BDRV_SECTOR_SIZE;
-e->flags = ret & ~BDRV_BLOCK_OFFSET_MASK;
+e->data = !!(ret & BDRV_BLOCK_DATA);
+e->zero = !!(ret & BDRV_BLOCK_ZERO);
e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
+e->has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);
e->depth = depth;
-e->bs = file;
+if (file && e->has_offset) {
+e->has_filename = true;
+e->filename = file->filename;
+}
return 0;
}
+static inline bool entry_mergable(const MapEntry *curr, const MapEntry *next)
+{
+if (curr->length == 0) {
+return false;
+}
+if (curr->zero != next->zero ||
+curr->data != next->data ||
+curr->depth != next->depth ||
+curr->has_filename != next->has_filename ||
+curr->has_offset != next->has_offset) {
+return false;
+}
+if (curr->has_filename && strcmp(curr->filename, next->filename)) {
+return false;
+}
+if (curr->has_of
[Qemu-devel] [PATCH v3 10/15] vpc: Assign bs->file->bs to file in vpc_co_get_block_status
Signed-off-by: Fam Zheng
---
block/vpc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/vpc.c b/block/vpc.c
index 912f5d0..412ff41 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -588,6 +588,7 @@ static int64_t coroutine_fn
vpc_co_get_block_status(BlockDriverState *bs,
if (be32_to_cpu(footer->type) == VHD_FIXED) {
*pnum = nb_sectors;
+*file = bs->file->bs;
return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
(sector_num << BDRV_SECTOR_BITS);
}
@@ -609,6 +610,7 @@ static int64_t coroutine_fn
vpc_co_get_block_status(BlockDriverState *bs,
/* *pnum can't be greater than one block for allocated
* sectors since there is always a bitmap in between. */
if (allocated) {
+*file = bs->file->bs;
return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
}
if (nb_sectors == 0) {
--
2.4.3
[Qemu-devel] [PATCH v3 09/15] vdi: Assign bs->file->bs to file in vdi_co_get_block_status
Signed-off-by: Fam Zheng --- block/vdi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/vdi.c b/block/vdi.c index 2199fd3..6b1a57b 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -550,6 +550,7 @@ static int64_t coroutine_fn vdi_co_get_block_status(BlockDriverState *bs, offset = s->header.offset_data + (uint64_t)bmap_entry * s->block_size + sector_in_block * SECTOR_SIZE; +*file = bs->file->bs; return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset; } -- 2.4.3
[Qemu-devel] [PATCH v3 08/15] sheepdog: Assign bs to file in sd_co_get_block_status
Signed-off-by: Fam Zheng
---
block/sheepdog.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 0f6789e..d5e7ff8 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2740,6 +2740,9 @@ sd_co_get_block_status(BlockDriverState *bs, int64_t
sector_num, int nb_sectors,
if (*pnum > nb_sectors) {
*pnum = nb_sectors;
}
+if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
+*file = bs;
+}
return ret;
}
--
2.4.3
[Qemu-devel] [PATCH v3 07/15] qed: Assign bs->file->bs to file in bdrv_qed_co_get_block_status
Reviewed-by: Eric Blake
Signed-off-by: Fam Zheng
---
block/qed.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/block/qed.c b/block/qed.c
index a6bbd8b..03af9c1 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -692,6 +692,7 @@ typedef struct {
uint64_t pos;
int64_t status;
int *pnum;
+BlockDriverState **file;
} QEDIsAllocatedCB;
static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t
len)
@@ -703,6 +704,7 @@ static void qed_is_allocated_cb(void *opaque, int ret,
uint64_t offset, size_t l
case QED_CLUSTER_FOUND:
offset |= qed_offset_into_cluster(s, cb->pos);
cb->status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
+*cb->file = cb->bs->file->bs;
break;
case QED_CLUSTER_ZERO:
cb->status = BDRV_BLOCK_ZERO;
@@ -734,6 +736,7 @@ static int64_t coroutine_fn
bdrv_qed_co_get_block_status(BlockDriverState *bs,
.pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE,
.status = BDRV_BLOCK_OFFSET_MASK,
.pnum = pnum,
+.file = file,
};
QEDRequest request = { .l2_table = NULL };
--
2.4.3
[Qemu-devel] [PATCH v3 06/15] parallels: Assign bs->file->bs to file in parallels_co_get_block_status
Reviewed-by: Eric Blake Signed-off-by: Fam Zheng --- block/parallels.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/parallels.c b/block/parallels.c index d1146f1..6552f32 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -273,6 +273,7 @@ static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs, return 0; } +*file = bs->file->bs; return (offset << BDRV_SECTOR_BITS) | BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; } -- 2.4.3
[Qemu-devel] [PATCH v3 11/15] vmdk: Return extent's file in bdrv_get_block_status
Signed-off-by: Fam Zheng
---
block/vmdk.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index f5a56fd..b60a5af 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1265,6 +1265,7 @@ static int64_t coroutine_fn
vmdk_co_get_block_status(BlockDriverState *bs,
0, 0);
qemu_co_mutex_unlock(&s->lock);
+index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
switch (ret) {
case VMDK_ERROR:
ret = -EIO;
@@ -1276,15 +1277,13 @@ static int64_t coroutine_fn
vmdk_co_get_block_status(BlockDriverState *bs,
ret = BDRV_BLOCK_ZERO;
break;
case VMDK_OK:
-ret = BDRV_BLOCK_DATA;
-if (extent->file == bs->file && !extent->compressed) {
-ret |= BDRV_BLOCK_OFFSET_VALID | offset;
-}
-
+ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+ret |= (offset + (index_in_cluster << BDRV_SECTOR_BITS))
+& BDRV_BLOCK_OFFSET_MASK;
+*file = extent->file->bs;
break;
}
-index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
n = extent->cluster_sectors - index_in_cluster;
if (n > nb_sectors) {
n = nb_sectors;
--
2.4.3
[Qemu-devel] [PATCH v3 03/15] qcow2: Assign bs->file->bs to file in qcow2_co_get_block_status
Signed-off-by: Fam Zheng
---
block/qcow2.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/qcow2.c b/block/qcow2.c
index 836888c..7634c42 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1302,6 +1302,7 @@ static int64_t coroutine_fn
qcow2_co_get_block_status(BlockDriverState *bs,
!s->cipher) {
index_in_cluster = sector_num & (s->cluster_sectors - 1);
cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS);
+*file = bs->file->bs;
status |= BDRV_BLOCK_OFFSET_VALID | cluster_offset;
}
if (ret == QCOW2_CLUSTER_ZERO) {
--
2.4.3
[Qemu-devel] [PATCH v3 12/15] qemu-img: In "map", use the returned "file" from bdrv_get_block_status
Now all drivers should return a correct "file", we can make use of it, even with the recursion into backing chain above. Signed-off-by: Fam Zheng --- qemu-img.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qemu-img.c b/qemu-img.c index 7954242..a7fa794 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -,7 +,7 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num, e->flags = ret & ~BDRV_BLOCK_OFFSET_MASK; e->offset = ret & BDRV_BLOCK_OFFSET_MASK; e->depth = depth; -e->bs = bs; +e->bs = file; return 0; } -- 2.4.3
[Qemu-devel] [PATCH v3 05/15] iscsi: Assign bs to file in iscsi_co_get_block_status
Reviewed-by: Eric Blake
Signed-off-by: Fam Zheng
---
block/iscsi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/block/iscsi.c b/block/iscsi.c
index 2d1e230..8c7f1b3 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -625,6 +625,9 @@ out:
if (iTask.task != NULL) {
scsi_free_scsi_task(iTask.task);
}
+if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
+*file = bs;
+}
return ret;
}
--
2.4.3
[Qemu-devel] [PATCH v3 04/15] raw: Assign bs to file in raw_co_get_block_status
Reviewed-by: Eric Blake
Signed-off-by: Fam Zheng
---
block/raw-posix.c | 1 +
block/raw_bsd.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 2cd7d68..9988aa4 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1873,6 +1873,7 @@ static int64_t coroutine_fn
raw_co_get_block_status(BlockDriverState *bs,
*pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE);
ret = BDRV_BLOCK_ZERO;
}
+*file = bs;
return ret | BDRV_BLOCK_OFFSET_VALID | start;
}
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 7d23c08..2d4c896 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -118,6 +118,7 @@ static int64_t coroutine_fn
raw_co_get_block_status(BlockDriverState *bs,
BlockDriverState **file)
{
*pnum = nb_sectors;
+*file = bs;
return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
(sector_num << BDRV_SECTOR_BITS);
}
--
2.4.3
[Qemu-devel] [PATCH v3 02/15] qcow: Assign bs->file->bs to file in qcow_co_get_block_status
Reviewed-by: Eric Blake Signed-off-by: Fam Zheng --- block/qcow.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/qcow.c b/block/qcow.c index 558f443..b59383f 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -509,6 +509,7 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs, return BDRV_BLOCK_DATA; } cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS); +*file = bs->file->bs; return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | cluster_offset; } -- 2.4.3
[Qemu-devel] [PATCH v3 00/15] qemu-img map: Allow driver to return file of the allocated block
v3: Add Eric's rev-by in patches 6, 7, 13, 14.
12: New, split out from the previous 13.
12->13: Refactor "entry_mergable" from imp_map().
Don't mess the merge conditions. [Paolo]
Address Eric's comments:
- Check has_foo before using foo.
- Remove blank line between comments and definition in schema.
- Use PRId64 instead of %ld.
- Merge short lines.
v2: Add Eric's rev-by in patches 2, 4, 5.
01: Refering -> referring in commit message. [Eric]
Recurse to "file" for sensible "zero" flag. [Paolo]
12: New. Make MapEntry a QAPI struct. [Paolo, Markus]
I stumbled upon this when looking at external bitmap formats.
Current "qemu-img map" command only displays filename if the data is allocated
in bs (bs->file) itself, or in the backing chain. Otherwise, it displays an
unfriendly error message:
$ qemu-img create -f vmdk -o subformat=monolithicFlat /tmp/test.vmdk 1G
$ qemu-img map /tmp/test.vmdk
Offset Length Mapped to File
qemu-img: File contains external, encrypted or compressed clusters.
This can be improved. This series extends the .bdrv_co_get_block_status
callback, to let block driver return the BDS of file; then updates all driver
to implement it; and lastly, it changes qemu-img to use this information in
"map" command:
$ qemu-img map /tmp/test.vmdk
Offset Length Mapped to File
0 0x4000 0 /tmp/test-flat.vmdk
$ qemu-img map --output json /tmp/test.vmdk
[{"length": 1073741824, "start": 0, "zero": false, "offset": 0, "depth": 0,
"file": "/tmp/test-flat.vmdk", "data": true}
]
Fam Zheng (15):
block: Add "file" output parameter to block status query functions
qcow: Assign bs->file->bs to file in qcow_co_get_block_status
qcow2: Assign bs->file->bs to file in qcow2_co_get_block_status
raw: Assign bs to file in raw_co_get_block_status
iscsi: Assign bs to file in iscsi_co_get_block_status
parallels: Assign bs->file->bs to file in
parallels_co_get_block_status
qed: Assign bs->file->bs to file in bdrv_qed_co_get_block_status
sheepdog: Assign bs to file in sd_co_get_block_status
vdi: Assign bs->file->bs to file in vdi_co_get_block_status
vpc: Assign bs->file->bs to file in vpc_co_get_block_status
vmdk: Return extent's file in bdrv_get_block_status
qemu-img: In "map", use the returned "file" from bdrv_get_block_status
qemu-img: Make MapEntry a QAPI struct
qemu-img: Use QAPI visitor to generate JSON
iotests: Add "qemu-img map" test for VMDK extents
block/io.c | 42 ---
block/iscsi.c | 9 +++-
block/mirror.c | 3 +-
block/parallels.c | 3 +-
block/qcow.c | 3 +-
block/qcow2.c | 3 +-
block/qed.c| 6 ++-
block/raw-posix.c | 4 +-
block/raw_bsd.c| 4 +-
block/sheepdog.c | 5 ++-
block/vdi.c| 3 +-
block/vmdk.c | 13 +++---
block/vpc.c| 4 +-
block/vvfat.c | 2 +-
include/block/block.h | 6 ++-
include/block/block_int.h | 3 +-
qapi/block-core.json | 27
qemu-img.c | 102 +
tests/qemu-iotests/059 | 10 +
tests/qemu-iotests/059.out | 38 +
tests/qemu-iotests/122.out | 96 +++---
21 files changed, 270 insertions(+), 116 deletions(-)
--
2.4.3
[Qemu-devel] [PATCH v3 01/15] block: Add "file" output parameter to block status query functions
The added parameter can be used to return the BDS pointer which the
valid offset is referring to. It's value should be ignored unless
BDRV_BLOCK_OFFSET_VALID in ret is set.
Until block drivers fill in the right value, let's clear it explicitly
right before calling .bdrv_get_block_status.
Signed-off-by: Fam Zheng
---
block/io.c| 42 --
block/iscsi.c | 6 --
block/mirror.c| 3 ++-
block/parallels.c | 2 +-
block/qcow.c | 2 +-
block/qcow2.c | 2 +-
block/qed.c | 3 ++-
block/raw-posix.c | 3 ++-
block/raw_bsd.c | 3 ++-
block/sheepdog.c | 2 +-
block/vdi.c | 2 +-
block/vmdk.c | 2 +-
block/vpc.c | 2 +-
block/vvfat.c | 2 +-
include/block/block.h | 6 --
include/block/block_int.h | 3 ++-
qemu-img.c| 7 +--
17 files changed, 59 insertions(+), 33 deletions(-)
diff --git a/block/io.c b/block/io.c
index adc1eab..71930ed 100644
--- a/block/io.c
+++ b/block/io.c
@@ -656,6 +656,7 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t
sector_num,
int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
{
int64_t target_sectors, ret, nb_sectors, sector_num = 0;
+BlockDriverState *file;
int n;
target_sectors = bdrv_nb_sectors(bs);
@@ -668,7 +669,7 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags
flags)
if (nb_sectors <= 0) {
return 0;
}
-ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
+ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file);
if (ret < 0) {
error_report("error getting block status at sector %" PRId64 ":
%s",
sector_num, strerror(-ret));
@@ -1455,6 +1456,7 @@ int bdrv_flush_all(void)
typedef struct BdrvCoGetBlockStatusData {
BlockDriverState *bs;
BlockDriverState *base;
+BlockDriverState **file;
int64_t sector_num;
int nb_sectors;
int *pnum;
@@ -1479,7 +1481,8 @@ typedef struct BdrvCoGetBlockStatusData {
*/
static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
int64_t sector_num,
- int nb_sectors, int *pnum)
+ int nb_sectors, int *pnum,
+ BlockDriverState **file)
{
int64_t total_sectors;
int64_t n;
@@ -1509,16 +1512,19 @@ static int64_t coroutine_fn
bdrv_co_get_block_status(BlockDriverState *bs,
return ret;
}
-ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
+*file = NULL;
+ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
+file);
if (ret < 0) {
*pnum = 0;
+*file = NULL;
return ret;
}
if (ret & BDRV_BLOCK_RAW) {
assert(ret & BDRV_BLOCK_OFFSET_VALID);
return bdrv_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS,
- *pnum, pnum);
+ *pnum, pnum, file);
}
if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
@@ -1535,13 +1541,14 @@ static int64_t coroutine_fn
bdrv_co_get_block_status(BlockDriverState *bs,
}
}
-if (bs->file &&
+if (*file && *file != bs &&
(ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
(ret & BDRV_BLOCK_OFFSET_VALID)) {
+BlockDriverState *file2;
int file_pnum;
-ret2 = bdrv_co_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS,
-*pnum, &file_pnum);
+ret2 = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
+*pnum, &file_pnum, &file2);
if (ret2 >= 0) {
/* Ignore errors. This is just providing extra information, it
* is useful but not necessary.
@@ -1566,14 +1573,15 @@ static int64_t coroutine_fn
bdrv_co_get_block_status_above(BlockDriverState *bs,
BlockDriverState *base,
int64_t sector_num,
int nb_sectors,
-int *pnum)
+int *pnum,
+BlockDriverState **file)
{
BlockDriverState *p;
int64_t ret = 0;
assert(bs != base);
for (p = bs; p != base; p = backing_bs(p)) {
-ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum);
+ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file);
if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) {
break;
}
@@ -1592,7 +1600,8 @@ static void coroutine_fn
bdrv_get_block_status_above_co_entry(void *opaque)
data->ret = bdrv_co_get_block_status_above(data->bs, data->base,
Re: [Qemu-devel] [PATCH v2 10/14] vpc: Assign bs->file->bs to file in vpc_co_get_block_status
On 11/25/2015 12:39 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng
> ---
> block/vpc.c | 2 ++
> 1 file changed, 2 insertions(+)
Reviewed-by: Eric Blake
>
> diff --git a/block/vpc.c b/block/vpc.c
> index 912f5d0..412ff41 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -588,6 +588,7 @@ static int64_t coroutine_fn
> vpc_co_get_block_status(BlockDriverState *bs,
>
> if (be32_to_cpu(footer->type) == VHD_FIXED) {
> *pnum = nb_sectors;
> +*file = bs->file->bs;
> return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
> (sector_num << BDRV_SECTOR_BITS);
> }
> @@ -609,6 +610,7 @@ static int64_t coroutine_fn
> vpc_co_get_block_status(BlockDriverState *bs,
> /* *pnum can't be greater than one block for allocated
> * sectors since there is always a bitmap in between. */
> if (allocated) {
> +*file = bs->file->bs;
> return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
> }
> if (nb_sectors == 0) {
>
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 09/14] vdi: Assign bs->file->bs to file in vdi_co_get_block_status
On 11/25/2015 12:39 AM, Fam Zheng wrote: > Signed-off-by: Fam Zheng > --- > block/vdi.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Eric Blake > > diff --git a/block/vdi.c b/block/vdi.c > index 2199fd3..6b1a57b 100644 > --- a/block/vdi.c > +++ b/block/vdi.c > @@ -550,6 +550,7 @@ static int64_t coroutine_fn > vdi_co_get_block_status(BlockDriverState *bs, > offset = s->header.offset_data + >(uint64_t)bmap_entry * s->block_size + >sector_in_block * SECTOR_SIZE; > +*file = bs->file->bs; > return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset; > } > > -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 08/14] sheepdog: Assign bs to file in sd_co_get_block_status
On 11/25/2015 12:39 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng
> ---
> block/sheepdog.c | 3 +++
> 1 file changed, 3 insertions(+)
Reviewed-by: Eric Blake
>
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 0f6789e..d5e7ff8 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -2740,6 +2740,9 @@ sd_co_get_block_status(BlockDriverState *bs, int64_t
> sector_num, int nb_sectors,
> if (*pnum > nb_sectors) {
> *pnum = nb_sectors;
> }
> +if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
> +*file = bs;
> +}
> return ret;
> }
>
>
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 01/14] block: Add "file" output parameter to block status query functions
On 11/25/2015 12:39 AM, Fam Zheng wrote: > The added parameter can be used to return the BDS pointer which the > valid offset is referring to. It's value should be ignored unless s/It's/Its/ (remember, "It's" is valid only if "It is" also works in the same place) > BDRV_BLOCK_OFFSET_VALID in ret is set. > > Until block drivers fill in the right value, let's clear it explicitly > right before calling .bdrv_get_block_status. > > Signed-off-by: Fam Zheng > --- With that fix, Reviewed-by: Eric Blake -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 13/14] qemu-img: Use QAPI visitor to generate JSON
On Wed, 11/25 08:42, Eric Blake wrote:
> On 11/25/2015 12:39 AM, Fam Zheng wrote:
> > A visible improvement is that "filename" is now included in the output
> > if it's valid.
> >
> > Signed-off-by: Fam Zheng
> > ---
> > qemu-img.c | 39 ---
> > tests/qemu-iotests/122.out | 96
> > ++
> > 2 files changed, 79 insertions(+), 56 deletions(-)
> >
>
> > @@ -2155,21 +2161,26 @@ static void dump_map_entry(OutputFormat
> > output_format, MapEntry *e,
> > }
> > break;
> > case OFORMAT_JSON:
> > -printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
> > - " \"depth\": %ld,"
> > - " \"zero\": %s, \"data\": %s",
> > - (e->start == 0 ? "[" : ",\n"),
> > - e->start, e->length, e->depth,
> > - e->zero ? "true" : "false",
> > - e->data ? "true" : "false");
> > -if (e->has_offset) {
> > -printf(", \"offset\": %"PRId64"", e->offset);
> > -}
> > -putchar('}');
> > +ov = qmp_output_visitor_new();
> > +visit_type_MapEntry(qmp_output_get_visitor(ov),
> > +&e, NULL, &error_abort);
> > +obj = qmp_output_get_qobject(ov);
> > +str = qobject_to_json(obj);
>
> It would be nice to someday add a visitor that goes directly to json,
> instead of through an intermediate QObject. But that's not for this
> series; your conversion here is sane.
>
> > @@ -2213,9 +2224,9 @@ static int get_block_status(BlockDriverState *bs,
> > int64_t sector_num,
> > e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
> > e->has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);
> > e->depth = depth;
> > -if (e->has_offset) {
> > +if (file && e->has_offset) {
> > e->has_filename = true;
> > -e->filename = bs->filename;
> > +e->filename = file->filename;
>
> Does this hunk belong in a different patch?
>
> Reviewed-by: Eric Blake
>
Yes, I can split this into a separate patch.
Fam
Re: [Qemu-devel] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct
On Wed, 11/25 08:36, Eric Blake wrote:
> On 11/25/2015 12:39 AM, Fam Zheng wrote:
> > The "flags" bit mask is expanded to two booleans, "data" and "zero";
> > "bs" is replaced with "filename" string.
> >
> > Signed-off-by: Fam Zheng
> > ---
> > qapi/block-core.json | 28
> > qemu-img.c | 48 ++--
> > 2 files changed, 50 insertions(+), 26 deletions(-)
> >
>
> > +##
> > +
> > +{ 'struct': 'MapEntry',
>
> Blank line is not typical here.
This is copied from ImageInfo above. Removing.
>
> > + 'data': {'start': 'int', 'length': 'int', 'data': 'bool',
> > + 'zero': 'bool', 'depth': 'int', '*offset': 'int',
> > + '*filename': 'str' } }
>
> Struct looks fine.
>
> >
> > -if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) ==
> > BDRV_BLOCK_DATA) {
> > +if (e->data && !e->zero) {
> > printf("%#-16"PRIx64"%#-16"PRIx64"%#-16"PRIx64"%s\n",
> > - e->start, e->length, e->offset, e->bs->filename);
> > + e->start, e->length, e->offset,
> > + e->has_filename ? e->filename : "");
> > }
>
> This blindly prints e->offset, even though...[1]
Will add check.
>
> > case OFORMAT_JSON:
> > -printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64",
> > \"depth\": %d,"
> > +printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
> > + " \"depth\": %ld,"
>
> %ld is wrong; it needs to be PRId64.
Yes.
>
> > " \"zero\": %s, \"data\": %s",
>
> Worth joining the two short lines into one?
OK.
>
> > @@ -2219,10 +2208,15 @@ static int get_block_status(BlockDriverState *bs,
> > int64_t sector_num,
> >
> > e->start = sector_num * BDRV_SECTOR_SIZE;
> > e->length = nb_sectors * BDRV_SECTOR_SIZE;
> > -e->flags = ret & ~BDRV_BLOCK_OFFSET_MASK;
> > +e->data = !!(ret & BDRV_BLOCK_DATA);
> > +e->zero = !!(ret & BDRV_BLOCK_ZERO);
> > e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
> > +e->has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);
>
> [1]... offset might be garbage if has_offset is false.
>
> > e->depth = depth;
> > -e->bs = bs;
> > +if (e->has_offset) {
> > +e->has_filename = true;
> > +e->filename = bs->filename;
> > +}
> > return 0;
> > }
>
> Are we guaranteed that bs->filename is non-NULL when offset is set? Or
> does this need to be if (e->has_offset && bs->filename)?
It's an array field:
struct BlockDriverState {
...
char filename[PATH_MAX];
So I think this is OK.
>
> >
> > @@ -2307,9 +2301,11 @@ static int img_map(int argc, char **argv)
> > goto out;
> > }
> >
> > -if (curr.length != 0 && curr.flags == next.flags &&
> > +if (curr.length != 0 && curr.zero == next.zero &&
> > +curr.data == next.data &&
> > curr.depth == next.depth &&
> > -((curr.flags & BDRV_BLOCK_OFFSET_VALID) == 0 ||
> > +!strcmp(curr.filename, next.filename) &&
>
> Is this strcmp valid even when !has_filename?
No, will check it.
Thanks for reviewing!
Fam
>
> > +(!curr.has_offset ||
> > curr.offset + curr.length == next.offset)) {
> > curr.length += next.length;
> > continue;
> >
>
> --
> Eric Blake eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
Re: [Qemu-devel] [PATCH v9] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
On 11/25/2015 09:23 PM, Eric Blake wrote: >> +static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator, >> +char >> *mediaType) > > Unusual indentation; more typical is: > > | static kern_return_t FindEjectableOpticalMedia(io_iterator_t > *mediaIterator, > | char *mediatType) And then my mailer messes it up :( > static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator, >char *mediatType) Let's see if that's better (the 'char' is directly beneath the 'io_iterator_t'). -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] spapr: Add /system-id
On 11/26/2015 11:49 AM, David Gibson wrote:
On Wed, Nov 25, 2015 at 04:15:01PM +0100, Alexander Graf wrote:
On 18.11.15 11:49, David Gibson wrote:
On Wed, Nov 18, 2015 at 06:45:39PM +1100, Alexey Kardashevskiy wrote:
On 11/09/2015 07:47 PM, David Gibson wrote:
On Mon, Nov 09, 2015 at 05:47:17PM +1100, Alexey Kardashevskiy wrote:
Section B.6.2.1 Root Node Properties of PAPR specification defines
a set of properties which shall be present in the device tree root,
one of these properties is "system-id" which "should be unique across
all systems and all manufacturers". Since UUID is meant to be unique,
it makes sense to use it as "system-id".
This adds "system-id" property to the device tree root when not empty.
Signed-off-by: Alexey Kardashevskiy
---
This might be expected by AIX so here is the patch.
I am really not sure if it makes sense to initialize property when
UUID is all zeroes as the requirement is "unique" and zero-uuid is
not.
Yeah, I think it would be better to omit system-id entirely when a
UUID hasn't been supplied.
so this did not go anywhere yet, did it?
No.
So where is it stuck?
I was waiting for a respin which didn't set the property when a UUID
hadn't been given.
This is the original patch:
+if (qemu_uuid_set) {
+_FDT((fdt_property_string(fdt, "system-id", buf)));
+}
I does not set property if qemu_uuid_set==false already. What did I miss?
--
Alexey
Re: [Qemu-devel] [PATCH v9] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
On 11/25/2015 09:10 PM, Programmingkid wrote:
> Mac OS X can be picky when it comes to allowing the user
> to use physical devices in QEMU. Most mounted volumes
> appear to be off limits to QEMU. If an issue is detected,
> a message is displayed showing the user how to unmount a
> volume.
>
> Signed-off-by: John Arbuckle
>
> ---
> Added DVD support - real DVD media can now be used in QEMU!
> Changed name of FindEjectableCDMedia to FindEjectableOpticalMedia.
> Added mediaType parameter to FindEjectableOpticalMedia() for media
> indentification.
> Removed unneeded FindEjectableCDMedia() prototype.
> FindEjectableOpticalMedia() now searches for both DVD's and CD's.
>
> +++ b/block/raw-posix.c
> @@ -42,9 +42,9 @@
> #include
> #include
> #include
> -//#include
> +#include
> #include
> -#endif
> +#endif /* (__APPLE__) && (__MACH__) */
>
I don't know if my review of v8 crossed your posting of this patch, but
I suggested that this hunk belongs in its own patch.
> #ifdef __sun__
> #define _POSIX_PTHREAD_SEMANTICS 1
> @@ -1975,32 +1975,44 @@ BlockDriver bdrv_file = {
> /* host device */
>
> #if defined(__APPLE__) && defined(__MACH__)
> -static kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator );
> static kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
> CFIndex maxPathSize, int flags);
> -kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator )
> +static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
> +char
> *mediaType)
Unusual indentation; more typical is:
| static kern_return_t FindEjectableOpticalMedia(io_iterator_t
*mediaIterator,
| char *mediatType)
> +int index;
> +for (index = 0; index < ARRAY_SIZE(matching_array); index++) {
> +classesToMatch = IOServiceMatching(matching_array[index]);
> +if (classesToMatch == NULL) {
> +printf("IOServiceMatching returned a NULL dictionary.\n");
Leftover debugging?
> +} else {
> +CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
Missing indentation.
> +
> kCFBooleanTrue);
> +}
> +kernResult = IOServiceGetMatchingServices(masterPort, classesToMatch,
> +
> mediaIterator);
More unusual indentation.
> +if (KERN_SUCCESS != kernResult) {
> +printf("IOServiceGetMatchingServices returned %d\n", kernResult);
> +}
>
> +/* If you found a match, leave the loop */
> +if (*mediaIterator != 0) {
> +DPRINTF("Matching using %s\n", matching_array[index]);
> +snprintf(mediaType, strlen(matching_array[index])+1, "%s",
Spaces around binary '+'.
> +/* if a working partition on the device was not found */
> +if (partition_found == false) {
> +error_setg(errp, "Error: Failed to find a working partition on "
> +
> "disc!\n");
and I already pointed out on v8 that this is not the correct usage of
error_setg(). So, here's hoping v10 addresses the comments here and
elsewhere.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v9] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
Mac OS X can be picky when it comes to allowing the user
to use physical devices in QEMU. Most mounted volumes
appear to be off limits to QEMU. If an issue is detected,
a message is displayed showing the user how to unmount a
volume.
Signed-off-by: John Arbuckle
---
Added DVD support - real DVD media can now be used in QEMU!
Changed name of FindEjectableCDMedia to FindEjectableOpticalMedia.
Added mediaType parameter to FindEjectableOpticalMedia() for media
indentification.
Removed unneeded FindEjectableCDMedia() prototype.
FindEjectableOpticalMedia() now searches for both DVD's and CD's.
block/raw-posix.c | 138 ++---
1 files changed, 100 insertions(+), 38 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index ccfec1c..a11a9e7 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -42,9 +42,9 @@
#include
#include
#include
-//#include
+#include
#include
-#endif
+#endif /* (__APPLE__) && (__MACH__) */
#ifdef __sun__
#define _POSIX_PTHREAD_SEMANTICS 1
@@ -1975,32 +1975,44 @@ BlockDriver bdrv_file = {
/* host device */
#if defined(__APPLE__) && defined(__MACH__)
-static kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator );
static kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
CFIndex maxPathSize, int flags);
-kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator )
+static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
+char
*mediaType)
{
kern_return_t kernResult;
mach_port_t masterPort;
CFMutableDictionaryRef classesToMatch;
+const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass};
kernResult = IOMasterPort( MACH_PORT_NULL, &masterPort );
if ( KERN_SUCCESS != kernResult ) {
printf( "IOMasterPort returned %d\n", kernResult );
}
-classesToMatch = IOServiceMatching( kIOCDMediaClass );
-if ( classesToMatch == NULL ) {
-printf( "IOServiceMatching returned a NULL dictionary.\n" );
-} else {
-CFDictionarySetValue( classesToMatch, CFSTR( kIOMediaEjectableKey ),
kCFBooleanTrue );
-}
-kernResult = IOServiceGetMatchingServices( masterPort, classesToMatch,
mediaIterator );
-if ( KERN_SUCCESS != kernResult )
-{
-printf( "IOServiceGetMatchingServices returned %d\n", kernResult );
-}
+int index;
+for (index = 0; index < ARRAY_SIZE(matching_array); index++) {
+classesToMatch = IOServiceMatching(matching_array[index]);
+if (classesToMatch == NULL) {
+printf("IOServiceMatching returned a NULL dictionary.\n");
+} else {
+CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
+
kCFBooleanTrue);
+}
+kernResult = IOServiceGetMatchingServices(masterPort, classesToMatch,
+
mediaIterator);
+if (KERN_SUCCESS != kernResult) {
+printf("IOServiceGetMatchingServices returned %d\n", kernResult);
+}
+/* If you found a match, leave the loop */
+if (*mediaIterator != 0) {
+DPRINTF("Matching using %s\n", matching_array[index]);
+snprintf(mediaType, strlen(matching_array[index])+1, "%s",
+
matching_array[index]);
+break;
+}
+}
return kernResult;
}
@@ -2033,7 +2045,36 @@ kern_return_t GetBSDPath(io_iterator_t mediaIterator,
char *bsdPath,
return kernResult;
}
-#endif
+/* Sets up a real cdrom for use in QEMU */
+static bool setup_cdrom(char *bsd_path, Error **errp)
+{
+int index, num_of_test_partitions = 2, fd;
+char test_partition[MAXPATHLEN];
+bool partition_found = false;
+
+/* look for a working partition */
+for (index = 0; index < num_of_test_partitions; index++) {
+snprintf(test_partition, sizeof(test_partition), "%ss%d", bsd_path,
+
index);
+fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);
+if (fd >= 0) {
+partition_found = true;
+qemu_close(fd);
+break;
+}
+}
+
+/* if a working partition on the device was not found */
+if (partition_found == false) {
+error_setg(errp, "Error: Failed to find a working partition on "
+
"disc!\n");
+} else {
+DPRINTF("Using %s as optical disc\n", test_partition);
+pstrcpy(bsd_path, MAXPATHLEN, test_partition);
+}
+return partition_found;
+}
+#endif /* defined(__APPLE__) && defined(__MACH__) */
static int hdev_probe_device(const char *filename)
{
@@ -2115,6 +2156,17 @@
Re: [Qemu-devel] [PATCH] error: Document how to accumulate multiple errors
On Tue, 11/17 17:05, Markus Armbruster wrote:
> Suggested-by: Eric Blake
> Signed-off-by: Markus Armbruster
> Reviewed-by: Eric Blake
> ---
> include/qapi/error.h | 17 +
> 1 file changed, 17 insertions(+)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 4d42cdc..b2362a5 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -76,6 +76,23 @@
> * But when all you do with the error is pass it on, please use
> * foo(arg, errp);
> * for readability.
> + *
> + * Receive and accumulate multiple errors (first one wins):
> + * Error *err = NULL, *local_err = NULL;
> + * foo(arg, &err);
> + * bar(arg, &local_err);
> + * error_propagate(&err, local_err);
> + * if (err) {
> + * handle the error...
> + * }
> + *
> + * Do *not* "optimize" this to
> + * foo(arg, &err);
> + * bar(arg, &err); // WRONG!
> + * if (err) {
> + * handle the error...
> + * }
> + * because this may pass a non-null err to bar().
> */
>
> #ifndef ERROR_H
> --
> 2.4.3
>
>
Curious if there is a recommended way to report both error messages to caller,
rather than overwriting the first with the second?
Fam
Re: [Qemu-devel] [RFC PATCH V2 0/3] IXGBE/VFIO: Add live migration support for SRIOV NIC
On Wed, Nov 25, 2015 at 7:15 PM, Dong, Eddie wrote: >> On Wed, Nov 25, 2015 at 12:21 AM, Lan Tianyu wrote: >> > On 2015年11月25日 13:30, Alexander Duyck wrote: >> >> No, what I am getting at is that you can't go around and modify the >> >> configuration space for every possible device out there. This >> >> solution won't scale. >> > >> > >> > PCI config space regs are emulation by Qemu and so We can find the >> > free PCI config space regs for the faked PCI capability. Its position >> > can be not permanent. >> >> Yes, but do you really want to edit every driver on every OS that you plan to >> support this on. What about things like direct assignment of regular >> Ethernet >> ports? What you really need is a solution that will work generically on any >> existing piece of hardware out there. > > The fundamental assumption of this patch series is to modify the driver in > guest to self-emulate or track the device state, so that the migration may be > possible. > I don't think we can modify OS, without modifying the drivers, even using the > PCIe hotplug mechanism. > In the meantime, modifying Windows OS is a big challenge given that only > Microsoft can do. While, modifying driver is relatively simple and manageable > to device vendors, if the device vendor want to support state-clone based > migration. The problem is the code you are presenting, even as a proof of concept is seriously flawed. It does a poor job of exposing how any of this can be duplicated for any other VF other than the one you are working on. I am not saying you cannot modify the drivers, however what you are doing is far too invasive. Do you seriously plan on modifying all of the PCI device drivers out there in order to allow any device that might be direct assigned to a port to support migration? I certainly hope not. That is why I have said that this solution will not scale. What I am counter proposing seems like a very simple proposition. It can be implemented in two steps. 1. Look at modifying dma_mark_clean(). It is a function called in the sync and unmap paths of the lib/swiotlb.c. If you could somehow modify it to take care of marking the pages you unmap for Rx as being dirty it will get you a good way towards your goal as it will allow you to continue to do DMA while you are migrating the VM. 2. Look at making use of the existing PCI suspend/resume calls that are there to support PCI power management. They have everything needed to allow you to pause and resume DMA for the device before and after the migration while retaining the driver state. If you can implement something that allows you to trigger these calls from the PCI subsystem such as hot-plug then you would have a generic solution that can be easily reproduced for multiple drivers beyond those supported by ixgbevf. Thanks. - Alex
Re: [Qemu-devel] [PATCH for-2.5] Avoid memory leak
On 11/25/2015 06:30 PM, dongxingshui wrote: > monitor.c: Avoid memory leak > > When send a wrong qmp command, a memory leak occurs. Fix it. Looks like the leak was introduced in 710aec9; would be worth amending the commit message to mention that. Reviewed-by: Eric Blake > > Signed-off-by: dongxingshui This looks like your first patch to qemu (at least I wasn't able to find anything from you via 'git log') - welcome to the community. I suspect that 'dongxingshui' is probably your login name; it doesn't give a Western reader like me an idea where to break into family and personal names. It's a bit nicer to use a 'Full Name' notation, or even to list your name in two forms (Latin and native UTF-8 characters); see this recent conversation: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg05208.html > --- > monitor.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/monitor.c b/monitor.c > index e4cf34e..af6cfc5 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -3906,6 +3906,7 @@ static void handle_qmp_command(JSONMessageParser > *parser, QList *tokens) > > err_out: > monitor_protocol_emitter(mon, data, local_err); > +error_free(local_err); > qobject_decref(data); > QDECREF(input); > QDECREF(args); > -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] spapr: Add /system-id
On Wed, Nov 25, 2015 at 04:15:01PM +0100, Alexander Graf wrote: > > > On 18.11.15 11:49, David Gibson wrote: > > On Wed, Nov 18, 2015 at 06:45:39PM +1100, Alexey Kardashevskiy wrote: > >> On 11/09/2015 07:47 PM, David Gibson wrote: > >>> On Mon, Nov 09, 2015 at 05:47:17PM +1100, Alexey Kardashevskiy wrote: > Section B.6.2.1 Root Node Properties of PAPR specification defines > a set of properties which shall be present in the device tree root, > one of these properties is "system-id" which "should be unique across > all systems and all manufacturers". Since UUID is meant to be unique, > it makes sense to use it as "system-id". > > This adds "system-id" property to the device tree root when not empty. > > Signed-off-by: Alexey Kardashevskiy > --- > > This might be expected by AIX so here is the patch. > I am really not sure if it makes sense to initialize property when > UUID is all zeroes as the requirement is "unique" and zero-uuid is > not. > >>> > >>> Yeah, I think it would be better to omit system-id entirely when a > >>> UUID hasn't been supplied. > >> > >> > >> so this did not go anywhere yet, did it? > > > > No. > > So where is it stuck? I was waiting for a respin which didn't set the property when a UUID hadn't been given. -- 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
[Qemu-devel] 答复: [RFC v1] virtio-crypto specification
Hi Varun, Thanks for the bridging. Hi Gonglei and virtio-crypto guys, As Varun said, dpacc is working on enabling the portability of data plane VNFs (leveraging software/hardware accelerators in the host) across hardware platforms, and virtio extensions for such accelerator had been recognized as a promising solution. In particular, we are interested in the use-case of a smallcell GW VNF, where hardware/host accelerators are used for crypto offloading. As part of the OPNFV initiative, we are targeting at adding features to the open-source platform, which would used as a reference implementation for NFV deployment. However, we are not member of OASIS, and is not familiar with its organization or procedure. So we start with the gap analysis and the PoC implementation, without knowing that there is actually an OASIS thread taking place in parallel. I understand it may not the the only application scenario for virtio-crypto spec you are doing, but assume it could be an interesting one in operators' NFV deployment. I believe we should definitely encourage this discussion and try to work together. I suppose we can start with an exchange of what we expect in our usecase and PoC with what you would provide with the specified virtual device. Please advise how we can start collaboration. Regards, Lingli +86 13810597148 -邮件原件- 发件人: Varun Sethi [mailto:varun.se...@freescale.com] 发送时间: 2015年11月25日 20:41 收件人: Gonglei (Arei); virtio-...@lists.oasis-open.org; qemu-devel@nongnu.org 抄送: Hanweidong (Randy); m...@redhat.com; Claudio Fontana; Huangpeng (Peter); Lauri Leukkunen; Jani Kokkonen; Denis Crasta; Arun Pathak; Lingli Deng; Caraman Mike 主题: RE: [RFC v1] virtio-crypto specification Hi Gonglei, Please find my comments inline. Regards Varun > -Original Message- > From: Gonglei (Arei) [mailto:arei.gong...@huawei.com] > Sent: Wednesday, November 25, 2015 1:14 PM > To: Sethi Varun-B16395 ; > virtio-dev@lists.oasis- open.org; qemu-devel@nongnu.org > Cc: Hanweidong (Randy) ; m...@redhat.com; > Claudio Fontana ; Huangpeng (Peter) > ; Lauri Leukkunen > ; Jani Kokkonen > ; Crasta Denis-B22176 > ; Pathak Arun-B33046 > > Subject: RE: [RFC v1] virtio-crypto specification > > Hello Varun, > > > > -Original Message- > > From: Varun Sethi [mailto:varun.se...@freescale.com] > > Sent: Wednesday, November 25, 2015 3:08 AM > > Subject: RE: [RFC v1] virtio-crypto specification > > > > Hi Gonglei, > > We have been working on the virtio-ipsec look-aside accelerator > > device specification at the OPNFV DPACC forum. The virtio-ipsec > > device is aimed at providing the protocol offload capabilities > > (offered by security hardware > > accelerator) to the VM. The device supports a control queue and > > encap/decap queue pair per guest vcpu (multi queue support). Based > > on the feature bits, a notification queue can also be supported. > > Following document gives the details for the virtio-ipsec device: > > https://wiki.opnfv.org/_media/dpacc/freescale-ipsec-virtual-accelera > > tor-rev- > 3. > > docx > > > > Currently we are focusing on userspace virtio-ipsec backend emulation. > > We are working on a POC for vhost-user IPSec backend emulation and > > guest VM kernel virtio-ipsec driver. > > > > Following document provides guest API details to utilize the > > virtio-ipsec look aside accelerator. > > https://wiki.opnfv.org/_media/dpacc/freescale-ipsec-virtual-accelera > > to > > r-gapi-r > > ev02.pdf > > > > Actually, our virtio-crypto device isn't limited on NFV scenario, but > all encrypt & decrypt scenarios which need to use para-virtualization > of accelerator hardwares. > [Varun] Agreed, but we can work towards a generic specification. > > We can look at collaborating for the virtio-crypto device definition. > > > > Welcome to join us :) > [Varun] Thanks, it would be nice if we can leverage the work done as a part of the virtio-ipsec device specification. I have also added Lingli to the mail chain. Lingli is leading the OPNFV DPACC effort. Please let us know if we can have a discussion on this. > Regards, > -Gonglei > > > Regards > > Varun > > > > -Original Message- > > From: qemu-devel-bounces+varun.sethi=freescale@nongnu.org > > [mailto:qemu-devel-bounces+varun.sethi=freescale@nongnu.org] On > > Behalf Of Gonglei (Arei) > > Sent: Friday, November 20, 2015 8:58 AM > > To: virtio-...@lists.oasis-open.org; qemu-devel@nongnu.org > > Cc: Hanweidong (Randy) ; m...@redhat.com; > > Claudio Fontana ; Huangpeng (Peter) > > ; Lauri Leukkunen > > ; Gonglei (Arei) > > ; Jani Kokkonen > > Subject: [Qemu-devel] [RFC v1] virtio-crypto specification > > > > Hi guys, > > > > After initial discussion at this year's KVM forum, I post the RFC > > version of virtio-crypto device specification now. > > > > If you have any comments, please let me know, thanks. > > > > Regards, > > -Gonglei > > > > > > 1 Crypto Device > > > > The virtio crypto dev
[Qemu-devel] [PATCH] Avoid memory leak
monitor.c: Avoid memory leak When send a wrong qmp command, a memory leak occurs. Fix it. Signed-off-by: dongxingshui --- monitor.c | 1 + 1 file changed, 1 insertion(+) diff --git a/monitor.c b/monitor.c index e4cf34e..af6cfc5 100644 --- a/monitor.c +++ b/monitor.c @@ -3906,6 +3906,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) err_out: monitor_protocol_emitter(mon, data, local_err); +error_free(local_err); qobject_decref(data); QDECREF(input); QDECREF(args); -- 2.5.1.1
Re: [Qemu-devel] [RFC PATCH V2 0/3] IXGBE/VFIO: Add live migration support for SRIOV NIC
> On Wed, Nov 25, 2015 at 12:21 AM, Lan Tianyu wrote: > > On 2015年11月25日 13:30, Alexander Duyck wrote: > >> No, what I am getting at is that you can't go around and modify the > >> configuration space for every possible device out there. This > >> solution won't scale. > > > > > > PCI config space regs are emulation by Qemu and so We can find the > > free PCI config space regs for the faked PCI capability. Its position > > can be not permanent. > > Yes, but do you really want to edit every driver on every OS that you plan to > support this on. What about things like direct assignment of regular Ethernet > ports? What you really need is a solution that will work generically on any > existing piece of hardware out there. The fundamental assumption of this patch series is to modify the driver in guest to self-emulate or track the device state, so that the migration may be possible. I don't think we can modify OS, without modifying the drivers, even using the PCIe hotplug mechanism. In the meantime, modifying Windows OS is a big challenge given that only Microsoft can do. While, modifying driver is relatively simple and manageable to device vendors, if the device vendor want to support state-clone based migration. Thx Eddie
Re: [Qemu-devel] poor virtio-scsi performance (fio testing)
On Wed, 11/25 13:10, Vasiliy Tolstov wrote:
> 2015-11-25 12:35 GMT+03:00 Stefan Hajnoczi :
> > You can get better aio=native performance with qemu.git/master. Please
> > see commit fc73548e444ae3239f6cef44a5200b5d2c3e85d1 ("virtio-blk: use
> > blk_io_plug/unplug for Linux AIO batching").
>
>
> Thanks Stefan! Does this patch only for virtio-blk or it can increase
> iops for virtio-scsi ?
>
The patch was for virtio-blk but qemu.git/master also has another patch for
virtio-scsi (5170f40 virtio-scsi: Call bdrv_io_plug/bdrv_io_unplug in cmd
request handling).
On the other hand it would be helpful to compare the difference between
virtio-blk and virtio-scsi in your environment. Could you do that?
Fam
Re: [Qemu-devel] [RFC v1] virtio-crypto specification
+Rajesh, Michal, Sebastian Denis -Original Message- From: Lingli Deng [mailto:denglin...@chinamobile.com] Sent: Wednesday, November 25, 2015 6:31 PM To: Sethi Varun-B16395 ; 'Gonglei (Arei)' ; virtio-...@lists.oasis-open.org; qemu-devel@nongnu.org Cc: 'Hanweidong (Randy)' ; m...@redhat.com; 'Claudio Fontana' ; 'Huangpeng (Peter)' ; 'Lauri Leukkunen' ; 'Jani Kokkonen' ; Crasta Denis-B22176 ; Pathak Arun-B33046 ; Caraman Mihai Claudiu-B02008 Subject: 答复: [RFC v1] virtio-crypto specification Hi Varun, Thanks for the bridging. Hi Gonglei and virtio-crypto guys, As Varun said, dpacc is working on enabling the portability of data plane VNFs (leveraging software/hardware accelerators in the host) across hardware platforms, and virtio extensions for such accelerator had been recognized as a promising solution. In particular, we are interested in the use-case of a smallcell GW VNF, where hardware/host accelerators are used for crypto offloading. As part of the OPNFV initiative, we are targeting at adding features to the open-source platform, which would used as a reference implementation for NFV deployment. However, we are not member of OASIS, and is not familiar with its organization or procedure. So we start with the gap analysis and the PoC implementation, without knowing that there is actually an OASIS thread taking place in parallel. I understand it may not the the only application scenario for virtio-crypto spec you are doing, but assume it could be an interesting one in operators' NFV deployment. I believe we should definitely encourage this discussion and try to work together. I suppose we can start with an exchange of what we expect in our usecase and PoC with what you would provide with the specified virtual device. Please advise how we can start collaboration. Regards, Lingli +86 13810597148 -邮件原件- 发件人: Varun Sethi [mailto:varun.se...@freescale.com] 发送时间: 2015年11月25日 20:41 收件人: Gonglei (Arei); virtio-...@lists.oasis-open.org; qemu-devel@nongnu.org 抄送: Hanweidong (Randy); m...@redhat.com; Claudio Fontana; Huangpeng (Peter); Lauri Leukkunen; Jani Kokkonen; Denis Crasta; Arun Pathak; Lingli Deng; Caraman Mike 主题: RE: [RFC v1] virtio-crypto specification Hi Gonglei, Please find my comments inline. Regards Varun > -Original Message- > From: Gonglei (Arei) [mailto:arei.gong...@huawei.com] > Sent: Wednesday, November 25, 2015 1:14 PM > To: Sethi Varun-B16395 ; > virtio-dev@lists.oasis- open.org; qemu-devel@nongnu.org > Cc: Hanweidong (Randy) ; m...@redhat.com; > Claudio Fontana ; Huangpeng (Peter) > ; Lauri Leukkunen > ; Jani Kokkonen > ; Crasta Denis-B22176 > ; Pathak Arun-B33046 > > Subject: RE: [RFC v1] virtio-crypto specification > > Hello Varun, > > > > -Original Message- > > From: Varun Sethi [mailto:varun.se...@freescale.com] > > Sent: Wednesday, November 25, 2015 3:08 AM > > Subject: RE: [RFC v1] virtio-crypto specification > > > > Hi Gonglei, > > We have been working on the virtio-ipsec look-aside accelerator > > device specification at the OPNFV DPACC forum. The virtio-ipsec > > device is aimed at providing the protocol offload capabilities > > (offered by security hardware > > accelerator) to the VM. The device supports a control queue and > > encap/decap queue pair per guest vcpu (multi queue support). Based > > on the feature bits, a notification queue can also be supported. > > Following document gives the details for the virtio-ipsec device: > > https://wiki.opnfv.org/_media/dpacc/freescale-ipsec-virtual-accelera > > tor-rev- > 3. > > docx > > > > Currently we are focusing on userspace virtio-ipsec backend emulation. > > We are working on a POC for vhost-user IPSec backend emulation and > > guest VM kernel virtio-ipsec driver. > > > > Following document provides guest API details to utilize the > > virtio-ipsec look aside accelerator. > > https://wiki.opnfv.org/_media/dpacc/freescale-ipsec-virtual-accelera > > to > > r-gapi-r > > ev02.pdf > > > > Actually, our virtio-crypto device isn't limited on NFV scenario, but > all encrypt & decrypt scenarios which need to use para-virtualization > of accelerator hardwares. > [Varun] Agreed, but we can work towards a generic specification. > > We can look at collaborating for the virtio-crypto device definition. > > > > Welcome to join us :) > [Varun] Thanks, it would be nice if we can leverage the work done as a part of the virtio-ipsec device specification. I have also added Lingli to the mail chain. Lingli is leading the OPNFV DPACC effort. Please let us know if we can have a discussion on this. > Regards, > -Gonglei > > > Regards > > Varun > > > > -Original Message- > > From: qemu-devel-bounces+varun.sethi=freescale@nongnu.org > > [mailto:qemu-devel-bounces+varun.sethi=freescale@nongnu.org] On > > Behalf Of Gonglei (Arei) > > Sent: Friday, November 20, 2015 8:58 AM > > To: virtio-...@lists.oasis-open.org; qemu-devel@nongnu.org > >
Re: [Qemu-devel] [PATCH] Revert "vhost: send SET_VRING_ENABLE at start/stop"
On Wed, Nov 25, 2015 at 02:42:05PM +0200, Michael S. Tsirkin wrote:
> This reverts commit 3a12f32229a046f4d4ab0a3a52fb01d2d5a1ab76.
>
> In case of live migration several queues can be enabled and not only the
> first one. So informing backend that only the first queue is enabled is
> wrong.
Reviewed-by: Yuanhan Liu
BTW, we should also update the spec about ring stop, right?
--yliu
>
> Reported-by: Thibaut Collet
> Cc: Yuanhan Liu
> Signed-off-by: Michael S. Tsirkin
> ---
> hw/virtio/vhost.c | 9 -
> 1 file changed, 9 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 1794f0d..de29968 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1226,11 +1226,6 @@ int vhost_dev_start(struct vhost_dev *hdev,
> VirtIODevice *vdev)
> }
> }
>
> -if (hdev->vhost_ops->vhost_set_vring_enable) {
> -/* only enable first vq pair by default */
> -hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0);
> -}
> -
> return 0;
> fail_log:
> vhost_log_put(hdev, false);
> @@ -1261,10 +1256,6 @@ void vhost_dev_stop(struct vhost_dev *hdev,
> VirtIODevice *vdev)
> hdev->vq_index + i);
> }
>
> -if (hdev->vhost_ops->vhost_set_vring_enable) {
> -hdev->vhost_ops->vhost_set_vring_enable(hdev, 0);
> -}
> -
> vhost_log_put(hdev, true);
> hdev->started = false;
> hdev->log = NULL;
> --
> MST
[Qemu-devel] [PATCH v2] ui/cocoa.m: Prevent activation clicks from going to guest
When QEMU is brought to the foreground, the click event that activates QEMU
should not go to the guest. Accidents happen when they do go to the guest
without giving the user a change to handle them. Buttons are clicked accidently.
Windows are closed accidently. Volumes are unmounted accidently. This patch
prevents these accidents from happening.
Signed-off-by: John Arbuckle
---
Added code that handles the right mouse button and the other mouse button.
ui/cocoa.m | 22 ++
1 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/ui/cocoa.m b/ui/cocoa.m
index 7730d0f..f23f5fd 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -696,6 +696,16 @@ QemuCocoaView *cocoaView;
mouse_event = true;
break;
case NSLeftMouseDown:
+/*
+ * This prevents the click that activates QEMU from being sent to
+ * the guest.
+ */
+if (!isMouseGrabbed && [self screenContainsPoint:p]) {
+[self grabMouse];
+[NSApp sendEvent:event];
+return;
+}
+
if ([event modifierFlags] & NSCommandKeyMask) {
buttons |= MOUSE_EVENT_RBUTTON;
} else {
@@ -704,10 +714,22 @@ QemuCocoaView *cocoaView;
mouse_event = true;
break;
case NSRightMouseDown:
+if (!isMouseGrabbed && [self screenContainsPoint:p]) {
+[self grabMouse];
+[NSApp sendEvent:event];
+return;
+}
+
buttons |= MOUSE_EVENT_RBUTTON;
mouse_event = true;
break;
case NSOtherMouseDown:
+if (!isMouseGrabbed && [self screenContainsPoint:p]) {
+[self grabMouse];
+[NSApp sendEvent:event];
+return;
+}
+
buttons |= MOUSE_EVENT_MBUTTON;
mouse_event = true;
break;
--
1.7.5.4
Re: [Qemu-devel] [PATCH v8] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
On 11/25/2015 05:24 PM, Programmingkid wrote:
> Mac OS X can be picky when it comes to allowing the user
> to use physical devices in QEMU. Most mounted volumes
> appear to be off limits to QEMU. If an issue is detected,
> a message is displayed showing the user how to unmount a
> volume.
>
> Signed-off-by: John Arbuckle
>
> ---
Right here (between the --- and diffstat) it's nice to post a changelog
of how v8 differs from v7, to help earlier reviewers focus on the
improvements.
> block/raw-posix.c | 98
> +++--
> 1 files changed, 72 insertions(+), 26 deletions(-)
> +++ b/block/raw-posix.c
> @@ -42,9 +42,8 @@
> #include
> #include
> #include
> -//#include
> #include
> -#endif
> +#endif /* (__APPLE__) && (__MACH__) */
>
This hunk looks to be an unrelated cleanup; you might want to just
propose it separately through the qemu-trivial queue (but don't forget
that even trivial patches must cc qemu-devel).
> +
> +/* look for a working partition */
> +for (index = 0; index < num_of_test_partitions; index++) {
> +snprintf(test_partition, sizeof(test_partition), "%ss%d", bsd_path,
> +
> index);
Unusual indentation. More typical would be:
snprintf(test_partition, sizeof(test_partition), "%ss%d",
bsd_path, index);
with the second line flush to the character after the ( of the first line.
> +fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);
Isn't qemu_open() supposed to provide O_LARGEFILE for ALL users
automatically? (That is, why would we ever _want_ to handle a file
using only 32-bit off_t?) But that's a separate issue; it looks like
you are copy-and-pasting from existing use of this idiom already in
raw-posix.c.
> +if (fd >= 0) {
> +partition_found = true;
> +qemu_close(fd);
> +break;
> +}
> +}
> +
> +/* if a working partition on the device was not found */
> +if (partition_found == false) {
> +error_setg(errp, "Error: Failed to find a working partition on "
> +
> "disc!\n");
Several violations of convention. error_setg() does not need a
redundant "Error: " prefix, should not end in '!' (we aren't shouting),
and should not end in newline. And with those fixes, you won't even
need the weird indentation.
error_setg(errp, "failed to find a working partition on disk");
>
> +/* Prints directions on mounting and unmounting a device */
> +static void printUnmountingDirections(const char *file_name)
Elsewhere, we use 'function_name', not 'functionName'.
> +{
> +error_report("Error: If device %s is mounted on the desktop, unmount"
> + " it first before using it in QEMU.\n",
> file_name);
> +error_report("Command to unmount device: diskutil unmountDisk %s\n",
> +
> file_name);
> +error_report("Command to mount device: diskutil mountDisk %s\n",
> +
> file_name);
Again, weird indentation. And don't use \n at the end of error_report().
> @@ -2123,32 +2162,32 @@ static int hdev_open(BlockDriverState *bs, QDict
> *options, int flags,
> int ret;
>
> #if defined(__APPLE__) && defined(__MACH__)
> -const char *filename = qdict_get_str(options, "filename");
> +const char *file_name = qdict_get_str(options, "filename");
No need to rename this variable.
> +
> +/* If a real optical drive was not found */
> +if (bsd_path[0] == '\0') {
> +error_setg(errp, "Error: failed to obtain bsd path for optical"
> + "
> drive!\n");
Again, weird indentation, redundant "Error: ", and no "!\n" at the end.
>
> +#if defined(__APPLE__) && defined(__MACH__)
> +/* if a physical device experienced an error while being opened */
> +if (strncmp(file_name, "/dev/", 5) == 0 && ret != 0) {
> +printUnmountingDirections(file_name);
Is this advice appropriate to ALL things under /dev/, or just cdroms?
> +return -1;
> +}
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
> +
> /* Since this does ioctl the device must be already opened */
> bs->sg = hdev_is_sg(bs);
>
>
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v8] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
Mac OS X can be picky when it comes to allowing the user
to use physical devices in QEMU. Most mounted volumes
appear to be off limits to QEMU. If an issue is detected,
a message is displayed showing the user how to unmount a
volume.
Signed-off-by: John Arbuckle
---
block/raw-posix.c | 98 +++--
1 files changed, 72 insertions(+), 26 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index ccfec1c..d0190c1 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -42,9 +42,8 @@
#include
#include
#include
-//#include
#include
-#endif
+#endif /* (__APPLE__) && (__MACH__) */
#ifdef __sun__
#define _POSIX_PTHREAD_SEMANTICS 1
@@ -2033,7 +2032,36 @@ kern_return_t GetBSDPath(io_iterator_t mediaIterator,
char *bsdPath,
return kernResult;
}
-#endif
+/* Sets up a real cdrom for use in QEMU */
+static bool setup_cdrom(char *bsd_path, Error **errp)
+{
+int index, num_of_test_partitions = 2, fd;
+char test_partition[MAXPATHLEN];
+bool partition_found = false;
+
+/* look for a working partition */
+for (index = 0; index < num_of_test_partitions; index++) {
+snprintf(test_partition, sizeof(test_partition), "%ss%d", bsd_path,
+
index);
+fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);
+if (fd >= 0) {
+partition_found = true;
+qemu_close(fd);
+break;
+}
+}
+
+/* if a working partition on the device was not found */
+if (partition_found == false) {
+error_setg(errp, "Error: Failed to find a working partition on "
+
"disc!\n");
+} else {
+DPRINTF("Using %s as optical disc\n", test_partition);
+pstrcpy(bsd_path, MAXPATHLEN, test_partition);
+}
+return partition_found;
+}
+#endif /* defined(__APPLE__) && defined(__MACH__) */
static int hdev_probe_device(const char *filename)
{
@@ -2115,6 +2143,17 @@ static bool hdev_is_sg(BlockDriverState *bs)
return false;
}
+/* Prints directions on mounting and unmounting a device */
+static void printUnmountingDirections(const char *file_name)
+{
+error_report("Error: If device %s is mounted on the desktop, unmount"
+ " it first before using it in QEMU.\n",
file_name);
+error_report("Command to unmount device: diskutil unmountDisk %s\n",
+
file_name);
+error_report("Command to mount device: diskutil mountDisk %s\n",
+
file_name);
+}
+
static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -2123,32 +2162,32 @@ static int hdev_open(BlockDriverState *bs, QDict
*options, int flags,
int ret;
#if defined(__APPLE__) && defined(__MACH__)
-const char *filename = qdict_get_str(options, "filename");
+const char *file_name = qdict_get_str(options, "filename");
-if (strstart(filename, "/dev/cdrom", NULL)) {
-kern_return_t kernResult;
+/* If using a real cdrom */
+if (strcmp(file_name, "/dev/cdrom") == 0) {
+char bsd_path[MAXPATHLEN];
io_iterator_t mediaIterator;
-char bsdPath[ MAXPATHLEN ];
-int fd;
-
-kernResult = FindEjectableCDMedia( &mediaIterator );
-kernResult = GetBSDPath(mediaIterator, bsdPath, sizeof(bsdPath),
-flags);
-if ( bsdPath[ 0 ] != '\0' ) {
-strcat(bsdPath,"s0");
-/* some CDs don't have a partition 0 */
-fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
-if (fd < 0) {
-bsdPath[strlen(bsdPath)-1] = '1';
-} else {
-qemu_close(fd);
-}
-filename = bsdPath;
-qdict_put(options, "filename", qstring_from_str(filename));
+FindEjectableCDMedia(&mediaIterator);
+GetBSDPath(mediaIterator, bsd_path, sizeof(bsd_path), flags);
+if (mediaIterator) {
+IOObjectRelease(mediaIterator);
+}
+
+/* If a real optical drive was not found */
+if (bsd_path[0] == '\0') {
+error_setg(errp, "Error: failed to obtain bsd path for optical"
+ "
drive!\n");
+return -1;
}
-if ( mediaIterator )
-IOObjectRelease( mediaIterator );
+/* If finding a partition on the cdrom disc failed */
+if (setup_cdrom(bsd_path, errp) == false) {
+printUnmountingDirections(bsd_path);
+return -1;
+}
+file_name = bsd_path;
+qdict_put(options, "filename", qstring_from_str(file_name)
[Qemu-devel] [PATCH v6 22/23] qapi: Split visit_end_struct() into pieces
As mentioned in previous patches, we want to call visit_end_struct()
functions unconditionally, so that visitors can release resources
tied up since the matching visit_start_struct() without also having
to worry about error priority if more than one error occurs.
Even though error_propagate() can be safely used to ignore a second
error during cleanup caused by a first error, it is simpler if the
cleanup cannot set an error, and we instead split the task of
checking that an input visitor has no unvisited input as a new
function visit_check_struct(), called only if all prior steps are
successful.
Signed-off-by: Eric Blake
---
v6: new patch, revised version of RFC based on discussion of v5 7/46
---
hmp.c | 8 +++-
hw/ppc/spapr_drc.c | 3 ++-
hw/virtio/virtio-balloon.c | 12 ++--
include/qapi/visitor-impl.h | 4 +++-
include/qapi/visitor.h | 12 ++--
qapi/opts-visitor.c | 17 +++--
qapi/qapi-dealloc-visitor.c | 2 +-
qapi/qapi-visit-core.c | 11 +--
qapi/qmp-input-visitor.c| 34 +++---
qapi/qmp-output-visitor.c | 2 +-
qom/object.c| 5 ++---
scripts/qapi-event.py | 3 ++-
scripts/qapi-visit.py | 9 -
vl.c| 9 -
14 files changed, 81 insertions(+), 50 deletions(-)
diff --git a/hmp.c b/hmp.c
index ec1d682..b09f78b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1663,7 +1663,6 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
void hmp_object_add(Monitor *mon, const QDict *qdict)
{
Error *err = NULL;
-Error *err_end = NULL;
QemuOpts *opts;
char *type = NULL;
char *id = NULL;
@@ -1696,13 +1695,12 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
if (err) {
goto out_end;
}
-
+visit_check_struct(v, &err);
out_end:
-visit_end_struct(v, &err_end);
-if (!err && !err_end) {
+visit_end_struct(v);
+if (!err) {
object_add(type, id, pdict, v, &err);
}
-error_propagate(&err, err_end);
out_clean:
opts_visitor_cleanup(ov);
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 03a1879..5ff270f 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -281,7 +281,8 @@ static void prop_get_fdt(Object *obj, Visitor *v, void
*opaque,
case FDT_END_NODE:
/* shouldn't ever see an FDT_END_NODE before FDT_BEGIN_NODE */
g_assert(fdt_depth > 0);
-visit_end_struct(v, &error_abort);
+visit_check_struct(v, &error_abort);
+visit_end_struct(v);
fdt_depth--;
break;
case FDT_PROP: {
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 1ce987a..531913b 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -136,15 +136,15 @@ static void balloon_stats_get_all(Object *obj, struct
Visitor *v,
goto out_nested;
}
}
+visit_check_struct(v, &err);
out_nested:
-error_propagate(errp, err);
-err = NULL;
-visit_end_struct(v, &err);
+visit_end_struct(v);
+if (!err) {
+visit_check_struct(v, &err);
+}
out_end:
-error_propagate(errp, err);
-err = NULL;
-visit_end_struct(v, &err);
+visit_end_struct(v);
out:
error_propagate(errp, err);
}
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 018f419..5350bdf 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -28,8 +28,10 @@ struct Visitor
* currently visit structs). */
void (*start_struct)(Visitor *v, void **obj, const char *kind,
const char *name, size_t size, Error **errp);
+/* May be NULL; most useful for input visitors. */
+void (*check_struct)(Visitor *v, Error **errp);
/* Must be provided if start_struct is present. */
-void (*end_struct)(Visitor *v, Error **errp);
+void (*end_struct)(Visitor *v);
/* May be NULL; most useful for input visitors. */
void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index cc1ff6d..f9ea325 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -57,11 +57,19 @@ typedef struct GenericList
void visit_start_struct(Visitor *v, void **obj, const char *kind,
const char *name, size_t size, Error **errp);
/**
+ * Prepare for completing a struct visit.
+ * Should be called prior to visit_end_struct() if all other intermediate
+ * visit steps were successful, to allow the caller one last chance to
+ * report errors such as remaining data that was not consumed by the visit.
+ */
+void visit_check_struct(Visitor *v, Error **errp);
+/**
* Complete a struct visit started earlier.
* Must be called after any successful use of visit_start_struct(),
- * even if intermediate processing was skipped due to errors.
+ * even if inter
[Qemu-devel] [PATCH v6 19/23] qapi-visit: Unify struct and union visit
We are finally at the point where gen_visit_struct() and
gen_visit_union() can be unified to a generic gen_visit_object().
The generated code for structs and for flat unions is unchanged.
For simple unions, a new visit_type_FOO_fields() is created,
wrapping the visit of the non-variant tag field:
|+static void visit_type_ChardevBackend_fields(Visitor *v, ChardevBackend
**obj, Error **errp)
|+{
|+Error *err = NULL;
|+
|+visit_type_ChardevBackendKind(v, &(*obj)->type, "type", &err);
|+if (err) {
|+goto out;
|+}
|+
|+out:
|+error_propagate(errp, err);
|+}
|+
| void visit_type_ChardevBackend(Visitor *v, ChardevBackend **obj, const char
*name, Error **errp)
| {
| Error *err = NULL;
|@@ -2319,7 +2332,7 @@ void visit_type_ChardevBackend(Visitor *
| if (!*obj) {
| goto out_obj;
| }
|-visit_type_ChardevBackendKind(v, &(*obj)->type, "type", &err);
|+visit_type_ChardevBackend_fields(v, obj, &err);
| if (err) {
Signed-off-by: Eric Blake
---
v6: new patch
---
scripts/qapi-visit.py | 133 +++---
1 file changed, 51 insertions(+), 82 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 421a5b5..f22ebeb 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -109,46 +109,6 @@ out:
return ret
-def gen_visit_struct(name, base, members):
-ret = gen_visit_struct_fields(name, base, members)
-
-# FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
-# *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
-# rather than leaving it non-NULL. As currently written, the caller must
-# call qapi_free_FOO() to avoid a memory leak of the partial FOO.
-ret += mcgen('''
-
-void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name,
Error **errp)
-{
-Error *err = NULL;
-
-visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s),
&err);
-if (err) {
-goto out;
-}
-if (!*obj) {
-goto out_obj;
-}
-''',
- name=name, c_name=c_name(name))
-if (base and not base.is_empty()) or members:
-ret += mcgen('''
-visit_type_%(c_name)s_fields(v, obj, &err);
-''',
- c_name=c_name(name))
-ret += mcgen('''
-out_obj:
-error_propagate(errp, err);
-err = NULL;
-visit_end_struct(v, &err);
-out:
-error_propagate(errp, err);
-}
-''')
-
-return ret
-
-
def gen_visit_list(name, element_type):
# FIXME: if *obj is NULL on entry, and the first visit_next_list()
# assigns to *obj, while a later one fails, we should clean up *obj
@@ -244,18 +204,24 @@ out:
return ret
-def gen_visit_union(name, base, variants):
+def gen_visit_object(name, base, members, variants):
ret = ''
assert base
if not base.is_empty():
ret += gen_visit_fields_decl(base)
+if members:
+ret += gen_visit_struct_fields(name, base, members)
+if variants:
+for var in variants.variants:
+# Ugly special case for simple union TODO get rid of it
+if not var.simple_union_type():
+ret += gen_visit_implicit_struct(var.type)
-for var in variants.variants:
-# Ugly special case for simple union TODO get rid of it
-if not var.simple_union_type():
-ret += gen_visit_implicit_struct(var.type)
-
+# FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
+# *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
+# rather than leaving it non-NULL. As currently written, the caller must
+# call qapi_free_FOO() to avoid a memory leak of the partial FOO.
ret += mcgen('''
void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name,
Error **errp)
@@ -272,61 +238,71 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
const char *name, Error
''',
c_name=c_name(name), name=name)
-if not base.is_empty():
+if not base.is_empty() or members:
+if members:
+type_name = c_name(name)
+cast = ''
+else:
+type_name = base.c_name()
+cast = '(%s **)' % type_name
ret += mcgen('''
-visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err);
+visit_type_%(c_name)s_fields(v, %(cast)sobj, &err);
''',
- c_name=base.c_name())
-else:
+ c_name=type_name, cast=cast)
+if variants:
+ret += gen_err_check(label='out_obj')
+
+if variants:
ret += mcgen('''
-visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
-''',
- c_type=variants.tag_member.type.c_name(),
- c_name=c_name(variants.tag_member.name),
- name=variants.tag_member.name)
-ret += gen_err_check(label='out_obj')
-ret += mcgen('''
if (!visit_start_union(v, !!(*obj)->u.d
[Qemu-devel] [PATCH v6 21/23] qapi: Simplify extra member error reporting in input visitors
When reporting that an unvisited member remains at the end of an
input visit for a struct, we were using g_hash_table_find()
coupled with a callback function that always returns true, to
locate an arbitrary member of the hash table. But if all we
need is one entry, we can get that from a single iteration on an
iterator, without needing to split logic to a callback function.
Suggested-by: Markus Armbruster
Signed-off-by: Eric Blake
---
v6: new patch, based on comments on RFC against v5 7/46
---
qapi/opts-visitor.c | 12 +++-
qapi/qmp-input-visitor.c | 14 +-
2 files changed, 8 insertions(+), 18 deletions(-)
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 37f172d..46dd9fe 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -150,17 +150,11 @@ opts_start_struct(Visitor *v, void **obj, const char
*kind,
}
-static gboolean
-ghr_true(gpointer ign_key, gpointer ign_value, gpointer ign_user_data)
-{
-return TRUE;
-}
-
-
static void
opts_end_struct(Visitor *v, Error **errp)
{
OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+GHashTableIter iter;
GQueue *any;
if (--ov->depth > 0) {
@@ -168,8 +162,8 @@ opts_end_struct(Visitor *v, Error **errp)
}
/* we should have processed all (distinct) QemuOpt instances */
-any = g_hash_table_find(ov->unprocessed_opts, &ghr_true, NULL);
-if (any) {
+g_hash_table_iter_init(&iter, ov->unprocessed_opts);
+if (g_hash_table_iter_next(&iter, NULL, (void **)&any)) {
const QemuOpt *first;
first = g_queue_peek_head(any);
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 9ff1e75..9dbe025 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -88,12 +88,6 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject
*obj, Error **errp)
qiv->nb_stack++;
}
-/** Only for qmp_input_pop. */
-static gboolean always_true(gpointer key, gpointer val, gpointer user_pkey)
-{
-*(const char **)user_pkey = (const char *)key;
-return TRUE;
-}
static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
{
@@ -102,9 +96,11 @@ static void qmp_input_pop(QmpInputVisitor *qiv, Error
**errp)
if (qiv->strict) {
GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h;
if (top_ht) {
-if (g_hash_table_size(top_ht)) {
-const char *key;
-g_hash_table_find(top_ht, always_true, &key);
+GHashTableIter iter;
+const char *key;
+
+g_hash_table_iter_init(&iter, top_ht);
+if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
error_setg(errp, QERR_QMP_EXTRA_MEMBER, key);
}
g_hash_table_unref(top_ht);
--
2.4.3
[Qemu-devel] [PATCH v6 23/23] qapi: Change visit_type_FOO() to no longer return partial objects
Returning a partial object on error is an invitation for a careless
caller to leak memory. As no one outside the testsuite was actually
relying on these semantics, it is cleaner to just document and
guarantee that ALL visit_type_FOO() functions leave a safe value
in *obj when an error is encountered during an input visitor.
Since input visitors have blind assignment semantics, we have to
track the result of whether an assignment is made all the way down
to each visitor callback implementation.
Signed-off-by: Eric Blake
---
v6: rebase on top of earlier doc and formatting improvements, mention
that *obj can be uninitialized on entry to an input visitor, rework
semantics to keep valgrind happy on uninitialized input, break some
long lines
---
include/qapi/visitor-impl.h| 6 ++---
include/qapi/visitor.h | 52 +++---
qapi/opts-visitor.c| 8 ---
qapi/qapi-dealloc-visitor.c| 9 +---
qapi/qapi-visit-core.c | 13 ++-
qapi/qmp-input-visitor.c | 17 +-
qapi/qmp-output-visitor.c | 6 +++--
qapi/string-input-visitor.c| 3 ++-
qapi/string-output-visitor.c | 3 ++-
scripts/qapi-visit.py | 40
tests/test-qmp-commands.c | 13 +--
tests/test-qmp-input-strict.c | 19 +++
tests/test-qmp-input-visitor.c | 10 ++--
13 files changed, 125 insertions(+), 74 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 5350bdf..a51aa2a 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -26,7 +26,7 @@ struct Visitor
{
/* Must be provided to visit structs (the string visitors do not
* currently visit structs). */
-void (*start_struct)(Visitor *v, void **obj, const char *kind,
+bool (*start_struct)(Visitor *v, void **obj, const char *kind,
const char *name, size_t size, Error **errp);
/* May be NULL; most useful for input visitors. */
void (*check_struct)(Visitor *v, Error **errp);
@@ -34,13 +34,13 @@ struct Visitor
void (*end_struct)(Visitor *v);
/* May be NULL; most useful for input visitors. */
-void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
+bool (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
Error **errp);
/* May be NULL */
void (*end_implicit_struct)(Visitor *v);
/* Must be set */
-void (*start_list)(Visitor *v, const char *name, Error **errp);
+bool (*start_list)(Visitor *v, const char *name, Error **errp);
/* Must be set */
GenericList *(*next_list)(Visitor *v, GenericList **list, Error **errp);
/* Must be set */
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index f9ea325..0b63a7a 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -30,6 +30,27 @@
* visitor-impl.h.
*/
+/* All qapi types have a corresponding function with a signature
+ * roughly compatible with this:
+ *
+ * void visit_type_FOO(Visitor *v, void *obj, const char *name, Error **errp);
+ *
+ * where *@obj is itself a pointer or a scalar. The visit functions for
+ * built-in types are declared here, while the functions for qapi-defined
+ * struct, union, enum, and list types are generated (see qapi-visit.h).
+ * Input visitors may receive an uninitialized *@obj, and guarantee a
+ * safe value is assigned (a new object on success, or NULL on failure).
+ * Output visitors expect *@obj to be properly initialized on entry.
+ *
+ * Additionally, all qapi structs have a generated function compatible
+ * with this:
+ *
+ * void qapi_free_FOO(void *obj);
+ *
+ * which behaves like free(), even if @obj is NULL or was only partially
+ * allocated before encountering an error.
+ */
+
/* This struct is layout-compatible with all other *List structs
* created by the qapi generator. */
typedef struct GenericList
@@ -51,10 +72,12 @@ typedef struct GenericList
* bytes, without regards to the previous value of *@obj. For other
* visitors, *@obj is the object to visit. Set *@errp on failure.
*
- * FIXME: *@obj can be modified even on error; this can lead to
- * memory leaks if clients aren't careful.
+ * Returns true if *@obj was allocated; if that happens, and an error
+ * occurs any time before the matching visit_end_struct(), then the
+ * caller (usually a visit_type_FOO() function) knows to undo the
+ * allocation before returning control further.
*/
-void visit_start_struct(Visitor *v, void **obj, const char *kind,
+bool visit_start_struct(Visitor *v, void **obj, const char *kind,
const char *name, size_t size, Error **errp);
/**
* Prepare for completing a struct visit.
@@ -73,14 +96,12 @@ void visit_end_struct(Visitor *v);
/**
* Prepare to visit an implicit struct.
- * Similar to visit_start_struct(), except that in implicit struct
- * represents a subset of keys t
[Qemu-devel] [PATCH v6 16/23] qapi: Track all failures between visit_start/stop
Inside the generated code between visit_start_struct() and
visit_end_struct(), we were blindly setting the error into
the caller's errp parameter. But a future patch to split
visit_end_struct() will require that we take action based
on whether an error has occurred, which requires us to track
all actions through a local err. Rewrite the visits to be
more in line with the other generated calls.
Signed-off-by: Eric Blake
---
v6: based loosely on v5 7/46, but mostly a rewrite to get the last
generated code in the same form as all the others, so that the
later conversion to split visit_check_struct() will be easier
---
scripts/qapi-visit.py | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 8c964b5..391bdfb 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -123,12 +123,18 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
const char *name, Error
Error *err = NULL;
visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s),
&err);
-if (!err) {
-if (*obj) {
-visit_type_%(c_name)s_fields(v, obj, errp);
-}
-visit_end_struct(v, &err);
+if (err) {
+goto out;
}
+if (!*obj) {
+goto out_obj;
+}
+visit_type_%(c_name)s_fields(v, obj, &err);
+out_obj:
+error_propagate(errp, err);
+err = NULL;
+visit_end_struct(v, &err);
+out:
error_propagate(errp, err);
}
''',
--
2.4.3
[Qemu-devel] [PATCH v6 13/23] qapi: Add type.is_empty() helper
And use it in qapi-types and qapi-event. Down the road, we may
want to lift our artificial restriction of no variants at the
top level of an event, at which point, inlining our check for
whether members is empty will no longer be sufficient. More
immediately, the new .is_empty() helper will help fix a bug in
qapi-visit.
Signed-off-by: Eric Blake
---
v6: new patch
---
scripts/qapi-event.py | 6 +++---
scripts/qapi-types.py | 2 +-
scripts/qapi.py | 3 +++
3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 720486f..51128f4 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -39,7 +39,7 @@ def gen_event_send(name, arg_type):
''',
proto=gen_event_send_proto(name, arg_type))
-if arg_type and arg_type.members:
+if arg_type and not arg_type.is_empty():
ret += mcgen('''
QmpOutputVisitor *qov;
Visitor *v;
@@ -58,7 +58,7 @@ def gen_event_send(name, arg_type):
''',
name=name)
-if arg_type and arg_type.members:
+if arg_type and not arg_type.is_empty():
ret += mcgen('''
qov = qmp_output_visitor_new();
g_assert(qov);
@@ -90,7 +90,7 @@ def gen_event_send(name, arg_type):
''',
c_enum=c_enum_const(event_enum_name, name))
-if arg_type and arg_type.members:
+if arg_type and not arg_type.is_empty():
ret += mcgen('''
out:
qmp_output_visitor_cleanup(qov);
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index d0e1f74..af6b324 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -76,7 +76,7 @@ struct %(c_name)s {
# potential issues with attempting to malloc space for zero-length
# structs in C, and also incompatibility with C++ (where an empty
# struct is size 1).
-if not (base and base.members) and not members and not variants:
+if (not base or base.is_empty()) and not members and not variants:
ret += mcgen('''
char qapi_dummy_field_for_empty_struct;
''')
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 4197b30..45bc5a7 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -975,6 +975,9 @@ class QAPISchemaObjectType(QAPISchemaType):
# See QAPISchema._make_implicit_object_type()
return self.name[0] == ':'
+def is_empty(self):
+return not self.members and not self.variants
+
def c_name(self):
assert not self.is_implicit()
return QAPISchemaType.c_name(self)
--
2.4.3
[Qemu-devel] [PATCH v6 20/23] qapi: Rework deallocation of partial struct
Commit cee2dedb noticed that if you have a partial flat union
(such as if an input parse failed due to a missing
discriminator), calling the dealloc visitor could result in
trying to dereference the NULL pointer. But the fix it proposed
requires the use of a 'data' member in the union, which may or
may not be the same size as other branches of the union
(consider a 32-bit platform where one of the branches is an
int64), so it feels fairly dirty. A better fix is to tweak all
of the generated visit_type_implicit_FOO() functions to avoid
dereferencing NULL in the first place, by not visiting the
fields if the struct pointer itself is not present, at which
point we no longer even need visit_start_union(). And no one
was implementing visit_end_union() callbacks.
While rewriting the code, use patterns that are closer to what
is used elsewhere in the generated visitors, by using 'goto'
to cleanup labels rather than putting followup code under 'if'
conditions. The change keeps the contract that any successful
use of visit_start_implicit_struct() will be paired with a
matching visit_end_implicit_struct(), even if intermediate
processing is skipped.
As an example of the changes to generated code:
|@@ -1331,10 +1331,16 @@ static void visit_type_implicit_Blockdev
| Error *err = NULL;
|
| visit_start_implicit_struct(v, (void **)obj,
sizeof(BlockdevOptionsArchipelago), &err);
|-if (!err) {
|-visit_type_BlockdevOptionsArchipelago_fields(v, obj, errp);
|-visit_end_implicit_struct(v);
|+if (err) {
|+goto out;
|+}
|+if (obj && !*obj) {
|+goto out_obj;
| }
|+visit_type_BlockdevOptionsArchipelago_fields(v, obj, &err);
|+out_obj:
|+visit_end_implicit_struct(v);
|+out:
| error_propagate(errp, err);
| }
...
|@@ -1479,9 +1539,6 @@ void visit_type_BlockdevOptions(Visitor
| if (err) {
| goto out_obj;
| }
|-if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
|-goto out_obj;
|-}
| switch ((*obj)->driver) {
| case BLOCKDEV_DRIVER_ARCHIPELAGO:
| visit_type_implicit_BlockdevOptionsArchipelago(v,
&(*obj)->u.archipelago, &err);
|@@ -1570,11 +1627,6 @@ void visit_type_BlockdevOptions(Visitor
| out_obj:
| error_propagate(errp, err);
| err = NULL;
|-if (*obj) {
|-visit_end_union(v, !!(*obj)->u.data, &err);
|-}
|-error_propagate(errp, err);
|-err = NULL;
| visit_end_struct(v, &err);
Signed-off-by: Eric Blake
---
v6: rebase due to deferring 7/46, and gen_err_check() improvements;
rewrite gen_visit_implicit_struct() more like other patterns
---
include/qapi/visitor-impl.h | 5 -
include/qapi/visitor.h | 12
qapi/qapi-dealloc-visitor.c | 26 --
qapi/qapi-visit-core.c | 15 ---
scripts/qapi-visit.py | 25 +
5 files changed, 9 insertions(+), 74 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 36984a7..018f419 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -78,11 +78,6 @@ struct Visitor
/* May be NULL; most useful for input visitors. */
void (*optional)(Visitor *v, bool *present, const char *name);
-
-/* FIXME - needs to be removed */
-bool (*start_union)(Visitor *v, bool data_present, Error **errp);
-/* FIXME - needs to be removed */
-void (*end_union)(Visitor *v, bool data_present, Error **errp);
};
/**
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index b4ed469..cc1ff6d 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -236,16 +236,4 @@ void visit_type_number(Visitor *v, double *obj, const char
*name,
*/
void visit_type_any(Visitor *v, QObject **obj, const char *name, Error **errp);
-/**
- * Mark the start of visiting the branches of a union. Return true if
- * @data_present.
- * FIXME: Should not be needed
- */
-bool visit_start_union(Visitor *v, bool data_present, Error **errp);
-/**
- * Mark the end of union branches, after visit_start_union().
- * FIXME: Should not be needed
- */
-void visit_end_union(Visitor *v, bool data_present, Error **errp);
-
#endif
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 8246070..8b489a4 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -171,31 +171,6 @@ static void qapi_dealloc_type_enum(Visitor *v, int *obj,
{
}
-/* If there's no data present, the dealloc visitor has nothing to free.
- * Thus, indicate to visitor code that the subsequent union fields can
- * be skipped. This is not an error condition, since the cleanup of the
- * rest of an object can continue unhindered, so leave errp unset in
- * these cases.
- *
- * NOTE: In cases where we're attempting to deallocate an object that
- * may have missing fields, the field indicating the union type may
- * be missing. In such a case, it's possible we don't have enough
- * information to different
[Qemu-devel] [PATCH v6 11/23] ppc: Improve use of qapi visitors
The implementation of prop_get_fdt() is taking some shortcuts
with the qapi visitor functions. Document them, and use
error_abort rather than NULL to ensure that any changes to
the visitors do not break our use of shortcuts.
Signed-off-by: Eric Blake
---
v6: new patch, split from RFC on v5 7/46
---
hw/ppc/spapr_drc.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 1846b4f..03a1879 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -276,21 +276,26 @@ static void prop_get_fdt(Object *obj, Visitor *v, void
*opaque,
case FDT_BEGIN_NODE:
fdt_depth++;
name = fdt_get_name(fdt, fdt_offset, &name_len);
-visit_start_struct(v, NULL, NULL, name, 0, NULL);
+visit_start_struct(v, NULL, "fdt", name, 0, &error_abort);
break;
case FDT_END_NODE:
/* shouldn't ever see an FDT_END_NODE before FDT_BEGIN_NODE */
g_assert(fdt_depth > 0);
-visit_end_struct(v, NULL);
+visit_end_struct(v, &error_abort);
fdt_depth--;
break;
case FDT_PROP: {
int i;
prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len);
name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
-visit_start_list(v, name, NULL);
+/* Note: since v is an output visitor, we can get away
+ * with just visiting a direct sequence of uint8 into the
+ * output array, instead of creating a uint8List and using
+ * visit_type_uint8List(). */
+visit_start_list(v, name, &error_abort);
for (i = 0; i < prop_len; i++) {
-visit_type_uint8(v, (uint8_t *)&prop->data[i], NULL, NULL);
+visit_type_uint8(v, (uint8_t *)&prop->data[i], NULL,
+ &error_abort);
}
visit_end_list(v);
--
2.4.3
[Qemu-devel] [PATCH v6 17/23] qapi: Eliminate empty visit_type_FOO_fields
For empty structs, such as the 'Abort' helper type used as part
of the 'transaction' command, we were emitting a no-op
visit_type_FOO_fields(). Optimize things to instead omit calls
for empty structs. Generated code changes resemble:
|-static void visit_type_Abort_fields(Visitor *v, Abort **obj, Error **errp)
|-{
|-Error *err = NULL;
|-
|-error_propagate(errp, err);
|-}
|-
| void visit_type_Abort(Visitor *v, Abort **obj, const char *name, Error **errp)
| {
| Error *err = NULL;
|@@ -112,7 +105,6 @@ void visit_type_Abort(Visitor *v, Abort
| if (!*obj) {
| goto out_obj;
| }
|-visit_type_Abort_fields(v, obj, &err);
| out_obj:
| error_propagate(errp, err);
Another reason for doing this optimization is that it gets us
closer to merging the code for visiting structs and unions:
since flat unions have no local members, they do not need to
have a visit_type_UNION_fields() emitted, even when they start
sharing the code used to visit structs.
Signed-off-by: Eric Blake
---
v6: new patch
---
scripts/qapi-visit.py | 53 ---
1 file changed, 29 insertions(+), 24 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 391bdfb..ed4fb20 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -35,22 +35,22 @@ void visit_type_%(c_name)s(Visitor *v, %(c_type)sobj, const
char *name, Error **
def gen_visit_fields_decl(typ):
-ret = ''
-if typ.name not in struct_fields_seen:
-ret += mcgen('''
+if typ.is_empty() or typ.name in struct_fields_seen:
+return ''
+
+struct_fields_seen.add(typ.name)
+return mcgen('''
static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error
**errp);
''',
- c_type=typ.c_name())
-struct_fields_seen.add(typ.name)
-return ret
+ c_type=typ.c_name())
def gen_visit_implicit_struct(typ):
-if typ in implicit_structs_seen:
+if typ.is_empty() or typ in implicit_structs_seen:
return ''
+
implicit_structs_seen.add(typ)
-
ret = gen_visit_fields_decl(typ)
ret += mcgen('''
@@ -74,7 +74,10 @@ static void visit_type_implicit_%(c_type)s(Visitor *v,
%(c_type)s **obj, Error *
def gen_visit_struct_fields(name, base, members):
ret = ''
-if base:
+if (not base or base.is_empty()) and not members:
+return ret
+
+if base and not base.is_empty():
ret += gen_visit_fields_decl(base)
struct_fields_seen.add(name)
@@ -87,7 +90,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v,
%(c_name)s **obj, Error **e
''',
c_name=c_name(name))
-if base:
+if base and not base.is_empty():
ret += mcgen('''
visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err);
''',
@@ -96,13 +99,9 @@ static void visit_type_%(c_name)s_fields(Visitor *v,
%(c_name)s **obj, Error **e
ret += gen_visit_fields(members, prefix='(*obj)->')
-# 'goto out' produced for base, and by gen_visit_fields() for each member
-if base or members:
-ret += mcgen('''
+ret += mcgen('''
out:
-''')
-ret += mcgen('''
error_propagate(errp, err);
}
''')
@@ -129,16 +128,22 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
const char *name, Error
if (!*obj) {
goto out_obj;
}
-visit_type_%(c_name)s_fields(v, obj, &err);
-out_obj:
-error_propagate(errp, err);
-err = NULL;
-visit_end_struct(v, &err);
-out:
-error_propagate(errp, err);
-}
''',
name=name, c_name=c_name(name))
+if (base and not base.is_empty()) or members:
+ret += mcgen('''
+visit_type_%(c_name)s_fields(v, obj, &err);
+''',
+ c_name=c_name(name))
+ret += mcgen('''
+out_obj:
+error_propagate(errp, err);
+err = NULL;
+visit_end_struct(v, &err);
+out:
+error_propagate(errp, err);
+}
+''')
return ret
@@ -300,7 +305,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
const char *name, Error
''',
c_type=simple_union_type.c_name(),
c_name=c_name(var.name))
-else:
+elif not var.type.is_empty():
ret += mcgen('''
visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
''',
--
2.4.3
[Qemu-devel] [PATCH v6 18/23] qapi: Canonicalize missing object to :empty
Now that we elide unnecessary visits of empty types, we can
start using the special ':empty' type in more places. By using
the empty type as the base class of every explicit struct or
union, and as the default data for any command or event, we can
simplify later logic in qapi-{visit,commands,event} by merely
checking whether the type is empty, without also having to worry
whether a type was even supplied.
Note that gen_object() in qapi-types still has to check for a
base, because it is also called for alternates (which have no
base).
No change to generated code.
Signed-off-by: Eric Blake
---
v6: new patch
---
scripts/qapi-commands.py| 7 ++---
scripts/qapi-event.py | 7 ++---
scripts/qapi-types.py | 4 +--
scripts/qapi-visit.py | 12 +
scripts/qapi.py | 22
tests/qapi-schema/event-case.out| 2 +-
tests/qapi-schema/flat-union-empty.out | 1 +
tests/qapi-schema/ident-with-escape.out | 1 +
tests/qapi-schema/indented-expr.out | 4 +--
tests/qapi-schema/qapi-schema-test.out | 45 ++---
tests/qapi-schema/union-clash-data.out | 2 ++
tests/qapi-schema/union-empty.out | 1 +
12 files changed, 78 insertions(+), 30 deletions(-)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 38cbffc..0f3cc57 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -65,7 +65,8 @@ def gen_marshal_vars(arg_type, ret_type):
''',
c_type=ret_type.c_type())
-if arg_type and not arg_type.is_empty():
+assert arg_type
+if not arg_type.is_empty():
ret += mcgen('''
QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
QapiDeallocVisitor *qdv;
@@ -97,7 +98,7 @@ def gen_marshal_vars(arg_type, ret_type):
def gen_marshal_input_visit(arg_type, dealloc=False):
ret = ''
-if not arg_type or arg_type.is_empty():
+if arg_type.is_empty():
return ret
if dealloc:
@@ -177,7 +178,7 @@ def gen_marshal(name, arg_type, ret_type):
# 'goto out' produced by gen_marshal_input_visit->gen_visit_fields()
# for each arg_type member, and by gen_call() for ret_type
-if (arg_type and not arg_type.is_empty()) or ret_type:
+if not arg_type.is_empty() or ret_type:
ret += mcgen('''
out:
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 5dc9726..7ee74a5 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -39,7 +39,8 @@ def gen_event_send(name, arg_type):
''',
proto=gen_event_send_proto(name, arg_type))
-if arg_type and not arg_type.is_empty():
+assert arg_type
+if not arg_type.is_empty():
ret += mcgen('''
QObject *obj;
QmpOutputVisitor *qov;
@@ -58,7 +59,7 @@ def gen_event_send(name, arg_type):
''',
name=name)
-if arg_type and not arg_type.is_empty():
+if not arg_type.is_empty():
c_name = 'NULL'
if not arg_type.is_implicit():
c_name = '"%s"' % arg_type.c_name()
@@ -91,7 +92,7 @@ out_obj:
''',
c_enum=c_enum_const(event_enum_name, name))
-if arg_type and not arg_type.is_empty():
+if not arg_type.is_empty():
ret += mcgen('''
out:
qmp_output_visitor_cleanup(qov);
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index af6b324..795a2bf 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -58,7 +58,7 @@ struct %(c_name)s {
''',
c_name=c_name(name))
-if base:
+if base and not base.is_empty():
ret += mcgen('''
/* Members inherited from %(c_name)s: */
''',
@@ -226,7 +226,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
def visit_object_type(self, name, info, base, members, variants):
self._fwdecl += gen_fwd_object_or_array(name)
self.decl += gen_object(name, base, members, variants)
-if base:
+if not base.is_implicit():
self.decl += gen_upcast(name, base)
self._gen_type_cleanup(name)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index ed4fb20..421a5b5 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -74,10 +74,11 @@ static void visit_type_implicit_%(c_type)s(Visitor *v,
%(c_type)s **obj, Error *
def gen_visit_struct_fields(name, base, members):
ret = ''
-if (not base or base.is_empty()) and not members:
+assert base
+if base.is_empty() and not members:
return ret
-if base and not base.is_empty():
+if not base.is_empty():
ret += gen_visit_fields_decl(base)
struct_fields_seen.add(name)
@@ -90,7 +91,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v,
%(c_name)s **obj, Error **e
''',
c_name=c_name(name))
-if base and not base.is_empty():
+if not base.is_empty():
ret += mcgen('''
visit_type_%(c_type)s_fields
[Qemu-devel] [PATCH v6 07/23] qapi: Document visitor interfaces
The visitor interface for mapping between QObject/QemuOpts/string
and qapi has formerly been documented only by reading source code,
making it difficult to propose changes to either scripts/qapi*.py
or to clients without knowing whether those changes would be safe.
This adds documentation, including mentioning when parameters can
be NULL, and where there are still some interface warts that would
be nice to remove. In particular, I have plans to remove
visit_start_union() in a future patch.
Signed-off-by: Eric Blake
---
v6: mention that input visitors blindly assign over *obj; wording
improvements
---
include/qapi/visitor-impl.h | 33 +++-
include/qapi/visitor.h | 192 +---
2 files changed, 215 insertions(+), 10 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index d071c06..2f4c89b 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -15,21 +15,37 @@
#include "qapi/error.h"
#include "qapi/visitor.h"
+/* This file describes the callback interface for implementing a
+ * QObject visitor. For the client interface, see visitor.h. When
+ * implementing the callbacks, it is easiest to declare a struct with
+ * 'Visitor visitor;' as the first member. Semantics for the
+ * callbacks are generally similar to the counterpart public
+ * interface. */
+
struct Visitor
{
-/* Must be set */
+/* Must be provided to visit structs (the string visitors do not
+ * currently visit structs). */
void (*start_struct)(Visitor *v, void **obj, const char *kind,
const char *name, size_t size, Error **errp);
+/* Must be provided if start_struct is present. */
void (*end_struct)(Visitor *v, Error **errp);
+/* May be NULL; most useful for input visitors. */
void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
Error **errp);
+/* May be NULL */
void (*end_implicit_struct)(Visitor *v, Error **errp);
+/* Must be set */
void (*start_list)(Visitor *v, const char *name, Error **errp);
+/* Must be set */
GenericList *(*next_list)(Visitor *v, GenericList **list, Error **errp);
+/* Must be set */
void (*end_list)(Visitor *v, Error **errp);
+/* Must be set, although the helpers input_type_enum() and
+ * output_type_enum() can be used. */
void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
/* May be NULL; only needed for input visitors. */
@@ -45,23 +61,38 @@ struct Visitor
/* Only required to visit sizes differently than (*type_uint64)(). */
void (*type_size)(Visitor *v, uint64_t *obj, const char *name,
Error **errp);
+
/* Must be set. */
void (*type_bool)(Visitor *v, bool *obj, const char *name, Error **errp);
+/* Must be set */
void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp);
+
+/* Must be provided to visit numbers (the opts visitor does not
+ * currently visit non-integers). */
void (*type_number)(Visitor *v, double *obj, const char *name,
Error **errp);
+/* Must be provided to visit arbitrary QTypes (the opts and string
+ * visitors do not currently visit arbitrary types). */
void (*type_any)(Visitor *v, QObject **obj, const char *name,
Error **errp);
/* May be NULL; most useful for input visitors. */
void (*optional)(Visitor *v, bool *present, const char *name);
+/* FIXME - needs to be removed */
bool (*start_union)(Visitor *v, bool data_present, Error **errp);
+/* FIXME - needs to be removed */
void (*end_union)(Visitor *v, bool data_present, Error **errp);
};
+/**
+ * A generic visitor.type_enum suitable for input visitors.
+ */
void input_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
+/**
+ * A generic visitor.type_enum suitable for output visitors.
+ */
void output_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index a14a16d..74159c6 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -18,6 +18,20 @@
#include "qapi/error.h"
#include
+/* This file describes the client view for visiting a map between
+ * generated QAPI C structs and another representation (command line
+ * options, strings, or QObjects). An input visitor converts from
+ * some other form into QAPI representation; an output visitor
+ * converts from QAPI back into another form. In the descriptions
+ * below, an object or dictionary refers to a JSON '{}', and a list
+ * refers to a JSON '[]'. These functions seldom need to be called
+ * directly, but are ins
[Qemu-devel] [PATCH v6 08/23] qapi: Drop unused error argument for list and implicit struct
No backend was setting an error when ending the visit of a list
or implicit struct. Make the callers a bit easier to follow by
making this a part of the contract, and removing the errp
argument - callers can then unconditionally end an object as
part of cleanup without having to think about whether a second
error is dominated by a first, because there is no second error.
A later patch will then tackle the larger task of splitting
visit_end_struct(), which can indeed set an error.
Signed-off-by: Eric Blake
---
v6: new patch, split from RFC on v5 7/46
---
hw/ppc/spapr_drc.c | 2 +-
include/qapi/visitor-impl.h | 4 ++--
include/qapi/visitor.h | 8 +---
qapi/opts-visitor.c | 2 +-
qapi/qapi-dealloc-visitor.c | 4 ++--
qapi/qapi-visit-core.c | 8
qapi/qmp-input-visitor.c | 9 ++---
qapi/qmp-output-visitor.c| 2 +-
qapi/string-input-visitor.c | 2 +-
qapi/string-output-visitor.c | 2 +-
scripts/qapi-visit.py| 10 +++---
11 files changed, 23 insertions(+), 30 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index f34bc04..1846b4f 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -293,7 +293,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, void
*opaque,
visit_type_uint8(v, (uint8_t *)&prop->data[i], NULL, NULL);
}
-visit_end_list(v, NULL);
+visit_end_list(v);
break;
}
default:
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 2f4c89b..36984a7 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -35,14 +35,14 @@ struct Visitor
void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
Error **errp);
/* May be NULL */
-void (*end_implicit_struct)(Visitor *v, Error **errp);
+void (*end_implicit_struct)(Visitor *v);
/* Must be set */
void (*start_list)(Visitor *v, const char *name, Error **errp);
/* Must be set */
GenericList *(*next_list)(Visitor *v, GenericList **list, Error **errp);
/* Must be set */
-void (*end_list)(Visitor *v, Error **errp);
+void (*end_list)(Visitor *v);
/* Must be set, although the helpers input_type_enum() and
* output_type_enum() can be used. */
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 74159c6..b4ed469 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -77,9 +77,11 @@ void visit_start_implicit_struct(Visitor *v, void **obj,
size_t size,
/**
* Complete an implicit struct visit started earlier.
* Must be called after any successful use of visit_start_implicit_struct(),
- * even if intermediate processing was skipped due to errors.
+ * even if intermediate processing was skipped due to errors. Unlike
+ * visit_end_struct(), there is no need for an error check; any errors
+ * such as extra input will be detected when ending the overall struct.
*/
-void visit_end_implicit_struct(Visitor *v, Error **errp);
+void visit_end_implicit_struct(Visitor *v);
/**
* Prepare to visit a list tied to an object key @name.
@@ -105,7 +107,7 @@ GenericList *visit_next_list(Visitor *v, GenericList
**list, Error **errp);
* even if intermediate processing was skipped due to errors, to allow
* the backend to release any resources.
*/
-void visit_end_list(Visitor *v, Error **errp);
+void visit_end_list(Visitor *v);
/**
* Check if an optional member @name of an object needs visiting.
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 8104e42..37f172d 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -263,7 +263,7 @@ opts_next_list(Visitor *v, GenericList **list, Error **errp)
static void
-opts_end_list(Visitor *v, Error **errp)
+opts_end_list(Visitor *v)
{
OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 5133d37..8246070 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -85,7 +85,7 @@ static void qapi_dealloc_start_implicit_struct(Visitor *v,
qapi_dealloc_push(qov, obj);
}
-static void qapi_dealloc_end_implicit_struct(Visitor *v, Error **errp)
+static void qapi_dealloc_end_implicit_struct(Visitor *v)
{
QapiDeallocVisitor *qov = to_qov(v);
void **obj = qapi_dealloc_pop(qov);
@@ -121,7 +121,7 @@ static GenericList *qapi_dealloc_next_list(Visitor *v,
GenericList **listp,
return NULL;
}
-static void qapi_dealloc_end_list(Visitor *v, Error **errp)
+static void qapi_dealloc_end_list(Visitor *v)
{
QapiDeallocVisitor *qov = to_qov(v);
void *obj = qapi_dealloc_pop(qov);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index be1fcdd..477d73a 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -36,10 +36,10 @@ void visit_start_implicit_struct(Visitor *v, void **obj,
size_t size,
}
}
-void visi
[Qemu-devel] [PATCH v6 02/23] qapi: Require int64/uint64 implementation
Now that all visitors supply both type_int64() and type_uint64()
callbacks, we can drop the redundant type_int() callback (the
public interface visit_type_int() remains, but calls into
type_int64() under the hood).
Signed-off-by: Eric Blake
---
v6: new patch, but stems from v5 23/46
---
include/qapi/visitor-impl.h | 1 -
qapi/opts-visitor.c | 1 -
qapi/qapi-dealloc-visitor.c | 1 -
qapi/qapi-visit-core.c | 50 ++--
qapi/qmp-input-visitor.c | 1 -
qapi/qmp-output-visitor.c| 1 -
qapi/string-input-visitor.c | 1 -
qapi/string-output-visitor.c | 1 -
8 files changed, 16 insertions(+), 41 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 44a21b7..70326e0 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -36,7 +36,6 @@ struct Visitor
void (*get_next_type)(Visitor *v, QType *type, bool promote_int,
const char *name, Error **errp);
-void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error **errp);
void (*type_bool)(Visitor *v, bool *obj, const char *name, Error **errp);
void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp);
void (*type_number)(Visitor *v, double *obj, const char *name,
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index bc2b976..8104e42 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -522,7 +522,6 @@ opts_visitor_new(const QemuOpts *opts)
*/
ov->visitor.type_enum = &input_type_enum;
-ov->visitor.type_int= &opts_type_int64;
ov->visitor.type_int64 = &opts_type_int64;
ov->visitor.type_uint64 = &opts_type_uint64;
ov->visitor.type_size = &opts_type_size;
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 5d1b2e6..5133d37 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -220,7 +220,6 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
v->visitor.next_list = qapi_dealloc_next_list;
v->visitor.end_list = qapi_dealloc_end_list;
v->visitor.type_enum = qapi_dealloc_type_enum;
-v->visitor.type_int = qapi_dealloc_type_int64;
v->visitor.type_int64 = qapi_dealloc_type_int64;
v->visitor.type_uint64 = qapi_dealloc_type_uint64;
v->visitor.type_bool = qapi_dealloc_type_bool;
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 6d63e40..88bed9c 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -97,19 +97,19 @@ void visit_type_enum(Visitor *v, int *obj, const char *
const strings[],
void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
{
-v->type_int(v, obj, name, errp);
+v->type_int64(v, obj, name, errp);
}
void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp)
{
-int64_t value;
+uint64_t value;
if (v->type_uint8) {
v->type_uint8(v, obj, name, errp);
} else {
value = *obj;
-v->type_int(v, &value, name, errp);
-if (value < 0 || value > UINT8_MAX) {
+v->type_uint64(v, &value, name, errp);
+if (value > UINT8_MAX) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
name ? name : "null", "uint8_t");
return;
@@ -120,14 +120,14 @@ void visit_type_uint8(Visitor *v, uint8_t *obj, const
char *name, Error **errp)
void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error
**errp)
{
-int64_t value;
+uint64_t value;
if (v->type_uint16) {
v->type_uint16(v, obj, name, errp);
} else {
value = *obj;
-v->type_int(v, &value, name, errp);
-if (value < 0 || value > UINT16_MAX) {
+v->type_uint64(v, &value, name, errp);
+if (value > UINT16_MAX) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
name ? name : "null", "uint16_t");
return;
@@ -138,14 +138,14 @@ void visit_type_uint16(Visitor *v, uint16_t *obj, const
char *name, Error **errp
void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error
**errp)
{
-int64_t value;
+uint64_t value;
if (v->type_uint32) {
v->type_uint32(v, obj, name, errp);
} else {
value = *obj;
-v->type_int(v, &value, name, errp);
-if (value < 0 || value > UINT32_MAX) {
+v->type_uint64(v, &value, name, errp);
+if (value > UINT32_MAX) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
name ? name : "null", "uint32_t");
return;
@@ -156,15 +156,7 @@ void visit_type_uint32(Visitor *v, uint32_t *obj, const
char *name, Error **errp
void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error
**errp)
{
-int64_t value;
-
-if (v->type_uint64) {
-v->type_uint64(v, obj, name, errp);
-} else {
-value = *obj;
-v->type_int(v, &v
[Qemu-devel] [PATCH v6 00/23] qapi visitor cleanups (post-introspection cleanups subset E)
Pending prerequisites: + Markus' "typedefs: Put them back into alphabetical order" https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04417.html + Markus' qapi-next branch http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/qapi-next + My v13 subset D patches: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04732.html Also available as a tag at this location: git fetch git://repo.or.cz/qemu/ericb.git qapi-cleanupv6e and will soon be part of my branch with the rest of the v5 series, at: http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi v6 notes: My set of patches related to qapi visitors has grown, and it's time that I post it on list again. Of course, since this is all 2.6 material, and there's already lots of patches earlier in the queue, I may need a v7 to pick up rebase changes. A lot of the new patches in this series are based on fallout from implementing an early RFC posted against a v5 review: https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg06878.html Backport diff from v5: 001/23:[down] 'qapi: Make all visitors supply int64/uint64 callbacks' 002/23:[down] 'qapi: Require int64/uint64 implementation' 003/23:[down] 'qapi: Consolidate visitor integer callbacks' 004/23:[down] 'qapi: Don't cast Enum* to int*' 005/23:[] [--] 'qmp: Fix reference-counting of qnull on empty output visit' 006/23:[] [--] 'qapi: Don't abuse stack to track qmp-output root' 007/23:[0100] [FC] 'qapi: Document visitor interfaces' 008/23:[down] 'qapi: Drop unused error argument for list and implicit struct' 009/23:[down] 'hmp: Improve use of qapi visitor' 010/23:[down] 'vl: Improve use of qapi visitor' 011/23:[down] 'ppc: Improve use of qapi visitors' 012/23:[down] 'balloon: Improve use of qapi visitor' 013/23:[down] 'qapi: Add type.is_empty() helper' 014/23:[down] 'qapi: Fix command with named empty argument type' 015/23:[down] 'qapi: Improve generated event use of qapi visitor' 016/23:[down] 'qapi: Track all failures between visit_start/stop' 017/23:[down] 'qapi: Eliminate empty visit_type_FOO_fields' 018/23:[down] 'qapi: Canonicalize missing object to :empty' 019/23:[down] 'qapi-visit: Unify struct and union visit' 020/23:[0029] [FC] 'qapi: Rework deallocation of partial struct' 021/23:[down] 'qapi: Simplify extra member error reporting in input visitors' 022/23:[down] 'qapi: Split visit_end_struct() into pieces' 023/23:[0174] [FC] 'qapi: Change visit_type_FOO() to no longer return partial objects' Subset F (and more?) will come later. In v5: https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05410.html I _did_ rearrange patches to try and group related features: 1-2: Groundwork cleanups 3-5: Add more test cases 6-16: Front-end cleanups 17-18: Introspection output cleanups 19-20: 'alternate' type cleanups 21-29: qapi visitor cleanups 30-45: qapi-ify netdev_add 46: add qapi shorthand for flat unions Lots of fixes based on additional testing, and rebased to track other changes that happened in the meantime. The series is huge; I can split off smaller portions as requested. In v4: https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02580.html add some more clean up patches rebase to Markus' recent work pull in part of Zoltán's work to make netdev_add a flat union, further enhancing it to be introspectible I might be able to rearrange some of these patches, or separate it into smaller independent series, if requested; but I'm posting now to get review started. In v3: https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02059.html redo cleanup of dealloc of partial struct add patches to make all visit_type_*() avoid leaks on failure add patches to allow boxed command arguments and events In v2: https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg00900.html rebase to Markus' v3 series rework how comments are emitted for fields inherited from base additional patches added for deleting colliding 'void *data' documentation updates to match code changes v1 was here: https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg05266.html https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg05325.html Eric Blake (23): qapi: Make all visitors supply int64/uint64 callbacks qapi: Require int64/uint64 implementation qapi: Consolidate visitor integer callbacks qapi: Don't cast Enum* to int* qmp: Fix reference-counting of qnull on empty output visit qapi: Don't abuse stack to track qmp-output root qapi: Document visitor interfaces qapi: Drop unused error argument for list and implicit struct hmp: Improve use of qapi visitor vl: Improve use of qapi visitor ppc: Improve use of qapi visitors balloon: Improve use of qapi visitor qapi: Add type.is_empty() helper qapi: Fix command with named empty argument type qapi: Improve generated event use of qapi visitor qapi: Track all failures between visit_start/stop qapi: Eliminate empty visit_type_FOO_fields qapi: Canonicalize missing object to :empty qapi-visit: Unify struct and union visit
[Qemu-devel] [PATCH v6 03/23] qapi: Consolidate visitor integer callbacks
Commit 4e27e819 introduced optional visitor callbacks for all
sorts of int types, but except for type_uint64() and type_size(),
none of them have ever been supplied (the generic implementation
based on using type_[u]int64() then bounds-checking works just
fine). In the interest of simplicity, it's easier to make the
visitor callback interface not have to worry about the other
sizes.
Signed-off-by: Eric Blake
---
v6: split off from v5 23/46
original version also appeared in v6-v9 of subset D
---
include/qapi/visitor-impl.h | 22 -
qapi/qapi-visit-core.c | 117 ++--
2 files changed, 59 insertions(+), 80 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 70326e0..d071c06 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -1,7 +1,7 @@
/*
* Core Definitions for QAPI Visitor implementations
*
- * Copyright (C) 2012 Red Hat, Inc.
+ * Copyright (C) 2012, 2015 Red Hat, Inc.
*
* Author: Paolo Bonizni
*
@@ -36,6 +36,16 @@ struct Visitor
void (*get_next_type)(Visitor *v, QType *type, bool promote_int,
const char *name, Error **errp);
+/* Must be set. */
+void (*type_int64)(Visitor *v, int64_t *obj, const char *name,
+ Error **errp);
+/* Must be set. */
+void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name,
+Error **errp);
+/* Only required to visit sizes differently than (*type_uint64)(). */
+void (*type_size)(Visitor *v, uint64_t *obj, const char *name,
+ Error **errp);
+/* Must be set. */
void (*type_bool)(Visitor *v, bool *obj, const char *name, Error **errp);
void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp);
void (*type_number)(Visitor *v, double *obj, const char *name,
@@ -46,16 +56,6 @@ struct Visitor
/* May be NULL; most useful for input visitors. */
void (*optional)(Visitor *v, bool *present, const char *name);
-void (*type_uint8)(Visitor *v, uint8_t *obj, const char *name, Error
**errp);
-void (*type_uint16)(Visitor *v, uint16_t *obj, const char *name, Error
**errp);
-void (*type_uint32)(Visitor *v, uint32_t *obj, const char *name, Error
**errp);
-void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name, Error
**errp);
-void (*type_int8)(Visitor *v, int8_t *obj, const char *name, Error **errp);
-void (*type_int16)(Visitor *v, int16_t *obj, const char *name, Error
**errp);
-void (*type_int32)(Visitor *v, int32_t *obj, const char *name, Error
**errp);
-void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error
**errp);
-/* visit_type_size() falls back to (*type_uint64)() if type_size is unset
*/
-void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error
**errp);
bool (*start_union)(Visitor *v, bool data_present, Error **errp);
void (*end_union)(Visitor *v, bool data_present, Error **errp);
};
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 88bed9c..be1fcdd 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -104,57 +104,48 @@ void visit_type_uint8(Visitor *v, uint8_t *obj, const
char *name, Error **errp)
{
uint64_t value;
-if (v->type_uint8) {
-v->type_uint8(v, obj, name, errp);
-} else {
-value = *obj;
-v->type_uint64(v, &value, name, errp);
-if (value > UINT8_MAX) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- name ? name : "null", "uint8_t");
-return;
-}
-*obj = value;
+value = *obj;
+v->type_uint64(v, &value, name, errp);
+if (value > UINT8_MAX) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ name ? name : "null", "uint8_t");
+return;
}
+*obj = value;
}
-void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error
**errp)
+void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name,
+ Error **errp)
{
uint64_t value;
-if (v->type_uint16) {
-v->type_uint16(v, obj, name, errp);
-} else {
-value = *obj;
-v->type_uint64(v, &value, name, errp);
-if (value > UINT16_MAX) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- name ? name : "null", "uint16_t");
-return;
-}
-*obj = value;
+value = *obj;
+v->type_uint64(v, &value, name, errp);
+if (value > UINT16_MAX) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ name ? name : "null", "uint16_t");
+return;
}
+*obj = value;
}
-void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error
**errp)
+void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name,
+ Error **errp)
{
uint64_t value;
-if (v
[Qemu-devel] [PATCH v6 15/23] qapi: Improve generated event use of qapi visitor
All other successful clients of visit_start_struct() were paired
with an unconditional visit_end_struct(); but the generated
code for events was relying on qmp_output_visitor_cleanup() to
work on an incomplete visit. Alter the code to guarantee that
the struct is completed, which will make a future patch to
split visit_end_struct() easier to reason about. While at it,
drop some assertions and comments that are not present in other
uses of the qmp output visitor, and rearrange the declaration
to make it easier for a future patch to introduce the notion of
a boxed event visit.
Signed-off-by: Eric Blake
---
v6: new patch
If desired, I can defer the hunk re-ordering the declaration of
obj to later in the series where it actually comes in handy.
---
scripts/qapi-event.py | 19 ++-
scripts/qapi.py | 5 +++--
2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 51128f4..5dc9726 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -41,9 +41,9 @@ def gen_event_send(name, arg_type):
if arg_type and not arg_type.is_empty():
ret += mcgen('''
+QObject *obj;
QmpOutputVisitor *qov;
Visitor *v;
-QObject *obj;
''')
@@ -59,27 +59,28 @@ def gen_event_send(name, arg_type):
name=name)
if arg_type and not arg_type.is_empty():
+c_name = 'NULL'
+if not arg_type.is_implicit():
+c_name = '"%s"' % arg_type.c_name()
ret += mcgen('''
qov = qmp_output_visitor_new();
-g_assert(qov);
-
v = qmp_output_get_visitor(qov);
-g_assert(v);
-/* Fake visit, as if all members are under a structure */
-visit_start_struct(v, NULL, "", "%(name)s", 0, &err);
+visit_start_struct(v, NULL, %(c_name)s, "%(name)s", 0, &err);
''',
- name=name)
+ c_name=c_name, name=name)
ret += gen_err_check()
-ret += gen_visit_fields(arg_type.members, need_cast=True)
+ret += gen_visit_fields(arg_type.members, need_cast=True,
+label='out_obj')
ret += mcgen('''
+out_obj:
visit_end_struct(v, &err);
if (err) {
goto out;
}
obj = qmp_output_get_qobject(qov);
-g_assert(obj != NULL);
+g_assert(obj);
qdict_put_obj(qmp, "data", obj);
''')
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 45bc5a7..ed2a063 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1637,7 +1637,8 @@ def gen_err_check(label='out', skiperr=False):
label=label)
-def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
+def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False,
+ label='out'):
ret = ''
if skiperr:
errparg = 'NULL'
@@ -1665,7 +1666,7 @@ def gen_visit_fields(members, prefix='', need_cast=False,
skiperr=False):
c_type=memb.type.c_name(), prefix=prefix, cast=cast,
c_name=c_name(memb.name), name=memb.name,
errp=errparg)
-ret += gen_err_check(skiperr=skiperr)
+ret += gen_err_check(skiperr=skiperr, label=label)
if memb.optional:
pop_indent()
--
2.4.3
[Qemu-devel] [PATCH v6 10/23] vl: Improve use of qapi visitor
Cache the visitor in a local variable instead of repeatedly
calling the accessor. Pass NULL for the visit_start_struct()
object (which matches the fact that we were already passing 0
for the size argument, because we aren't using the visit to
allocate a qapi struct). Pass "object" for the struct name,
for better error messages. Reflow the logic so that we don't
have to undo an object_add().
A later patch will then split the error detection currently
in visit_struct_end(), at which point we can again hoist the
object_add() to occur before the label as one of the cleanups
enabled by that split.
Signed-off-by: Eric Blake
---
v6: new patch, split from RFC on v5 7/46
---
vl.c | 26 --
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/vl.c b/vl.c
index 4d6263d..4e69815 100644
--- a/vl.c
+++ b/vl.c
@@ -2828,43 +2828,42 @@ static bool object_create_delayed(const char *type)
static int object_create(void *opaque, QemuOpts *opts, Error **errp)
{
Error *err = NULL;
+Error *err_end = NULL;
char *type = NULL;
char *id = NULL;
-void *dummy = NULL;
OptsVisitor *ov;
QDict *pdict;
bool (*type_predicate)(const char *) = opaque;
+Visitor *v;
ov = opts_visitor_new(opts);
pdict = qemu_opts_to_qdict(opts, NULL);
+v = opts_get_visitor(ov);
-visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
+visit_start_struct(v, NULL, "object", NULL, 0, &err);
if (err) {
goto out;
}
qdict_del(pdict, "qom-type");
-visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
+visit_type_str(v, &type, "qom-type", &err);
if (err) {
goto out;
}
if (!type_predicate(type)) {
-goto out;
+goto out_end;
}
qdict_del(pdict, "id");
-visit_type_str(opts_get_visitor(ov), &id, "id", &err);
+visit_type_str(v, &id, "id", &err);
if (err) {
-goto out;
+goto out_end;
}
-object_add(type, id, pdict, opts_get_visitor(ov), &err);
-if (err) {
-goto out;
-}
-visit_end_struct(opts_get_visitor(ov), &err);
-if (err) {
-qmp_object_del(id, NULL);
+out_end:
+visit_end_struct(v, &err_end);
+if (!err && !err_end) {
+object_add(type, id, pdict, v, &err);
}
out:
@@ -2873,7 +2872,6 @@ out:
QDECREF(pdict);
g_free(id);
g_free(type);
-g_free(dummy);
if (err) {
error_report_err(err);
return -1;
--
2.4.3
[Qemu-devel] [PATCH v6 12/23] balloon: Improve use of qapi visitor
Rework the control flow of balloon_stats_get_all() to make it
easier for a later patch to split visit_end_struct(). Also
switch to the uint64 visitor to match the data type.
Signed-off-by: Eric Blake
---
v6: new patch, split from RFC on v5 7/46
---
hw/virtio/virtio-balloon.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 9671635..1ce987a 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -130,10 +130,13 @@ static void balloon_stats_get_all(Object *obj, struct
Visitor *v,
if (err) {
goto out_end;
}
-for (i = 0; !err && i < VIRTIO_BALLOON_S_NR; i++) {
-visit_type_int64(v, (int64_t *) &s->stats[i], balloon_stat_names[i],
- &err);
+for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) {
+visit_type_uint64(v, &s->stats[i], balloon_stat_names[i], &err);
+if (err) {
+goto out_nested;
+}
}
+out_nested:
error_propagate(errp, err);
err = NULL;
visit_end_struct(v, &err);
--
2.4.3
[Qemu-devel] [PATCH v6 05/23] qmp: Fix reference-counting of qnull on empty output visit
Commit 6c2f9a15 ensured that we would not return NULL when the
caller used an output visitor but had nothing to visit. But
in doing so, it added a FIXME about a reference count leak
that could abort qemu in the (unlikely) case of SIZE_MAX such
visits (more plausible on 32-bit).
This fixes things by documenting the internal contracts, and
explaining why the internal function can return NULL and only
the public facing interface needs to worry about qnull(),
thus avoiding over-referencing the qnull_ global object.
It does not, however, fix the stupidity of the stack mixing
up two separate pieces of information; add a FIXME to explain
that issue.
Signed-off-by: Eric Blake
---
v6: no change
---
qapi/qmp-output-visitor.c | 30 --
tests/test-qmp-output-visitor.c | 2 ++
2 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index e66ab3c..9d0f9d1 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -29,6 +29,12 @@ typedef QTAILQ_HEAD(QStack, QStackEntry) QStack;
struct QmpOutputVisitor
{
Visitor visitor;
+/* FIXME: we are abusing stack to hold two separate pieces of
+ * information: the current root object, and the stack of objects
+ * still being built. Worse, our behavior is inconsistent:
+ * visiting two top-level scalars in a row discards the first in
+ * favor of the second, but visiting two top-level objects in a
+ * row tries to append the second object into the first. */
QStack stack;
};
@@ -41,10 +47,12 @@ static QmpOutputVisitor *to_qov(Visitor *v)
return container_of(v, QmpOutputVisitor, visitor);
}
+/* Push @value onto the stack of current QObjects being built */
static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
{
QStackEntry *e = g_malloc0(sizeof(*e));
+assert(value);
e->value = value;
if (qobject_type(e->value) == QTYPE_QLIST) {
e->is_list_head = true;
@@ -52,16 +60,20 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov,
QObject *value)
QTAILQ_INSERT_HEAD(&qov->stack, e, node);
}
+/* Grab and remove the most recent QObject from the stack */
static QObject *qmp_output_pop(QmpOutputVisitor *qov)
{
QStackEntry *e = QTAILQ_FIRST(&qov->stack);
QObject *value;
+
+assert(e);
QTAILQ_REMOVE(&qov->stack, e, node);
value = e->value;
g_free(e);
return value;
}
+/* Grab the root QObject, if any, in preparation to empty the stack */
static QObject *qmp_output_first(QmpOutputVisitor *qov)
{
QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
@@ -72,24 +84,32 @@ static QObject *qmp_output_first(QmpOutputVisitor *qov)
* handle null.
*/
if (!e) {
-return qnull();
+/* No root */
+return NULL;
}
-
+assert(e->value);
return e->value;
}
+/* Grab the most recent QObject from the stack, which must exist */
static QObject *qmp_output_last(QmpOutputVisitor *qov)
{
QStackEntry *e = QTAILQ_FIRST(&qov->stack);
+
+assert(e);
return e->value;
}
+/* Add @value to the current QObject being built.
+ * If the stack is visiting a dictionary or list, @value is now owned
+ * by that container. Otherwise, @value is now the root. */
static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
QObject *value)
{
QObject *cur;
if (QTAILQ_EMPTY(&qov->stack)) {
+/* Stack was empty, track this object as root */
qmp_output_push_obj(qov, value);
return;
}
@@ -98,13 +118,16 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov,
const char *name,
switch (qobject_type(cur)) {
case QTYPE_QDICT:
+assert(name);
qdict_put_obj(qobject_to_qdict(cur), name, value);
break;
case QTYPE_QLIST:
qlist_append_obj(qobject_to_qlist(cur), value);
break;
default:
+/* The previous root was a scalar, replace it with a new root */
qobject_decref(qmp_output_pop(qov));
+assert(QTAILQ_EMPTY(&qov->stack));
qmp_output_push_obj(qov, value);
break;
}
@@ -205,11 +228,14 @@ static void qmp_output_type_any(Visitor *v, QObject
**obj, const char *name,
qmp_output_add_obj(qov, name, *obj);
}
+/* Finish building, and return the root object. Will not be NULL. */
QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
{
QObject *obj = qmp_output_first(qov);
if (obj) {
qobject_incref(obj);
+} else {
+obj = qnull();
}
return obj;
}
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 3078442..8e6fc33 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -461,6 +461,8 @@ static void test_visitor_out_empty(TestOutputVisitorData
*data,
arg = qmp_output_get_qobject(data->qov);
g_assert(qobject_type(arg) == QTYPE_QNULL
[Qemu-devel] [PATCH v6 06/23] qapi: Don't abuse stack to track qmp-output root
The previous commit documented an inconsistency in how we are
using the stack of qmp-output-visitor. Normally, pushing a
single top-level object puts the object on the stack twice:
once as the root, and once as the current container being
appended to; but popping that struct only pops once. However,
qmp_ouput_add() was trying to either set up the added object
as the new root (works if you parse two top-level scalars in a
row: the second replaces the first as the root) or as a member
of the current container (works as long as you have an open
container on the stack; but if you have popped the first
top-level container, it then resolves to the root and still
tries to add into that existing container).
Fix the stupidity by not tracking two separate things in the
stack.
Not done here: maybe qmp_output_get_object() should assert that
the stack is empty, rather than letting users look at the current
root even while the root is still being visited.
Signed-off-by: Eric Blake
---
v6: no change
---
qapi/qmp-output-visitor.c | 70 +++
1 file changed, 22 insertions(+), 48 deletions(-)
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 9d0f9d1..4a28ce3 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -29,13 +29,8 @@ typedef QTAILQ_HEAD(QStack, QStackEntry) QStack;
struct QmpOutputVisitor
{
Visitor visitor;
-/* FIXME: we are abusing stack to hold two separate pieces of
- * information: the current root object, and the stack of objects
- * still being built. Worse, our behavior is inconsistent:
- * visiting two top-level scalars in a row discards the first in
- * favor of the second, but visiting two top-level objects in a
- * row tries to append the second object into the first. */
-QStack stack;
+QStack stack; /* Stack of containers still growing */
+QObject *root; /* Root of the output visit */
};
#define qmp_output_add(qov, name, value) \
@@ -52,6 +47,7 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov,
QObject *value)
{
QStackEntry *e = g_malloc0(sizeof(*e));
+assert(qov->root);
assert(value);
e->value = value;
if (qobject_type(e->value) == QTYPE_QLIST) {
@@ -76,28 +72,15 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov)
/* Grab the root QObject, if any, in preparation to empty the stack */
static QObject *qmp_output_first(QmpOutputVisitor *qov)
{
-QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
-
-/*
- * FIXME Wrong, because qmp_output_get_qobject() will increment
- * the refcnt *again*. We need to think through how visitors
- * handle null.
- */
-if (!e) {
-/* No root */
-return NULL;
-}
-assert(e->value);
-return e->value;
+return qov->root;
}
-/* Grab the most recent QObject from the stack, which must exist */
+/* Grab the most recent QObject from the stack, if any */
static QObject *qmp_output_last(QmpOutputVisitor *qov)
{
QStackEntry *e = QTAILQ_FIRST(&qov->stack);
-assert(e);
-return e->value;
+return e ? e->value : NULL;
}
/* Add @value to the current QObject being built.
@@ -108,28 +91,23 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov,
const char *name,
{
QObject *cur;
-if (QTAILQ_EMPTY(&qov->stack)) {
-/* Stack was empty, track this object as root */
-qmp_output_push_obj(qov, value);
-return;
-}
-
cur = qmp_output_last(qov);
-switch (qobject_type(cur)) {
-case QTYPE_QDICT:
-assert(name);
-qdict_put_obj(qobject_to_qdict(cur), name, value);
-break;
-case QTYPE_QLIST:
-qlist_append_obj(qobject_to_qlist(cur), value);
-break;
-default:
-/* The previous root was a scalar, replace it with a new root */
-qobject_decref(qmp_output_pop(qov));
-assert(QTAILQ_EMPTY(&qov->stack));
-qmp_output_push_obj(qov, value);
-break;
+if (!cur) {
+qobject_decref(qov->root);
+qov->root = value;
+} else {
+switch (qobject_type(cur)) {
+case QTYPE_QDICT:
+assert(name);
+qdict_put_obj(qobject_to_qdict(cur), name, value);
+break;
+case QTYPE_QLIST:
+qlist_append_obj(qobject_to_qlist(cur), value);
+break;
+default:
+g_assert_not_reached();
+}
}
}
@@ -249,16 +227,12 @@ void qmp_output_visitor_cleanup(QmpOutputVisitor *v)
{
QStackEntry *e, *tmp;
-/* The bottom QStackEntry, if any, owns the root QObject. See the
- * qmp_output_push_obj() invocations in qmp_output_add_obj(). */
-QObject *root = QTAILQ_EMPTY(&v->stack) ? NULL : qmp_output_first(v);
-
QTAILQ_FOREACH_SAFE(e, &v->stack, node, tmp) {
QTAILQ_REMOVE(&v->stack, e, node);
g_free(e);
}
-qobject_decref(root);
+qobject_decref(v->root);
g_free(v);
}
--
2.4.3
[Qemu-devel] [PATCH v6 01/23] qapi: Make all visitors supply int64/uint64 callbacks
Our qapi visitor contract supports multiple integer visitors:
type_int (64-bit signed; mandatory), type_int64 (64-bit signed,
optional), type_uint64 (64-bit unsigned, optional), and
type_size (64-bit unsigned, optional, with the possibility of
parsing differently than type_uint64 for the case of suffixed
sizes). But the default code treats any implementation without
type_uint64 as just falling back on type_int, which can cause
confusing results for values larger than LLONG_MAX (such as
having to pass in a negative 2s complement value on input,
and getting a negative result on output).
This patch does not fully address the disparity in parsing,
but does move towards a cleaner situation where EVERY visitor
provides both signed and unsigned variants as entry points;
then each client can either implement sane differences between
the two, or document in place with a FIXME that there is
munging going on for large values. The next patch will then
clean up the code to no longer allow conditional type_uint64.
Signed-off-by: Eric Blake
---
v6: new patch, but stems from v5 23/46
---
qapi/opts-visitor.c | 5 +++--
qapi/qapi-dealloc-visitor.c | 19 ++-
qapi/qmp-input-visitor.c | 23 ---
qapi/qmp-output-visitor.c| 15 ---
qapi/string-input-visitor.c | 22 +++---
qapi/string-output-visitor.c | 16 +---
6 files changed, 77 insertions(+), 23 deletions(-)
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index ef5fb8b..bc2b976 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -354,7 +354,7 @@ opts_type_bool(Visitor *v, bool *obj, const char *name,
Error **errp)
static void
-opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
+opts_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp)
{
OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
const QemuOpt *opt;
@@ -522,7 +522,8 @@ opts_visitor_new(const QemuOpts *opts)
*/
ov->visitor.type_enum = &input_type_enum;
-ov->visitor.type_int= &opts_type_int;
+ov->visitor.type_int= &opts_type_int64;
+ov->visitor.type_int64 = &opts_type_int64;
ov->visitor.type_uint64 = &opts_type_uint64;
ov->visitor.type_size = &opts_type_size;
ov->visitor.type_bool = &opts_type_bool;
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 737deab..5d1b2e6 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -136,8 +136,13 @@ static void qapi_dealloc_type_str(Visitor *v, char **obj,
const char *name,
}
}
-static void qapi_dealloc_type_int(Visitor *v, int64_t *obj, const char *name,
- Error **errp)
+static void qapi_dealloc_type_int64(Visitor *v, int64_t *obj,
+const char *name, Error **errp)
+{
+}
+
+static void qapi_dealloc_type_uint64(Visitor *v, uint64_t *obj,
+ const char *name, Error **errp)
{
}
@@ -159,11 +164,6 @@ static void qapi_dealloc_type_anything(Visitor *v, QObject
**obj,
}
}
-static void qapi_dealloc_type_size(Visitor *v, uint64_t *obj, const char *name,
- Error **errp)
-{
-}
-
static void qapi_dealloc_type_enum(Visitor *v, int *obj,
const char * const strings[],
const char *kind, const char *name,
@@ -220,12 +220,13 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
v->visitor.next_list = qapi_dealloc_next_list;
v->visitor.end_list = qapi_dealloc_end_list;
v->visitor.type_enum = qapi_dealloc_type_enum;
-v->visitor.type_int = qapi_dealloc_type_int;
+v->visitor.type_int = qapi_dealloc_type_int64;
+v->visitor.type_int64 = qapi_dealloc_type_int64;
+v->visitor.type_uint64 = qapi_dealloc_type_uint64;
v->visitor.type_bool = qapi_dealloc_type_bool;
v->visitor.type_str = qapi_dealloc_type_str;
v->visitor.type_number = qapi_dealloc_type_number;
v->visitor.type_any = qapi_dealloc_type_anything;
-v->visitor.type_size = qapi_dealloc_type_size;
v->visitor.start_union = qapi_dealloc_start_union;
QTAILQ_INIT(&v->stack);
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 932b5f3..96dafcb 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -224,8 +224,23 @@ static void qmp_input_get_next_type(Visitor *v, QType
*type, bool promote_int,
}
}
-static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name,
- Error **errp)
+static void qmp_input_type_int64(Visitor *v, int64_t *obj, const char *name,
+ Error **errp)
+{
+QmpInputVisitor *qiv = to_qiv(v);
+QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true));
+
+if (!qint) {
+error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
+
[Qemu-devel] [PATCH v6 04/23] qapi: Don't cast Enum* to int*
C compilers are allowed to represent enums as a smaller type
than int, if all enum values fit in the smaller type. There
are even compiler flags that force the use of this smaller
representation, and using them changes the ABI of a binary.
Therefore, our generated code for visit_type_ENUM() (for all
qapi enums) was wrong for casting Enum* to int* when calling
visit_type_enum().
It appears that no one has been doing this for qemu, because
if they had, we are potentially dereferencing beyond bounds
or even risking a SIGBUS on platforms where unaligned pointer
dereferencing is fatal. Better is to avoid the practice
entirely, and just use the correct types.
This matches the fix for alternate qapi types, earlier in
"qapi: Simplify visiting of alternate types".
Signed-off-by: Eric Blake
---
v6: new patch
---
scripts/qapi-visit.py | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index dc2a336..ddfb769 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -172,12 +172,13 @@ out:
def gen_visit_enum(name):
-# FIXME cast from enum *obj to int * invalidely assumes enum is int
return mcgen('''
void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name,
Error **errp)
{
-visit_type_enum(v, (int *)obj, %(c_name)s_lookup, "%(name)s", name, errp);
+int tmp = *obj;
+visit_type_enum(v, &tmp, %(c_name)s_lookup, "%(name)s", name, errp);
+*obj = tmp;
}
''',
c_name=c_name(name), name=name)
--
2.4.3
[Qemu-devel] [PATCH v6 09/23] hmp: Improve use of qapi visitor
Cache the visitor in a local variable instead of repeatedly
calling the accessor. Pass NULL for the visit_start_struct()
object (which matches the fact that we were already passing 0
for the size argument, because we aren't using the visit to
allocate a qapi struct). Pass "object" for the struct name,
for better error messages. Reflow the logic so that we don't
have to undo an object_add().
A later patch will then split the error detection currently
in visit_struct_end(), at which point we can again hoist the
object_add() to occur before the label as one of the cleanups
enabled by that split.
Signed-off-by: Eric Blake
---
v6: new patch, split from RFC on v5 7/46
---
hmp.c | 18 --
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/hmp.c b/hmp.c
index c2b2c16..ec1d682 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1667,9 +1667,9 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
QemuOpts *opts;
char *type = NULL;
char *id = NULL;
-void *dummy = NULL;
OptsVisitor *ov;
QDict *pdict;
+Visitor *v;
opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
if (err) {
@@ -1678,30 +1678,29 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
ov = opts_visitor_new(opts);
pdict = qdict_clone_shallow(qdict);
+v = opts_get_visitor(ov);
-visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
+visit_start_struct(v, NULL, "object", NULL, 0, &err);
if (err) {
goto out_clean;
}
qdict_del(pdict, "qom-type");
-visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
+visit_type_str(v, &type, "qom-type", &err);
if (err) {
goto out_end;
}
qdict_del(pdict, "id");
-visit_type_str(opts_get_visitor(ov), &id, "id", &err);
+visit_type_str(v, &id, "id", &err);
if (err) {
goto out_end;
}
-object_add(type, id, pdict, opts_get_visitor(ov), &err);
-
out_end:
-visit_end_struct(opts_get_visitor(ov), &err_end);
-if (!err && err_end) {
-qmp_object_del(id, NULL);
+visit_end_struct(v, &err_end);
+if (!err && !err_end) {
+object_add(type, id, pdict, v, &err);
}
error_propagate(&err, err_end);
out_clean:
@@ -1711,7 +1710,6 @@ out_clean:
qemu_opts_del(opts);
g_free(id);
g_free(type);
-g_free(dummy);
out:
hmp_handle_error(mon, &err);
--
2.4.3
[Qemu-devel] [PATCH v6 14/23] qapi: Fix command with named empty argument type
The generator special-cased
{ 'command':'foo', 'data': {} }
to avoid emitting a visitor variable, but failed to see that
{ 'struct':'NamedEmptyType, 'data': {} }
{ 'command':'foo', 'data':'NamedEmptyType' }
needs the same treatment. Without a fix to the generator, the
change to qapi-schema-test.json fails to compile with:
tests/test-qmp-marshal.c: In function ‘qmp_marshal_user_def_cmd0’:
tests/test-qmp-marshal.c:264:14: error: variable ‘v’ set but not used
[-Werror=unused-but-set-variable]
Visitor *v;
^
Signed-off-by: Eric Blake
---
v6: new patch
---
scripts/qapi-commands.py| 6 +++---
tests/qapi-schema/qapi-schema-test.json | 2 ++
tests/qapi-schema/qapi-schema-test.out | 2 ++
tests/test-qmp-commands.c | 5 +
4 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 561e47a..38cbffc 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -65,7 +65,7 @@ def gen_marshal_vars(arg_type, ret_type):
''',
c_type=ret_type.c_type())
-if arg_type:
+if arg_type and not arg_type.is_empty():
ret += mcgen('''
QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
QapiDeallocVisitor *qdv;
@@ -97,7 +97,7 @@ def gen_marshal_vars(arg_type, ret_type):
def gen_marshal_input_visit(arg_type, dealloc=False):
ret = ''
-if not arg_type:
+if not arg_type or arg_type.is_empty():
return ret
if dealloc:
@@ -177,7 +177,7 @@ def gen_marshal(name, arg_type, ret_type):
# 'goto out' produced by gen_marshal_input_visit->gen_visit_fields()
# for each arg_type member, and by gen_call() for ret_type
-if (arg_type and arg_type.members) or ret_type:
+if (arg_type and not arg_type.is_empty()) or ret_type:
ret += mcgen('''
out:
diff --git a/tests/qapi-schema/qapi-schema-test.json
b/tests/qapi-schema/qapi-schema-test.json
index 4b89527..a0fdb88 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -18,6 +18,8 @@
{ 'struct': 'Empty1', 'data': { } }
{ 'struct': 'Empty2', 'base': 'Empty1', 'data': { } }
+{ 'command': 'user_def_cmd0', 'data': 'Empty2', 'returns': 'Empty2' }
+
# for testing override of default naming heuristic
{ 'enum': 'QEnumTwo',
'prefix': 'QENUM_TWO',
diff --git a/tests/qapi-schema/qapi-schema-test.out
b/tests/qapi-schema/qapi-schema-test.out
index 2c546b7..d8f9108 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -198,6 +198,8 @@ command guest-sync :obj-guest-sync-arg -> any
gen=True success_response=True
command user_def_cmd None -> None
gen=True success_response=True
+command user_def_cmd0 Empty2 -> Empty2
+ gen=True success_response=True
command user_def_cmd1 :obj-user_def_cmd1-arg -> None
gen=True success_response=True
command user_def_cmd2 :obj-user_def_cmd2-arg -> UserDefTwo
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 9f35b80..b132775 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -12,6 +12,11 @@ void qmp_user_def_cmd(Error **errp)
{
}
+Empty2 *qmp_user_def_cmd0(Error **errp)
+{
+return g_new0(Empty2, 1);
+}
+
void qmp_user_def_cmd1(UserDefOne * ud1, Error **errp)
{
}
--
2.4.3
Re: [Qemu-devel] [PULL for-2.5 0/6] qemu-ga patch queue for 2.5
Quoting Eric Blake (2015-11-25 17:47:00)
> On 11/25/2015 03:47 PM, Michael Roth wrote:
> > The following changes since commit 1a4dab849d5d06191ab5e5850f6b8bfcad8ceb47:
> >
> > Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into
> > staging (2015-11-25 14:47:06 +)
> >
> > are available in the git repository at:
> >
> >
> > git://github.com/mdroth/qemu.git tags/qga-pull-2015-11-25-tag
> >
> > for you to fetch changes up to 35f8c32a200c8133c66642ce0dac721b3480178a:
> >
> > qga: added another non-interactive gspawn() helper file. (2015-11-25
> > 15:34:48 -0600)
> >
> >
> > qemu-ga patch queue for 2.5
> >
> > * include additional w32 MSI install components needed for
> > guest-exec
> > * fix 'make install' when compiling with --disable-tools
> > * fix potential data corruption/loss when accessing files
> > bi-directionally via guest-file-{read,write}
> > * explicitly document how integer args for guest-file-seek map to
> > SEEK_SET/SEEK_CUR/etc to avoid platform-specific differences
> >
> >
> > Eric Blake (1):
> > qga: Better mapping of SEEK_* in guest-file-seek
> >
> > Marc-André Lureau (2):
> > qga: flush explicitly when needed
> > tests: add file-write-read test
>
> A v2 would be wise to ensure we have all the required S-o-b tags (see 3/6)
v2 sent. Not sure why I can't seem to get that patch right. Sorry for the noise.
>
> --
> Eric Blake eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
[Qemu-devel] [PULL v2 for-2.5 5/6] qga: Better mapping of SEEK_* in guest-file-seek
From: Eric Blake
Exposing OS-specific SEEK_ constants in our qapi was a mistake
(if the host has SEEK_CUR as 1, but the guest has it as 2, then
the semantics are unclear what should happen); if we had a time
machine, we would instead expose only a symbolic enum. It's too
late to change the fact that we have an integer in qapi, but we
can at least document what mapping we want to enforce for all
qga clients (and luckily, it happens to be the mapping that both
Linux and Windows use); then fix the code to match that mapping.
It also helps us filter out unsupported SEEK_DATA and SEEK_HOLE.
In the future, we may wish to move our QGA_SEEK_* constants into
qga/qapi-schema.json, along with updating the schema to take an
alternate type (either the integer, or the string value of the
enum name) - but that's too much risk during hard freeze.
Signed-off-by: Eric Blake
Signed-off-by: Michael Roth
---
qga/commands-posix.c | 19 ++-
qga/commands-win32.c | 20 +++-
qga/guest-agent-core.h | 7 +++
qga/qapi-schema.json | 4 ++--
tests/test-qga.c | 5 +++--
5 files changed, 49 insertions(+), 6 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index cf1d7ec..c2ff970 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -553,17 +553,34 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle,
const char *buf_b64,
}
struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
- int64_t whence, Error **errp)
+ int64_t whence_code, Error **errp)
{
GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
GuestFileSeek *seek_data = NULL;
FILE *fh;
int ret;
+int whence;
if (!gfh) {
return NULL;
}
+/* We stupidly exposed 'whence':'int' in our qapi */
+switch (whence_code) {
+case QGA_SEEK_SET:
+whence = SEEK_SET;
+break;
+case QGA_SEEK_CUR:
+whence = SEEK_CUR;
+break;
+case QGA_SEEK_END:
+whence = SEEK_END;
+break;
+default:
+error_setg(errp, "invalid whence code %"PRId64, whence_code);
+return NULL;
+}
+
fh = gfh->fh;
ret = fseek(fh, offset, whence);
if (ret == -1) {
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 41f6dd9..0654fe4 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -382,7 +382,7 @@ done:
}
GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
- int64_t whence, Error **errp)
+ int64_t whence_code, Error **errp)
{
GuestFileHandle *gfh;
GuestFileSeek *seek_data;
@@ -390,11 +390,29 @@ GuestFileSeek *qmp_guest_file_seek(int64_t handle,
int64_t offset,
LARGE_INTEGER new_pos, off_pos;
off_pos.QuadPart = offset;
BOOL res;
+int whence;
+
gfh = guest_file_handle_find(handle, errp);
if (!gfh) {
return NULL;
}
+/* We stupidly exposed 'whence':'int' in our qapi */
+switch (whence_code) {
+case QGA_SEEK_SET:
+whence = SEEK_SET;
+break;
+case QGA_SEEK_CUR:
+whence = SEEK_CUR;
+break;
+case QGA_SEEK_END:
+whence = SEEK_END;
+break;
+default:
+error_setg(errp, "invalid whence code %"PRId64, whence_code);
+return NULL;
+}
+
fh = gfh->fh;
res = SetFilePointerEx(fh, off_pos, &new_pos, whence);
if (!res) {
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index e92c6ab..238dc6b 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -15,6 +15,13 @@
#define QGA_READ_COUNT_DEFAULT 4096
+/* Mapping of whence codes used by guest-file-seek. */
+enum {
+QGA_SEEK_SET = 0,
+QGA_SEEK_CUR = 1,
+QGA_SEEK_END = 2,
+};
+
typedef struct GAState GAState;
typedef struct GACommandState GACommandState;
extern GAState *ga_state;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 78362e0..01c9ee4 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -318,13 +318,13 @@
#
# Seek to a position in the file, as with fseek(), and return the
# current file position afterward. Also encapsulates ftell()'s
-# functionality, just Set offset=0, whence=SEEK_CUR.
+# functionality, with offset=0 and whence=1.
#
# @handle: filehandle returned by guest-file-open
#
# @offset: bytes to skip over in the file stream
#
-# @whence: SEEK_SET, SEEK_CUR, or SEEK_END, as with fseek()
+# @whence: 0 for SEEK_SET, 1 for SEEK_CUR, or 2 for SEEK_END
#
# Returns: @GuestFileSeek on success.
#
diff --git a/tests/test-qga.c b/tests/test-qga.c
index 3b99d9d..e6a84d1 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -13,6 +13,7 @@
#include "libqtest.h"
#include "config-host.h"
+#include "qga/guest-agent-core.h"
typedef struct {
char *test_dir;
@@ -457,7 +458,7 @@ static void test_qga_file_o
[Qemu-devel] [PULL v2 for-2.5 4/6] tests: add file-write-read test
From: Marc-André Lureau
This test exhibits a POSIX behaviour regarding switching between write
and read. It's undefined result if the application doesn't ensure a
flush between the two operations (with glibc, the flush can be implicit
when the buffer size is relatively small). The previous commit fixes
this test.
Related to:
https://bugzilla.redhat.com/show_bug.cgi?id=1210246
Signed-off-by: Marc-André Lureau
Reviewed-by: Eric Blake
Signed-off-by: Michael Roth
---
tests/test-qga.c | 95 ++--
1 file changed, 93 insertions(+), 2 deletions(-)
diff --git a/tests/test-qga.c b/tests/test-qga.c
index 6473846..3b99d9d 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -352,10 +352,10 @@ static void test_qga_network_get_interfaces(gconstpointer
fix)
static void test_qga_file_ops(gconstpointer fix)
{
const TestFixture *fixture = fix;
-const guchar helloworld[] = "Hello World!\n";
+const unsigned char helloworld[] = "Hello World!\n";
const char *b64;
gchar *cmd, *path, *enc;
-guchar *dec;
+unsigned char *dec;
QDict *ret, *val;
int64_t id, eof;
gsize count;
@@ -496,6 +496,96 @@ static void test_qga_file_ops(gconstpointer fix)
g_free(cmd);
}
+static void test_qga_file_write_read(gconstpointer fix)
+{
+const TestFixture *fixture = fix;
+const unsigned char helloworld[] = "Hello World!\n";
+const char *b64;
+gchar *cmd, *enc;
+QDict *ret, *val;
+int64_t id, eof;
+gsize count;
+
+/* open */
+ret = qmp_fd(fixture->fd, "{'execute': 'guest-file-open',"
+ " 'arguments': { 'path': 'foo', 'mode': 'w+' } }");
+g_assert_nonnull(ret);
+qmp_assert_no_error(ret);
+id = qdict_get_int(ret, "return");
+QDECREF(ret);
+
+enc = g_base64_encode(helloworld, sizeof(helloworld));
+/* write */
+cmd = g_strdup_printf("{'execute': 'guest-file-write',"
+ " 'arguments': { 'handle': %" PRId64 ","
+ " 'buf-b64': '%s' } }", id, enc);
+ret = qmp_fd(fixture->fd, cmd);
+g_assert_nonnull(ret);
+qmp_assert_no_error(ret);
+
+val = qdict_get_qdict(ret, "return");
+count = qdict_get_int(val, "count");
+eof = qdict_get_bool(val, "eof");
+g_assert_cmpint(count, ==, sizeof(helloworld));
+g_assert_cmpint(eof, ==, 0);
+QDECREF(ret);
+g_free(cmd);
+
+/* read (check implicit flush) */
+cmd = g_strdup_printf("{'execute': 'guest-file-read',"
+ " 'arguments': { 'handle': %" PRId64 "} }",
+ id);
+ret = qmp_fd(fixture->fd, cmd);
+val = qdict_get_qdict(ret, "return");
+count = qdict_get_int(val, "count");
+eof = qdict_get_bool(val, "eof");
+b64 = qdict_get_str(val, "buf-b64");
+g_assert_cmpint(count, ==, 0);
+g_assert(eof);
+g_assert_cmpstr(b64, ==, "");
+QDECREF(ret);
+g_free(cmd);
+
+/* seek to 0 */
+cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
+ " 'arguments': { 'handle': %" PRId64 ", "
+ " 'offset': %d, 'whence': %d } }",
+ id, 0, SEEK_SET);
+ret = qmp_fd(fixture->fd, cmd);
+qmp_assert_no_error(ret);
+val = qdict_get_qdict(ret, "return");
+count = qdict_get_int(val, "position");
+eof = qdict_get_bool(val, "eof");
+g_assert_cmpint(count, ==, 0);
+g_assert(!eof);
+QDECREF(ret);
+g_free(cmd);
+
+/* read */
+cmd = g_strdup_printf("{'execute': 'guest-file-read',"
+ " 'arguments': { 'handle': %" PRId64 "} }",
+ id);
+ret = qmp_fd(fixture->fd, cmd);
+val = qdict_get_qdict(ret, "return");
+count = qdict_get_int(val, "count");
+eof = qdict_get_bool(val, "eof");
+b64 = qdict_get_str(val, "buf-b64");
+g_assert_cmpint(count, ==, sizeof(helloworld));
+g_assert(eof);
+g_assert_cmpstr(b64, ==, enc);
+QDECREF(ret);
+g_free(cmd);
+g_free(enc);
+
+/* close */
+cmd = g_strdup_printf("{'execute': 'guest-file-close',"
+ " 'arguments': {'handle': %" PRId64 "} }",
+ id);
+ret = qmp_fd(fixture->fd, cmd);
+QDECREF(ret);
+g_free(cmd);
+}
+
static void test_qga_get_time(gconstpointer fix)
{
const TestFixture *fixture = fix;
@@ -762,6 +852,7 @@ int main(int argc, char **argv)
g_test_add_data_func("/qga/get-memory-blocks", &fix,
test_qga_get_memory_blocks);
g_test_add_data_func("/qga/file-ops", &fix, test_qga_file_ops);
+g_test_add_data_func("/qga/file-write-read", &fix,
test_qga_file_write_read);
g_test_add_data_func("/qga/get-time", &fix, test_qga_get_time);
g_test_add_data_func("/qga/invalid-cmd", &fix, test_qga_invalid_cmd);
g_test_add_data_func("/qga/fsfreeze-status", &fix,
--
1.9.1
[Qemu-devel] [PULL v2 for-2.5 3/6] qga: flush explicitly when needed
From: Marc-André Lureau
According to the specification:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html
"the application shall ensure that output is not directly followed by
input without an intervening call to fflush() or to a file positioning
function (fseek(), fsetpos(), or rewind()), and input is not directly
followed by output without an intervening call to a file positioning
function, unless the input operation encounters end-of-file."
Without this change, an fwrite() followed by an fread() may lose the
previously written content, as shown in the following test.
Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1210246
Signed-off-by: Marc-André Lureau
Reviewed-by: Eric Blake
* don't confuse {write,read}() with f{write,read}() in
commit msg (Laszlo)
Signed-off-by: Michael Roth
---
qga/commands-posix.c | 37 +
1 file changed, 37 insertions(+)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 0ebd473..cf1d7ec 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -216,9 +216,16 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns,
Error **errp)
}
}
+typedef enum {
+RW_STATE_NEW,
+RW_STATE_READING,
+RW_STATE_WRITING,
+} RwState;
+
typedef struct GuestFileHandle {
uint64_t id;
FILE *fh;
+RwState state;
QTAILQ_ENTRY(GuestFileHandle) next;
} GuestFileHandle;
@@ -460,6 +467,17 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle,
bool has_count,
}
fh = gfh->fh;
+
+/* explicitly flush when switching from writing to reading */
+if (gfh->state == RW_STATE_WRITING) {
+int ret = fflush(fh);
+if (ret == EOF) {
+error_setg_errno(errp, errno, "failed to flush file");
+return NULL;
+}
+gfh->state = RW_STATE_NEW;
+}
+
buf = g_malloc0(count+1);
read_count = fread(buf, 1, count, fh);
if (ferror(fh)) {
@@ -473,6 +491,7 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle,
bool has_count,
if (read_count) {
read_data->buf_b64 = g_base64_encode(buf, read_count);
}
+gfh->state = RW_STATE_READING;
}
g_free(buf);
clearerr(fh);
@@ -496,6 +515,16 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const
char *buf_b64,
}
fh = gfh->fh;
+
+if (gfh->state == RW_STATE_READING) {
+int ret = fseek(fh, 0, SEEK_CUR);
+if (ret == -1) {
+error_setg_errno(errp, errno, "failed to seek file");
+return NULL;
+}
+gfh->state = RW_STATE_NEW;
+}
+
buf = g_base64_decode(buf_b64, &buf_len);
if (!has_count) {
@@ -515,6 +544,7 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const
char *buf_b64,
write_data = g_new0(GuestFileWrite, 1);
write_data->count = write_count;
write_data->eof = feof(fh);
+gfh->state = RW_STATE_WRITING;
}
g_free(buf);
clearerr(fh);
@@ -538,10 +568,15 @@ struct GuestFileSeek *qmp_guest_file_seek(int64_t handle,
int64_t offset,
ret = fseek(fh, offset, whence);
if (ret == -1) {
error_setg_errno(errp, errno, "failed to seek file");
+if (errno == ESPIPE) {
+/* file is non-seekable, stdio shouldn't be buffering anyways */
+gfh->state = RW_STATE_NEW;
+}
} else {
seek_data = g_new0(GuestFileSeek, 1);
seek_data->position = ftell(fh);
seek_data->eof = feof(fh);
+gfh->state = RW_STATE_NEW;
}
clearerr(fh);
@@ -562,6 +597,8 @@ void qmp_guest_file_flush(int64_t handle, Error **errp)
ret = fflush(fh);
if (ret == EOF) {
error_setg_errno(errp, errno, "failed to flush file");
+} else {
+gfh->state = RW_STATE_NEW;
}
}
--
1.9.1
[Qemu-devel] [PULL v2 for-2.5 6/6] qga: added another non-interactive gspawn() helper file.
From: Yuri Pudgorodskiy With previous commit we added gspawn-win64-helper-console.exe, required for gspawn() mingw implementation. Unfortunatly when running as a service without interactive desktop, gspawn() also requires another helper app. Added gspawn-win64-helper.exe and gspawn-win32-helper.exe for corresponding architectures. Signed-off-by: Yuri Pudgorodskiy Signed-off-by: Denis V. Lunev CC: Michael Roth * remove trailing whitespace Signed-off-by: Michael Roth --- qga/installer/qemu-ga.wxs | 7 +++ 1 file changed, 7 insertions(+) diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs index f25afdd..9473875 100644 --- a/qga/installer/qemu-ga.wxs +++ b/qga/installer/qemu-ga.wxs @@ -95,11 +95,17 @@ + + + + + + @@ -159,6 +165,7 @@ + -- 1.9.1
[Qemu-devel] [PULL v2 for-2.5 2/6] qga: gspawn() console helper to Windows guest agent msi build
From: Yuri Pudgorodskiy This helper, gspawn-win64-helper-console.exe for 64-bit and gspawn-win32-helper-console.exe for 32-bit environment, is needed for gspawn() mingw implementation, used by guest-exec command. Without these files guest-exec command on Windows will not work with "file not found" diagnostic message. Signed-off-by: Yuri Pudgorodskiy Signed-off-by: Denis V. Lunev CC: Michael Roth Signed-off-by: Michael Roth --- qga/installer/qemu-ga.wxs | 11 +++ 1 file changed, 11 insertions(+) diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs index 6804f02..f25afdd 100644 --- a/qga/installer/qemu-ga.wxs +++ b/qga/installer/qemu-ga.wxs @@ -91,6 +91,16 @@ + + + + + + + + + + @@ -148,6 +158,7 @@ + -- 1.9.1
[Qemu-devel] [PULL v2 for-2.5 0/6] qemu-ga patch queue for 2.5
The following changes since commit 1a4dab849d5d06191ab5e5850f6b8bfcad8ceb47:
Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging
(2015-11-25 14:47:06 +)
are available in the git repository at:
git://github.com/mdroth/qemu.git tags/qga-pull-2015-11-25-v2-tag
for you to fetch changes up to 44c6e00c3fd85b9c496bd3e74108ace126813a59:
qga: added another non-interactive gspawn() helper file. (2015-11-25 17:56:45
-0600)
qemu-ga patch queue for 2.5
* include additional w32 MSI install components needed for
guest-exec
* fix 'make install' when compiling with --disable-tools
* fix potential data corruption/loss when accessing files
bi-directionally via guest-file-{read,write}
* explicitly document how integer args for guest-file-seek map to
SEEK_SET/SEEK_CUR/etc to avoid platform-specific differences
v2:
* fixed missing SoB
Eric Blake (1):
qga: Better mapping of SEEK_* in guest-file-seek
Marc-André Lureau (2):
qga: flush explicitly when needed
tests: add file-write-read test
Michael Roth (1):
makefile: fix qemu-ga make install for --disable-tools
Yuri Pudgorodskiy (2):
qga: gspawn() console helper to Windows guest agent msi build
qga: added another non-interactive gspawn() helper file.
Makefile | 5 +--
qga/commands-posix.c | 56 ++-
qga/commands-win32.c | 20 +-
qga/guest-agent-core.h| 7
qga/installer/qemu-ga.wxs | 18 +
qga/qapi-schema.json | 4 +-
tests/test-qga.c | 98 +--
7 files changed, 197 insertions(+), 11 deletions(-)
[Qemu-devel] [PULL v2 for-2.5 1/6] makefile: fix qemu-ga make install for --disable-tools
ab59e3e introduced a fix for `make install` on w32 that involved filtering out qemu-ga from $TOOLS install recipe so that we could append $(EXESUF) to it before attempting to install the binary via install-prog function. install-prog takes a list of binaries to install to a particular directory. If the list is empty it breaks. We guard against this by ensuring $TOOLS is not empty prior to calling. However, ab59e3e introduces extra filtering after this check which can still result on us attempting to call install-prog with an empty list of binaries. In particular, this occurs if we build with the --disable-tools configure option, which results in qemu-ga being the only member of $TOOLS. Fix this by doing a simple s/qemu-ga/qemu-ga$(EXESUF)/ pass through $TOOLS instead of filtering out qemu-ga to handle it seperately. Reported-by: Steve Ellcey Cc: Stefan Weil Signed-off-by: Michael Roth --- Makefile | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Makefile b/Makefile index c7fa427..930ac27 100644 --- a/Makefile +++ b/Makefile @@ -440,10 +440,7 @@ endif install: all $(if $(BUILD_DOCS),install-doc) \ install-datadir install-localstatedir ifneq ($(TOOLS),) - $(call install-prog,$(filter-out qemu-ga,$(TOOLS)),$(DESTDIR)$(bindir)) -ifneq (,$(findstring qemu-ga,$(TOOLS))) - $(call install-prog,qemu-ga$(EXESUF),$(DESTDIR)$(bindir)) -endif + $(call install-prog,$(subst qemu-ga,qemu-ga$(EXESUF),$(TOOLS)),$(DESTDIR)$(bindir)) endif ifneq ($(CONFIG_MODULES),) $(INSTALL_DIR) "$(DESTDIR)$(qemu_moddir)" -- 1.9.1
Re: [Qemu-devel] [PULL for-2.5 0/6] qemu-ga patch queue for 2.5
On 11/25/2015 03:47 PM, Michael Roth wrote:
> The following changes since commit 1a4dab849d5d06191ab5e5850f6b8bfcad8ceb47:
>
> Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging
> (2015-11-25 14:47:06 +)
>
> are available in the git repository at:
>
>
> git://github.com/mdroth/qemu.git tags/qga-pull-2015-11-25-tag
>
> for you to fetch changes up to 35f8c32a200c8133c66642ce0dac721b3480178a:
>
> qga: added another non-interactive gspawn() helper file. (2015-11-25
> 15:34:48 -0600)
>
>
> qemu-ga patch queue for 2.5
>
> * include additional w32 MSI install components needed for
> guest-exec
> * fix 'make install' when compiling with --disable-tools
> * fix potential data corruption/loss when accessing files
> bi-directionally via guest-file-{read,write}
> * explicitly document how integer args for guest-file-seek map to
> SEEK_SET/SEEK_CUR/etc to avoid platform-specific differences
>
>
> Eric Blake (1):
> qga: Better mapping of SEEK_* in guest-file-seek
>
> Marc-André Lureau (2):
> qga: flush explicitly when needed
> tests: add file-write-read test
A v2 would be wise to ensure we have all the required S-o-b tags (see 3/6)
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PULL for-2.5 3/6] qga: flush explicitly when needed
On 11/25/2015 03:47 PM, Michael Roth wrote:
> From: Marc-André Lureau
>
> According to the specification:
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html
>
> "the application shall ensure that output is not directly followed by
> input without an intervening call to fflush() or to a file positioning
> function (fseek(), fsetpos(), or rewind()), and input is not directly
> followed by output without an intervening call to a file positioning
> function, unless the input operation encounters end-of-file."
>
> Without this change, an fwrite() followed by an fread() may lose the
> previously written content, as shown in the following test.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1210246
>
> Reviewed-by: Eric Blake
> * don't confuse {write,read}() with f{write,read}() in
> commit msg (Laszlo)
> Signed-off-by: Michael Roth
> ---
Looks like you lost Marc-Andre's S-o-b :(
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PULL for-2.5 6/6] qga: added another non-interactive gspawn() helper file.
From: Yuri Pudgorodskiy With previous commit we added gspawn-win64-helper-console.exe, required for gspawn() mingw implementation. Unfortunatly when running as a service without interactive desktop, gspawn() also requires another helper app. Added gspawn-win64-helper.exe and gspawn-win32-helper.exe for corresponding architectures. Signed-off-by: Yuri Pudgorodskiy Signed-off-by: Denis V. Lunev CC: Michael Roth * remove trailing whitespace Signed-off-by: Michael Roth --- qga/installer/qemu-ga.wxs | 7 +++ 1 file changed, 7 insertions(+) diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs index f25afdd..9473875 100644 --- a/qga/installer/qemu-ga.wxs +++ b/qga/installer/qemu-ga.wxs @@ -95,11 +95,17 @@ + + + + + + @@ -159,6 +165,7 @@ + -- 1.9.1
[Qemu-devel] [PULL for-2.5 5/6] qga: Better mapping of SEEK_* in guest-file-seek
From: Eric Blake
Exposing OS-specific SEEK_ constants in our qapi was a mistake
(if the host has SEEK_CUR as 1, but the guest has it as 2, then
the semantics are unclear what should happen); if we had a time
machine, we would instead expose only a symbolic enum. It's too
late to change the fact that we have an integer in qapi, but we
can at least document what mapping we want to enforce for all
qga clients (and luckily, it happens to be the mapping that both
Linux and Windows use); then fix the code to match that mapping.
It also helps us filter out unsupported SEEK_DATA and SEEK_HOLE.
In the future, we may wish to move our QGA_SEEK_* constants into
qga/qapi-schema.json, along with updating the schema to take an
alternate type (either the integer, or the string value of the
enum name) - but that's too much risk during hard freeze.
Signed-off-by: Eric Blake
Signed-off-by: Michael Roth
---
qga/commands-posix.c | 19 ++-
qga/commands-win32.c | 20 +++-
qga/guest-agent-core.h | 7 +++
qga/qapi-schema.json | 4 ++--
tests/test-qga.c | 5 +++--
5 files changed, 49 insertions(+), 6 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index cf1d7ec..c2ff970 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -553,17 +553,34 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle,
const char *buf_b64,
}
struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
- int64_t whence, Error **errp)
+ int64_t whence_code, Error **errp)
{
GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
GuestFileSeek *seek_data = NULL;
FILE *fh;
int ret;
+int whence;
if (!gfh) {
return NULL;
}
+/* We stupidly exposed 'whence':'int' in our qapi */
+switch (whence_code) {
+case QGA_SEEK_SET:
+whence = SEEK_SET;
+break;
+case QGA_SEEK_CUR:
+whence = SEEK_CUR;
+break;
+case QGA_SEEK_END:
+whence = SEEK_END;
+break;
+default:
+error_setg(errp, "invalid whence code %"PRId64, whence_code);
+return NULL;
+}
+
fh = gfh->fh;
ret = fseek(fh, offset, whence);
if (ret == -1) {
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 41f6dd9..0654fe4 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -382,7 +382,7 @@ done:
}
GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
- int64_t whence, Error **errp)
+ int64_t whence_code, Error **errp)
{
GuestFileHandle *gfh;
GuestFileSeek *seek_data;
@@ -390,11 +390,29 @@ GuestFileSeek *qmp_guest_file_seek(int64_t handle,
int64_t offset,
LARGE_INTEGER new_pos, off_pos;
off_pos.QuadPart = offset;
BOOL res;
+int whence;
+
gfh = guest_file_handle_find(handle, errp);
if (!gfh) {
return NULL;
}
+/* We stupidly exposed 'whence':'int' in our qapi */
+switch (whence_code) {
+case QGA_SEEK_SET:
+whence = SEEK_SET;
+break;
+case QGA_SEEK_CUR:
+whence = SEEK_CUR;
+break;
+case QGA_SEEK_END:
+whence = SEEK_END;
+break;
+default:
+error_setg(errp, "invalid whence code %"PRId64, whence_code);
+return NULL;
+}
+
fh = gfh->fh;
res = SetFilePointerEx(fh, off_pos, &new_pos, whence);
if (!res) {
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index e92c6ab..238dc6b 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -15,6 +15,13 @@
#define QGA_READ_COUNT_DEFAULT 4096
+/* Mapping of whence codes used by guest-file-seek. */
+enum {
+QGA_SEEK_SET = 0,
+QGA_SEEK_CUR = 1,
+QGA_SEEK_END = 2,
+};
+
typedef struct GAState GAState;
typedef struct GACommandState GACommandState;
extern GAState *ga_state;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 78362e0..01c9ee4 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -318,13 +318,13 @@
#
# Seek to a position in the file, as with fseek(), and return the
# current file position afterward. Also encapsulates ftell()'s
-# functionality, just Set offset=0, whence=SEEK_CUR.
+# functionality, with offset=0 and whence=1.
#
# @handle: filehandle returned by guest-file-open
#
# @offset: bytes to skip over in the file stream
#
-# @whence: SEEK_SET, SEEK_CUR, or SEEK_END, as with fseek()
+# @whence: 0 for SEEK_SET, 1 for SEEK_CUR, or 2 for SEEK_END
#
# Returns: @GuestFileSeek on success.
#
diff --git a/tests/test-qga.c b/tests/test-qga.c
index 3b99d9d..e6a84d1 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -13,6 +13,7 @@
#include "libqtest.h"
#include "config-host.h"
+#include "qga/guest-agent-core.h"
typedef struct {
char *test_dir;
@@ -457,7 +458,7 @@ static void test_qga_file_o
[Qemu-devel] [PULL for-2.5 4/6] tests: add file-write-read test
From: Marc-André Lureau
This test exhibits a POSIX behaviour regarding switching between write
and read. It's undefined result if the application doesn't ensure a
flush between the two operations (with glibc, the flush can be implicit
when the buffer size is relatively small). The previous commit fixes
this test.
Related to:
https://bugzilla.redhat.com/show_bug.cgi?id=1210246
Signed-off-by: Marc-André Lureau
Reviewed-by: Eric Blake
Signed-off-by: Michael Roth
---
tests/test-qga.c | 95 ++--
1 file changed, 93 insertions(+), 2 deletions(-)
diff --git a/tests/test-qga.c b/tests/test-qga.c
index 6473846..3b99d9d 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -352,10 +352,10 @@ static void test_qga_network_get_interfaces(gconstpointer
fix)
static void test_qga_file_ops(gconstpointer fix)
{
const TestFixture *fixture = fix;
-const guchar helloworld[] = "Hello World!\n";
+const unsigned char helloworld[] = "Hello World!\n";
const char *b64;
gchar *cmd, *path, *enc;
-guchar *dec;
+unsigned char *dec;
QDict *ret, *val;
int64_t id, eof;
gsize count;
@@ -496,6 +496,96 @@ static void test_qga_file_ops(gconstpointer fix)
g_free(cmd);
}
+static void test_qga_file_write_read(gconstpointer fix)
+{
+const TestFixture *fixture = fix;
+const unsigned char helloworld[] = "Hello World!\n";
+const char *b64;
+gchar *cmd, *enc;
+QDict *ret, *val;
+int64_t id, eof;
+gsize count;
+
+/* open */
+ret = qmp_fd(fixture->fd, "{'execute': 'guest-file-open',"
+ " 'arguments': { 'path': 'foo', 'mode': 'w+' } }");
+g_assert_nonnull(ret);
+qmp_assert_no_error(ret);
+id = qdict_get_int(ret, "return");
+QDECREF(ret);
+
+enc = g_base64_encode(helloworld, sizeof(helloworld));
+/* write */
+cmd = g_strdup_printf("{'execute': 'guest-file-write',"
+ " 'arguments': { 'handle': %" PRId64 ","
+ " 'buf-b64': '%s' } }", id, enc);
+ret = qmp_fd(fixture->fd, cmd);
+g_assert_nonnull(ret);
+qmp_assert_no_error(ret);
+
+val = qdict_get_qdict(ret, "return");
+count = qdict_get_int(val, "count");
+eof = qdict_get_bool(val, "eof");
+g_assert_cmpint(count, ==, sizeof(helloworld));
+g_assert_cmpint(eof, ==, 0);
+QDECREF(ret);
+g_free(cmd);
+
+/* read (check implicit flush) */
+cmd = g_strdup_printf("{'execute': 'guest-file-read',"
+ " 'arguments': { 'handle': %" PRId64 "} }",
+ id);
+ret = qmp_fd(fixture->fd, cmd);
+val = qdict_get_qdict(ret, "return");
+count = qdict_get_int(val, "count");
+eof = qdict_get_bool(val, "eof");
+b64 = qdict_get_str(val, "buf-b64");
+g_assert_cmpint(count, ==, 0);
+g_assert(eof);
+g_assert_cmpstr(b64, ==, "");
+QDECREF(ret);
+g_free(cmd);
+
+/* seek to 0 */
+cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
+ " 'arguments': { 'handle': %" PRId64 ", "
+ " 'offset': %d, 'whence': %d } }",
+ id, 0, SEEK_SET);
+ret = qmp_fd(fixture->fd, cmd);
+qmp_assert_no_error(ret);
+val = qdict_get_qdict(ret, "return");
+count = qdict_get_int(val, "position");
+eof = qdict_get_bool(val, "eof");
+g_assert_cmpint(count, ==, 0);
+g_assert(!eof);
+QDECREF(ret);
+g_free(cmd);
+
+/* read */
+cmd = g_strdup_printf("{'execute': 'guest-file-read',"
+ " 'arguments': { 'handle': %" PRId64 "} }",
+ id);
+ret = qmp_fd(fixture->fd, cmd);
+val = qdict_get_qdict(ret, "return");
+count = qdict_get_int(val, "count");
+eof = qdict_get_bool(val, "eof");
+b64 = qdict_get_str(val, "buf-b64");
+g_assert_cmpint(count, ==, sizeof(helloworld));
+g_assert(eof);
+g_assert_cmpstr(b64, ==, enc);
+QDECREF(ret);
+g_free(cmd);
+g_free(enc);
+
+/* close */
+cmd = g_strdup_printf("{'execute': 'guest-file-close',"
+ " 'arguments': {'handle': %" PRId64 "} }",
+ id);
+ret = qmp_fd(fixture->fd, cmd);
+QDECREF(ret);
+g_free(cmd);
+}
+
static void test_qga_get_time(gconstpointer fix)
{
const TestFixture *fixture = fix;
@@ -762,6 +852,7 @@ int main(int argc, char **argv)
g_test_add_data_func("/qga/get-memory-blocks", &fix,
test_qga_get_memory_blocks);
g_test_add_data_func("/qga/file-ops", &fix, test_qga_file_ops);
+g_test_add_data_func("/qga/file-write-read", &fix,
test_qga_file_write_read);
g_test_add_data_func("/qga/get-time", &fix, test_qga_get_time);
g_test_add_data_func("/qga/invalid-cmd", &fix, test_qga_invalid_cmd);
g_test_add_data_func("/qga/fsfreeze-status", &fix,
--
1.9.1
[Qemu-devel] [PULL for-2.5 3/6] qga: flush explicitly when needed
From: Marc-André Lureau
According to the specification:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html
"the application shall ensure that output is not directly followed by
input without an intervening call to fflush() or to a file positioning
function (fseek(), fsetpos(), or rewind()), and input is not directly
followed by output without an intervening call to a file positioning
function, unless the input operation encounters end-of-file."
Without this change, an fwrite() followed by an fread() may lose the
previously written content, as shown in the following test.
Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1210246
Reviewed-by: Eric Blake
* don't confuse {write,read}() with f{write,read}() in
commit msg (Laszlo)
Signed-off-by: Michael Roth
---
qga/commands-posix.c | 37 +
1 file changed, 37 insertions(+)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 0ebd473..cf1d7ec 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -216,9 +216,16 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns,
Error **errp)
}
}
+typedef enum {
+RW_STATE_NEW,
+RW_STATE_READING,
+RW_STATE_WRITING,
+} RwState;
+
typedef struct GuestFileHandle {
uint64_t id;
FILE *fh;
+RwState state;
QTAILQ_ENTRY(GuestFileHandle) next;
} GuestFileHandle;
@@ -460,6 +467,17 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle,
bool has_count,
}
fh = gfh->fh;
+
+/* explicitly flush when switching from writing to reading */
+if (gfh->state == RW_STATE_WRITING) {
+int ret = fflush(fh);
+if (ret == EOF) {
+error_setg_errno(errp, errno, "failed to flush file");
+return NULL;
+}
+gfh->state = RW_STATE_NEW;
+}
+
buf = g_malloc0(count+1);
read_count = fread(buf, 1, count, fh);
if (ferror(fh)) {
@@ -473,6 +491,7 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle,
bool has_count,
if (read_count) {
read_data->buf_b64 = g_base64_encode(buf, read_count);
}
+gfh->state = RW_STATE_READING;
}
g_free(buf);
clearerr(fh);
@@ -496,6 +515,16 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const
char *buf_b64,
}
fh = gfh->fh;
+
+if (gfh->state == RW_STATE_READING) {
+int ret = fseek(fh, 0, SEEK_CUR);
+if (ret == -1) {
+error_setg_errno(errp, errno, "failed to seek file");
+return NULL;
+}
+gfh->state = RW_STATE_NEW;
+}
+
buf = g_base64_decode(buf_b64, &buf_len);
if (!has_count) {
@@ -515,6 +544,7 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const
char *buf_b64,
write_data = g_new0(GuestFileWrite, 1);
write_data->count = write_count;
write_data->eof = feof(fh);
+gfh->state = RW_STATE_WRITING;
}
g_free(buf);
clearerr(fh);
@@ -538,10 +568,15 @@ struct GuestFileSeek *qmp_guest_file_seek(int64_t handle,
int64_t offset,
ret = fseek(fh, offset, whence);
if (ret == -1) {
error_setg_errno(errp, errno, "failed to seek file");
+if (errno == ESPIPE) {
+/* file is non-seekable, stdio shouldn't be buffering anyways */
+gfh->state = RW_STATE_NEW;
+}
} else {
seek_data = g_new0(GuestFileSeek, 1);
seek_data->position = ftell(fh);
seek_data->eof = feof(fh);
+gfh->state = RW_STATE_NEW;
}
clearerr(fh);
@@ -562,6 +597,8 @@ void qmp_guest_file_flush(int64_t handle, Error **errp)
ret = fflush(fh);
if (ret == EOF) {
error_setg_errno(errp, errno, "failed to flush file");
+} else {
+gfh->state = RW_STATE_NEW;
}
}
--
1.9.1
[Qemu-devel] [PULL for-2.5 2/6] qga: gspawn() console helper to Windows guest agent msi build
From: Yuri Pudgorodskiy This helper, gspawn-win64-helper-console.exe for 64-bit and gspawn-win32-helper-console.exe for 32-bit environment, is needed for gspawn() mingw implementation, used by guest-exec command. Without these files guest-exec command on Windows will not work with "file not found" diagnostic message. Signed-off-by: Yuri Pudgorodskiy Signed-off-by: Denis V. Lunev CC: Michael Roth Signed-off-by: Michael Roth --- qga/installer/qemu-ga.wxs | 11 +++ 1 file changed, 11 insertions(+) diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs index 6804f02..f25afdd 100644 --- a/qga/installer/qemu-ga.wxs +++ b/qga/installer/qemu-ga.wxs @@ -91,6 +91,16 @@ + + + + + + + + + + @@ -148,6 +158,7 @@ + -- 1.9.1
[Qemu-devel] [PULL for-2.5 0/6] qemu-ga patch queue for 2.5
The following changes since commit 1a4dab849d5d06191ab5e5850f6b8bfcad8ceb47:
Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging
(2015-11-25 14:47:06 +)
are available in the git repository at:
git://github.com/mdroth/qemu.git tags/qga-pull-2015-11-25-tag
for you to fetch changes up to 35f8c32a200c8133c66642ce0dac721b3480178a:
qga: added another non-interactive gspawn() helper file. (2015-11-25 15:34:48
-0600)
qemu-ga patch queue for 2.5
* include additional w32 MSI install components needed for
guest-exec
* fix 'make install' when compiling with --disable-tools
* fix potential data corruption/loss when accessing files
bi-directionally via guest-file-{read,write}
* explicitly document how integer args for guest-file-seek map to
SEEK_SET/SEEK_CUR/etc to avoid platform-specific differences
Eric Blake (1):
qga: Better mapping of SEEK_* in guest-file-seek
Marc-André Lureau (2):
qga: flush explicitly when needed
tests: add file-write-read test
Michael Roth (1):
makefile: fix qemu-ga make install for --disable-tools
Yuri Pudgorodskiy (2):
qga: gspawn() console helper to Windows guest agent msi build
qga: added another non-interactive gspawn() helper file.
Makefile | 5 +--
qga/commands-posix.c | 56 ++-
qga/commands-win32.c | 20 +-
qga/guest-agent-core.h| 7
qga/installer/qemu-ga.wxs | 18 +
qga/qapi-schema.json | 4 +-
tests/test-qga.c | 98 +--
7 files changed, 197 insertions(+), 11 deletions(-)
[Qemu-devel] [PULL for-2.5 1/6] makefile: fix qemu-ga make install for --disable-tools
ab59e3e introduced a fix for `make install` on w32 that involved filtering out qemu-ga from $TOOLS install recipe so that we could append $(EXESUF) to it before attempting to install the binary via install-prog function. install-prog takes a list of binaries to install to a particular directory. If the list is empty it breaks. We guard against this by ensuring $TOOLS is not empty prior to calling. However, ab59e3e introduces extra filtering after this check which can still result on us attempting to call install-prog with an empty list of binaries. In particular, this occurs if we build with the --disable-tools configure option, which results in qemu-ga being the only member of $TOOLS. Fix this by doing a simple s/qemu-ga/qemu-ga$(EXESUF)/ pass through $TOOLS instead of filtering out qemu-ga to handle it seperately. Reported-by: Steve Ellcey Cc: Stefan Weil Signed-off-by: Michael Roth --- Makefile | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Makefile b/Makefile index c7fa427..930ac27 100644 --- a/Makefile +++ b/Makefile @@ -440,10 +440,7 @@ endif install: all $(if $(BUILD_DOCS),install-doc) \ install-datadir install-localstatedir ifneq ($(TOOLS),) - $(call install-prog,$(filter-out qemu-ga,$(TOOLS)),$(DESTDIR)$(bindir)) -ifneq (,$(findstring qemu-ga,$(TOOLS))) - $(call install-prog,qemu-ga$(EXESUF),$(DESTDIR)$(bindir)) -endif + $(call install-prog,$(subst qemu-ga,qemu-ga$(EXESUF),$(TOOLS)),$(DESTDIR)$(bindir)) endif ifneq ($(CONFIG_MODULES),) $(INSTALL_DIR) "$(DESTDIR)$(qemu_moddir)" -- 1.9.1
Re: [Qemu-devel] [PATCH v3 for-2.5 11/12] qjson: surprise, allocating 6 QObjects per token is expensive
On 11/25/2015 02:23 PM, Markus Armbruster wrote: > From: Paolo Bonzini > > Replace the contents of the tokens GQueue with a simple struct. This cuts > the amount of memory allocated by tests/check-qjson from ~500MB to ~20MB, > and the execution time from 600ms to 80ms on my laptop. Still a lot (some > could be saved by using an intrusive list, such as QSIMPLEQ, instead of > the GQueue), but the savings are already massive and the right thing to > do would probably be to get rid of json-streamer completely. > > Signed-off-by: Paolo Bonzini > Message-Id: <1448300659-23559-5-git-send-email-pbonz...@redhat.com> > [Straightforwardly rebased on my patches] > Signed-off-by: Markus Armbruster > --- > include/qapi/qmp/json-streamer.h | 7 +++ > qobject/json-parser.c| 115 > --- > qobject/json-streamer.c | 19 +++ > 3 files changed, 63 insertions(+), 78 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 for-2.5 10/12] qjson: store tokens in a GQueue
On 11/25/2015 02:23 PM, Markus Armbruster wrote: > From: Paolo Bonzini > > Even though we still have the "streamer" concept, the tokens can now > be deleted as they are read. While doing so convert from QList to > GQueue, since the next step will make tokens not a QObject and we > will have to do the conversion anyway. > > Signed-off-by: Paolo Bonzini > Message-Id: <1448300659-23559-4-git-send-email-pbonz...@redhat.com> > Signed-off-by: Markus Armbruster > --- Reviewed-by: Eric Blake -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 for-2.5 09/12] qjson: Convert to parser to recursive descent
On 11/25/2015 02:23 PM, Markus Armbruster wrote:
> We backtrack in parse_value(), even though JSON is LL(1) and thus can
> be parsed by straightforward recursive descent. Do exactly that.
>
> Based on an almost-correct patch from Paolo Bonzini.
>
> Signed-off-by: Markus Armbruster
> ---
> qobject/json-parser.c | 165
> ++
> 1 file changed, 47 insertions(+), 118 deletions(-)
>
> static QObject *parse_value(JSONParserContext *ctxt, va_list *ap)
> {
> -QObject *obj;
> +QObject *token;
>
> -obj = parse_object(ctxt, ap);
> -if (obj == NULL) {
> -obj = parse_array(ctxt, ap);
> -}
> -if (obj == NULL) {
> -obj = parse_escape(ctxt, ap);
> -}
> -if (obj == NULL) {
> -obj = parse_keyword(ctxt);
> -}
> -if (obj == NULL) {
> -obj = parse_literal(ctxt);
> +token = parser_context_peek_token(ctxt);
> +if (token == NULL) {
> +parse_error(ctxt, NULL, "premature EOI");
Should we spell that out as 'end of input'?
But that's cosmetic, and doesn't affect correctness of the conversion.
Reviewed-by: Eric Blake
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH for 2.5 1/1] qga: added another non-interactive gspawn() helper file.
Quoting Denis V. Lunev (2015-11-25 13:02:26)
> From: Yuri Pudgorodskiy
>
> With previous commit we added gspawn-win64-helper-console.exe,
> required for gspawn() mingw implementation.
> Unfortunatly when running as a service without interactive
> desktop, gspawn() also requires another helper app.
>
> Added gspawn-win64-helper.exe and gspawn-win32-helper.exe
> for corresponding architectures.
>
> Signed-off-by: Yuri Pudgorodskiy
> Signed-off-by: Denis V. Lunev
> CC: Michael Roth
Thanks, applied to qga tree with minor whitespace fixup:
https://github.com/mdroth/qemu/commits/qga
I noticed something testing this though: if we run qemu-ga
from a console, then exec something like ipconfig with
capture-output: true, qemu-ga returns that output via
guest-exec-status.
If we run it as a service however, there's no output.
# with qemu-ga started via console
{'execute':'guest-exec','arguments':{'path':'/Windows/System32/ipconfig.exe',
'capture-output':true}}
{"return": {"pid": 1644}}
{'execute':'guest-exec-status','arguments':{'pid':1644}}
{"return": {"exitcode": 0, "out-data":
"DQpXaW5kb3dzIElQIENvbmZpZ3VyYXRpb24NCg0KDQpFdGhlcm5ldCBhZGFwdGVyIExvY2FsIEFyZWEgQ29ubmVjdGlvbiAyOg0KDQogICBDb25uZWN0aW9uLXNwZWNpZmljIEROUyBTdWZmaXggIC4gOiANCiAgIExpbmstbG9jYWwgSVB2NiBBZGRyZXNzIC4gLiAuIC4gLiA6IGZlODA6OjMwMDU6NmRjOjNjNmE6NTQ2NCUxNA0KICAgSVB2NCBBZGRyZXNzLiAuIC4gLiAuIC4gLiAuIC4gLiAuIDogMTkyLjE2OC4xMjIuMTQNCiAgIFN1Ym5ldCBNYXNrIC4gLiAuIC4gLiAuIC4gLiAuIC4gLiA6IDI1NS4yNTUuMjU1LjANCiAgIERlZmF1bHQgR2F0ZXdheSAuIC4gLiAuIC4gLiAuIC4gLiA6IDE5Mi4xNjguMTIyLjENCg0KVHVubmVsIGFkYXB0ZXIgaXNhdGFwLns3Q0Q3OTAwQy05NThCLTRBRUMtQkUwRC0yMTNERjM1NjQ2MEZ9Og0KDQogICBNZWRpYSBTdGF0ZSAuIC4gLiAuIC4gLiAuIC4gLiAuIC4gOiBNZWRpYSBkaXNjb25uZWN0ZWQNCiAgIENvbm5lY3Rpb24tc3BlY2lmaWMgRE5TIFN1ZmZpeCAgLiA6IA0KDQpUdW5uZWwgYWRhcHRlciBMb2NhbCBBcmVhIENvbm5lY3Rpb24qIDExOg0KDQogICBNZWRpYSBTdGF0ZSAuIC4gLiAuIC4gLiAuIC4gLiAuIC4gOiBNZWRpYSBkaXNjb25uZWN0ZWQNCiAgIENvbm5lY3Rpb24tc3BlY2lmaWMgRE5TIFN1ZmZpeCAgLiA6IA0K",
"exited": true}}
# wtih qemu-ga started via windows service
{'execute':'guest-exec','arguments':{'path':'/Windows/System32/ipconfig.exe',
'capture-output':true}}
{"return": {"pid": 1176}}
{'execute':'guest-exec-status','arguments':{'pid':1176}}
{"return": {"exitcode": 0, "exited": true}}
Is this expected?
> ---
> qga/installer/qemu-ga.wxs | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
> index f25afdd..7c59972 100644
> --- a/qga/installer/qemu-ga.wxs
> +++ b/qga/installer/qemu-ga.wxs
> @@ -95,11 +95,17 @@
> Guid="{446185B3-87BE-43D2-96B8-0FEFD9E8696D}">
> Name="gspawn-win32-helper-console.exe"
> Source="$(var.Mingw_bin)/gspawn-win32-helper-console.exe" KeyPath="yes"
> DiskId="1"/>
>
> + Guid="{CD67A5A3-2DB1-4DA1-A67A-8D71E797B466}">
> + Name="gspawn-win32-helper.exe"
> Source="$(var.Mingw_bin)/gspawn-win32-helper.exe" KeyPath="yes" DiskId="1"/>
> +
>
>
> Guid="{9E615A9F-349A-4992-A5C2-C10BAD173660}">
> Name="gspawn-win64-helper-console.exe"
> Source="$(var.Mingw_bin)/gspawn-win64-helper-console.exe" KeyPath="yes"
> DiskId="1"/>
>
> + Guid="{D201AD22-1846-4E4F-B6E1-C7A908ED2457}">
> + Name="gspawn-win64-helper.exe"
> Source="$(var.Mingw_bin)/gspawn-win64-helper.exe" KeyPath="yes" DiskId="1"/>
> +
>
> Guid="{35EE3558-D34B-4F0A-B8BD-430FF0775246}">
> Source="$(var.Mingw_bin)/iconv.dll" KeyPath="yes" DiskId="1"/>
> @@ -159,6 +165,7 @@
>
>
>
> +
>
>
>
> --
> 2.1.4
>
