Re: [Qemu-devel] [PULL 00/33] virtio, pc: fixes and features
Dear Peter, Peter Maydell <peter.mayd...@linaro.org> writes: > On 10 October 2016 at 15:13, Peter Maydell <peter.mayd...@linaro.org> wrote: >> This failed 'make check' on aarch64 host (everything else was ok): >> >> TEST: tests/pxe-test... (pid=11699) >> /ppc64/pxe/virtio: ** >> ERROR:/home/petmay01/qemu/tests/boot-sector.c:120:boot_sector_test: >> assertion failed (signature == SIGN >> ATURE): (0x2020 == 0xdead) >> FAIL >> GTester: last random seed: R02S87a02de849c98998299177b1a4c7a31b >> (pid=19477) >> /ppc64/pxe/spapr-vlan: ** >> ERROR:/home/petmay01/qemu/tests/boot-sector.c:120:boot_sector_test: >> assertion failed (signature == SIGN >> ATURE): (0x2020 == 0xdead) >> FAIL >> GTester: last random seed: R02Sf9cf55ad239a137dd20d3085abf91524 >> (pid=24055) >> FAIL: tests/pxe-test > > Several subsequent test runs passed, so I'm inclined to suspect > this is not related to the pull request but is actually an > over-enthusiastic timeout and the build machine was heavily > loaded or something. There's a race condition in the tests. Both the "i386" and the "x86_64" set of tests are creating and removing the same set of files: tests/acpi-test-disk.raw (hard-coded in tests/bios-tables-test.c) and tests/pxe-test-disk.raw (hard-coded in tests/pxe-test.c). FWIW, it's not obvious at first sight that the tests added to check-qtest-i386-y will also be included in check-qtest-x86_64-y. The line responsible for that is hiding in the midst of other assignments, without even a blank line separating it from the rest. Assigning to a common variable first and then including it in both check-qtest-i386-y and check-qtest-x86_64-y would make it a lot easier to understand IMO. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr.: DE281696641 signature.asc Description: PGP signature
[Qemu-devel] [PATCH v2 0/2] virtio-serial: virtio console emergency write support
This series adds support for the virtio 1.0 "emergency write" (VIRTIO_CONSOLE_F_EMERG_WRITE) virtio feature. This is useful for early guest debugging and might be used in cases where the guest crashes so badly that it cannot use virtqueues. v1→v2: - rebased on current master (resolved conflict: virtio-pci page-per-vq compat property) - split into two patches; no code changes (requested by Amit) Sascha Silbe (2): virtio-serial: add plumbing for virtio console emergency write support virtio-serial: enable virtio console emergency write feature hw/char/virtio-serial-bus.c | 49 --- include/hw/compat.h | 5 include/hw/virtio/virtio-serial.h | 2 ++ 3 files changed, 53 insertions(+), 3 deletions(-) -- 1.9.1
[Qemu-devel] [PATCH v2 2/2] virtio-serial: enable virtio console emergency write feature
Add support for enabling the virtio 1.0 "emergency write"
(VIRTIO_CONSOLE_F_EMERG_WRITE) feature. The previous patch introduced
the plumbing required for this; now we expose the virtio feature to
the guest. The feature is disabled for compatibility machines to avoid
exposing a new feature to existing guests.
As required by the virtio 1.0 spec, the emergency write functionality
is available to the guest even if the guest doesn't negotatiate the
feature, as well as before feature negotation.
Reviewed-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
v1→v2:
- rebased on current master (virtio-pci page-per-vq compat property)
- split into two patches; no code changes (requested by Amit)
hw/char/virtio-serial-bus.c | 12 +---
include/hw/compat.h | 5 +
include/hw/virtio/virtio-serial.h | 2 ++
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 57419b2..db2a9f1 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -541,6 +541,7 @@ static uint64_t get_features(VirtIODevice *vdev, uint64_t
features,
vser = VIRTIO_SERIAL(vdev);
+features |= vser->host_features;
if (vser->bus.max_nr_ports > 1) {
virtio_add_feature(, VIRTIO_CONSOLE_F_MULTIPORT);
}
@@ -1003,6 +1004,7 @@ static void virtio_serial_device_realize(DeviceState
*dev, Error **errp)
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
VirtIOSerial *vser = VIRTIO_SERIAL(dev);
uint32_t i, max_supported_ports;
+size_t config_size = sizeof(struct virtio_console_config);
if (!vser->serial.max_virtserial_ports) {
error_setg(errp, "Maximum number of serial ports not specified");
@@ -1017,10 +1019,12 @@ static void virtio_serial_device_realize(DeviceState
*dev, Error **errp)
return;
}
-/* We don't support emergency write, skip it for now. */
-/* TODO: cleaner fix, depending on host features. */
+if (!virtio_has_feature(vser->host_features,
+VIRTIO_CONSOLE_F_EMERG_WRITE)) {
+config_size = offsetof(struct virtio_console_config, emerg_wr);
+}
virtio_init(vdev, "virtio-serial", VIRTIO_ID_CONSOLE,
-offsetof(struct virtio_console_config, emerg_wr));
+config_size);
/* Spawn a new virtio-serial bus on which the ports will ride as devices */
qbus_create_inplace(>bus, sizeof(vser->bus), TYPE_VIRTIO_SERIAL_BUS,
@@ -1116,6 +1120,8 @@ VMSTATE_VIRTIO_DEVICE(console, 3, virtio_serial_load,
virtio_vmstate_save);
static Property virtio_serial_properties[] = {
DEFINE_PROP_UINT32("max_ports", VirtIOSerial, serial.max_virtserial_ports,
31),
+DEFINE_PROP_BIT64("emergency-write", VirtIOSerial, host_features,
+ VIRTIO_CONSOLE_F_EMERG_WRITE, true),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/hw/compat.h b/include/hw/compat.h
index a1d6694..552030a 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -6,6 +6,11 @@
.driver = "virtio-pci",\
.property = "page-per-vq",\
.value= "on",\
+},\
+{\
+.driver = "virtio-serial-device",\
+.property = "emergency-write",\
+.value= "off",\
},
#define HW_COMPAT_2_6 \
diff --git a/include/hw/virtio/virtio-serial.h
b/include/hw/virtio/virtio-serial.h
index 730c88d..b19c447 100644
--- a/include/hw/virtio/virtio-serial.h
+++ b/include/hw/virtio/virtio-serial.h
@@ -184,6 +184,8 @@ struct VirtIOSerial {
struct VirtIOSerialPostLoad *post_load;
virtio_serial_conf serial;
+
+uint64_t host_features;
};
/* Interface to the virtio-serial bus */
--
1.9.1
[Qemu-devel] [PATCH v2 1/2] virtio-serial: add plumbing for virtio console emergency write support
Add the infrastructure required for the virtio 1.0 "emergency write"
(VIRTIO_CONSOLE_F_EMERG_WRITE) feature. Because we don't touch the
size of the configuration area, guests will not be able to actually
make use of this without further patches.
Reviewed-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
v1→v2:
- rebased on current master (virtio-pci page-per-vq compat property)
- split into two patches; no code changes (requested by Amit)
hw/char/virtio-serial-bus.c | 37 +
1 file changed, 37 insertions(+)
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index db57a38..57419b2 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -75,6 +75,19 @@ static VirtIOSerialPort *find_port_by_name(char *name)
return NULL;
}
+static VirtIOSerialPort *find_first_connected_console(VirtIOSerial *vser)
+{
+VirtIOSerialPort *port;
+
+QTAILQ_FOREACH(port, >ports, next) {
+VirtIOSerialPortClass const *vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+if (vsc->is_console && port->host_connected) {
+return port;
+}
+}
+return NULL;
+}
+
static bool use_multiport(VirtIOSerial *vser)
{
VirtIODevice *vdev = VIRTIO_DEVICE(vser);
@@ -547,6 +560,29 @@ static void get_config(VirtIODevice *vdev, uint8_t
*config_data)
vser->serial.max_virtserial_ports);
}
+/* Guest sent new config info */
+static void set_config(VirtIODevice *vdev, const uint8_t *config_data)
+{
+VirtIOSerial *vser = VIRTIO_SERIAL(vdev);
+struct virtio_console_config *config =
+(struct virtio_console_config *)config_data;
+uint8_t emerg_wr_lo = le32_to_cpu(config->emerg_wr);
+VirtIOSerialPort *port = find_first_connected_console(vser);
+VirtIOSerialPortClass *vsc;
+
+if (!config->emerg_wr) {
+return;
+}
+/* Make sure we don't misdetect an emergency write when the guest
+ * does a short config write after an emergency write. */
+config->emerg_wr = 0;
+if (!port) {
+return;
+}
+vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+(void)vsc->have_data(port, _wr_lo, 1);
+}
+
static void guest_reset(VirtIOSerial *vser)
{
VirtIOSerialPort *port;
@@ -1098,6 +1134,7 @@ static void virtio_serial_class_init(ObjectClass *klass,
void *data)
vdc->unrealize = virtio_serial_device_unrealize;
vdc->get_features = get_features;
vdc->get_config = get_config;
+vdc->set_config = set_config;
vdc->set_status = set_status;
vdc->reset = vser_reset;
vdc->save = virtio_serial_save_device;
--
1.9.1
Re: [Qemu-devel] [PATCH] virtio-serial: virtio console emergency write support
Dear Amit, Amit Shah <amit.s...@redhat.com> writes: > On (Wed) 21 Sep 2016 [14:17:48], Sascha Silbe wrote: >> Add support for the virtio 1.0 "emergency write" >> (VIRTIO_CONSOLE_F_EMERG_WRITE) feature. This is useful for early guest >> debugging and might be used in cases where the guest crashes so badly >> that it cannot use virtqueues. >> >> Disabled for compatibility machines to avoid exposing a new feature to >> existing guests. >> >> Reviewed-by: Cornelia Huck <cornelia.h...@de.ibm.com> >> Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com> > > This looks fine, but can you split the patch - adding > find_first_connected_console(), set_config, and then finally enabling > the emergency_write feature? Sure. I've split it into two patches: One introducing find_first_connected_console() + set_config() but with no way for the guest to actually exploit it. The other patch exposing the emergency write virtio feature to the guest and disabling it for older machines using the compat property. Will send the updated series once the build tests complete. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
[Qemu-devel] [PATCH] virtio-serial: virtio console emergency write support
Add support for the virtio 1.0 "emergency write"
(VIRTIO_CONSOLE_F_EMERG_WRITE) feature. This is useful for early guest
debugging and might be used in cases where the guest crashes so badly
that it cannot use virtqueues.
Disabled for compatibility machines to avoid exposing a new feature to
existing guests.
Reviewed-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
hw/char/virtio-serial-bus.c | 49 ---
include/hw/compat.h | 6 -
include/hw/virtio/virtio-serial.h | 2 ++
3 files changed, 53 insertions(+), 4 deletions(-)
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index db57a38..db2a9f1 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -75,6 +75,19 @@ static VirtIOSerialPort *find_port_by_name(char *name)
return NULL;
}
+static VirtIOSerialPort *find_first_connected_console(VirtIOSerial *vser)
+{
+VirtIOSerialPort *port;
+
+QTAILQ_FOREACH(port, >ports, next) {
+VirtIOSerialPortClass const *vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+if (vsc->is_console && port->host_connected) {
+return port;
+}
+}
+return NULL;
+}
+
static bool use_multiport(VirtIOSerial *vser)
{
VirtIODevice *vdev = VIRTIO_DEVICE(vser);
@@ -528,6 +541,7 @@ static uint64_t get_features(VirtIODevice *vdev, uint64_t
features,
vser = VIRTIO_SERIAL(vdev);
+features |= vser->host_features;
if (vser->bus.max_nr_ports > 1) {
virtio_add_feature(, VIRTIO_CONSOLE_F_MULTIPORT);
}
@@ -547,6 +561,29 @@ static void get_config(VirtIODevice *vdev, uint8_t
*config_data)
vser->serial.max_virtserial_ports);
}
+/* Guest sent new config info */
+static void set_config(VirtIODevice *vdev, const uint8_t *config_data)
+{
+VirtIOSerial *vser = VIRTIO_SERIAL(vdev);
+struct virtio_console_config *config =
+(struct virtio_console_config *)config_data;
+uint8_t emerg_wr_lo = le32_to_cpu(config->emerg_wr);
+VirtIOSerialPort *port = find_first_connected_console(vser);
+VirtIOSerialPortClass *vsc;
+
+if (!config->emerg_wr) {
+return;
+}
+/* Make sure we don't misdetect an emergency write when the guest
+ * does a short config write after an emergency write. */
+config->emerg_wr = 0;
+if (!port) {
+return;
+}
+vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+(void)vsc->have_data(port, _wr_lo, 1);
+}
+
static void guest_reset(VirtIOSerial *vser)
{
VirtIOSerialPort *port;
@@ -967,6 +1004,7 @@ static void virtio_serial_device_realize(DeviceState *dev,
Error **errp)
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
VirtIOSerial *vser = VIRTIO_SERIAL(dev);
uint32_t i, max_supported_ports;
+size_t config_size = sizeof(struct virtio_console_config);
if (!vser->serial.max_virtserial_ports) {
error_setg(errp, "Maximum number of serial ports not specified");
@@ -981,10 +1019,12 @@ static void virtio_serial_device_realize(DeviceState
*dev, Error **errp)
return;
}
-/* We don't support emergency write, skip it for now. */
-/* TODO: cleaner fix, depending on host features. */
+if (!virtio_has_feature(vser->host_features,
+VIRTIO_CONSOLE_F_EMERG_WRITE)) {
+config_size = offsetof(struct virtio_console_config, emerg_wr);
+}
virtio_init(vdev, "virtio-serial", VIRTIO_ID_CONSOLE,
-offsetof(struct virtio_console_config, emerg_wr));
+config_size);
/* Spawn a new virtio-serial bus on which the ports will ride as devices */
qbus_create_inplace(>bus, sizeof(vser->bus), TYPE_VIRTIO_SERIAL_BUS,
@@ -1080,6 +1120,8 @@ VMSTATE_VIRTIO_DEVICE(console, 3, virtio_serial_load,
virtio_vmstate_save);
static Property virtio_serial_properties[] = {
DEFINE_PROP_UINT32("max_ports", VirtIOSerial, serial.max_virtserial_ports,
31),
+DEFINE_PROP_BIT64("emergency-write", VirtIOSerial, host_features,
+ VIRTIO_CONSOLE_F_EMERG_WRITE, true),
DEFINE_PROP_END_OF_LIST(),
};
@@ -1098,6 +1140,7 @@ static void virtio_serial_class_init(ObjectClass *klass,
void *data)
vdc->unrealize = virtio_serial_device_unrealize;
vdc->get_features = get_features;
vdc->get_config = get_config;
+vdc->set_config = set_config;
vdc->set_status = set_status;
vdc->reset = vser_reset;
vdc->save = virtio_serial_save_device;
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 08dd4fb..d48507a 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -2,7 +2,11 @@
#define HW_COMPAT_H
#define HW_COMPAT_2_7 \
-/* empty */
+{
Re: [Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name
Dear Eric,
Eric Blake <ebl...@redhat.com> writes:
> arglist = (all other parameters)
> if self.device_name:
> arglist.append(id=self.device_name)
> else:
> arglist.append(device='drive0')
> invoke(self.vm.qmp, arglist)
That would be:
qmp_args = {'cmd': 'blockdev-change-medium', 'filename': new_img,
'format': iotests.imgfmt}
if self.device_name is not None:
qmp_args['id'] = self.device_name
else:
qmp_args['device'] = 'drive0'
result = self.vm.qmp(**qmp_args)
Or:
qmp_args = {'filename': new_img, 'format': iotests.imgfmt}
if self.device_name is not None:
qmp_args['id'] = self.device_name
else:
qmp_args['device'] = 'drive0'
result = self.vm.qmp('blockdev-change-medium', **qmp_args)
FWIW these days I tend to use Python magics (like *args / **kwargs) only
if there's a _significant_ improvement in either functionality or
readability; especially for code bases that are being worked on by
developers who don't speak Python "natively". Using magics sparingly
also makes it easier for static analysis tools like pylint.
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name
Dear Eric,
Sascha Silbe <si...@linux.vnet.ibm.com> writes:
> result = self.vm.qmp('blockdev-change-medium',
> id=self.device_name or 'drive0',
> filename=new_img,
> format=iotests.imgfmt)
Sorry, my eyes deceived me. I thought the original code was setting the
"id" parameter in both cases. QEMUMachine.qmp() and
QEMUMonitorProtocol.cmd() together translate the keyword arguments
directly into QMP arguments, so passing None is different from not
passing anything at all. The if/else is probably the best approach in
that case; everything else I can think of would just be more
complicated.
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH 10/10] qemu-iotests/118: Test media change with qdev name
Dear Eric,
(replying only to the Python coding part, haven't looked at the patch
itself)
Eric Blake <ebl...@redhat.com> writes:
> On 08/19/2016 11:50 AM, Kevin Wolf wrote:
>> @@ -76,9 +79,15 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
>> self.assert_qmp(result, 'return[0]/inserted/image/filename',
>> new_img)
>>
>> def test_blockdev_change_medium(self):
>> -result = self.vm.qmp('blockdev-change-medium', device='drive0',
>> - filename=new_img,
>> -
>> format=iotests.imgfmt)
>> +if self.device_name is not None:
>> +result = self.vm.qmp('blockdev-change-medium',
>> + id=self.device_name, filename=new_img,
>> + format=iotests.imgfmt)
>> +else:
>> +result = self.vm.qmp('blockdev-change-medium',
>> + device='drive0', filename=new_img,
>> + format=iotests.imgfmt)
>
> I'm not enough of a python guru to know if there is any way to compress
> this to a shorter format (I do know, however, that the lack of an
> obvious ?: operator in python can indeed result in verbose if/else
> clauses compared to other languages).
Actually there is a direct equivalent of "?:" (which unfortunately isn't
in C11) in Python: "or". So you can use:
result = self.vm.qmp('blockdev-change-medium',
id=self.device_name or 'drive0', filename=new_img,
format=iotests.imgfmt)
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH v3 6/8] docker: make sure debootstrap is at least 1.0.67
Dear Fam,
Fam Zheng <f...@redhat.com> writes:
[...]
>> +NEED_DEBOOTSTRAP=true
>> +elif ! (echo "${MIN_DEBOOTSTRAP_VERSION}" ; "${DEBOOTSTRAP}" --version \
>> +| cut -d ' ' -f 2) | sort -t . -n -k 1,1 -k 2,2 -k 3,3 -C; then
>
> Like -V, -C is not available on OSX either. Maybe use "-c"? If so, I can fix
> it
> when applying.
>
> https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man1/sort.1.html
Thanks for the pointer. So Apple took a 2005 version of GNU sort,
removed version sort support (introduced in 1997) and neglected to
update it for POSIX.1-2008 compliance [1]?
Guess using "-c" and redirecting stderr to /dev/null is our only option
then. Feel free to fix that up when applying.
Thanks for pointing out these portability issues!
Sascha
[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/sort.html
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
[Qemu-devel] [PATCH v3 2/8] docker: avoid dependency on 'realpath' package
The 'realpath' executable is shipped in a separate package that isn't installed by default on some distros. We already use 'readlink -e' (provided by GNU coreutils) in some other part of the code, so let's settle for that instead. Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com> --- Too bad there isn't a POSIX equivalent of this. tests/docker/Makefile.include | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include index 4f4707d..1b20db0 100644 --- a/tests/docker/Makefile.include +++ b/tests/docker/Makefile.include @@ -116,7 +116,7 @@ docker-run-%: docker-qemu-src -e EXTRA_CONFIGURE_OPTS=$(EXTRA_CONFIGURE_OPTS) \ -e V=$V -e J=$J -e DEBUG=$(DEBUG)\ -e CCACHE_DIR=/var/tmp/ccache \ - -v $$(realpath $(DOCKER_SRC_COPY)):/var/tmp/qemu:z$(COMMA)ro \ + -v $$(readlink -e $(DOCKER_SRC_COPY)):/var/tmp/qemu:z$(COMMA)ro \ -v $(DOCKER_CCACHE_DIR):/var/tmp/ccache:z \ qemu:$(IMAGE) \ /var/tmp/qemu/run \ -- 1.9.1
[Qemu-devel] [PATCH v3 4/8] docker: debian-bootstrap.pre: print helpful message if DEB_ARCH/DEB_TYPE unset
The debian-bootstrap image doesn't choose a default architecture and
distribution version, instead the user has to set both DEB_ARCH and
DEB_TYPE in the environment. Print a reasonably helpful message if
either of them isn't set instead of complaining about "qemu-" being
missing or erroring out because we cannot cd to the mirror URL.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
v2→v3:
- send error messages to stderr
tests/docker/dockerfiles/debian-bootstrap.pre | 13 +
1 file changed, 13 insertions(+)
diff --git a/tests/docker/dockerfiles/debian-bootstrap.pre
b/tests/docker/dockerfiles/debian-bootstrap.pre
index 9b95a6b..3d9f7f4 100755
--- a/tests/docker/dockerfiles/debian-bootstrap.pre
+++ b/tests/docker/dockerfiles/debian-bootstrap.pre
@@ -15,6 +15,19 @@ exit_and_skip()
if [ -z $FAKEROOT ]; then
echo "Please install fakeroot to enable bootstraping" >&2
exit_and_skip
+
+fi
+
+if [ -z "${DEB_ARCH}" ]; then
+echo "Please set DEB_ARCH to choose an architecture (e.g. armhf)" >&2
+exit_and_skip
+
+fi
+
+if [ -z "${DEB_TYPE}" ]; then
+echo "Please set DEB_TYPE to a Debian archive name (e.g. testing)" >&2
+exit_and_skip
+
fi
# We check in order for
--
1.9.1
[Qemu-devel] [PATCH v3 3/8] docker: debian-bootstrap.pre: print error messages to stderr
Send error messages where they belong so they're seen even if stdout
is redirected to /dev/null.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
v2→v3:
- new patch
tests/docker/dockerfiles/debian-bootstrap.pre | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/docker/dockerfiles/debian-bootstrap.pre
b/tests/docker/dockerfiles/debian-bootstrap.pre
index 5d9c8d5..9b95a6b 100755
--- a/tests/docker/dockerfiles/debian-bootstrap.pre
+++ b/tests/docker/dockerfiles/debian-bootstrap.pre
@@ -13,7 +13,7 @@ exit_and_skip()
# fakeroot is needed to run the bootstrap stage
#
if [ -z $FAKEROOT ]; then
-echo "Please install fakeroot to enable bootstraping"
+echo "Please install fakeroot to enable bootstraping" >&2
exit_and_skip
fi
@@ -38,7 +38,7 @@ if [ -z $DEBOOTSTRAP_DIR ]; then
else
DEBOOTSTRAP=${DEBOOTSTRAP_DIR}/debootstrap
if [ ! -f $DEBOOTSTRAP ]; then
-echo "Couldn't find script at ${DEBOOTSTRAP}"
+echo "Couldn't find script at ${DEBOOTSTRAP}" >&2
exit_and_skip
fi
fi
@@ -48,7 +48,7 @@ fi
#
BINFMT_DIR=/proc/sys/fs/binfmt_misc
if [ ! -e $BINFMT_DIR ]; then
- echo "binfmt_misc needs enabling for a QEMU bootstrap to work"
+ echo "binfmt_misc needs enabling for a QEMU bootstrap to work" >&2
exit_and_skip
else
# DEB_ARCH and QEMU arch names are not totally aligned
@@ -76,7 +76,7 @@ else
;;
esac
if [ ! -e "${BINFMT_DIR}/$QEMU" ]; then
-echo "No binfmt_misc rule to run $QEMU, can't bootstrap"
+echo "No binfmt_misc rule to run $QEMU, can't bootstrap" >&2
exit_and_skip
fi
fi
--
1.9.1
[Qemu-devel] [PATCH v3 8/8] docker: silence debootstrap when --quiet is given
If we silence docker when --quiet is given, we should also silence the .pre script (i.e. debootstrap). Only discards stdout, so some diagnostics (e.g. from git clone) are still printed. Most of the verbose output is gone however and this way we still have a chance to see error messages. Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com> --- tests/docker/docker.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/docker/docker.py b/tests/docker/docker.py index efb2bf4..b85c165 100755 --- a/tests/docker/docker.py +++ b/tests/docker/docker.py @@ -239,8 +239,9 @@ class BuildCommand(SubCommand): # Is there a .pre file to run in the build context? docker_pre = os.path.splitext(args.dockerfile)[0]+".pre" if os.path.exists(docker_pre): +stdout = DEVNULL if args.quiet else None rc = subprocess.call(os.path.realpath(docker_pre), - cwd=docker_dir) + cwd=docker_dir, stdout=stdout) if rc == 3: print "Skip" return 0 -- 1.9.1
[Qemu-devel] [PATCH v3 5/8] docker: print warning if EXECUTABLE is not set when building debootstrap image
Building the debian-debootstrap image will usually fail if EXECUTABLE
isn't set (when using the Makefile). Warn the user in this case so
they know why it's failing.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
tests/docker/Makefile.include | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 1b20db0..19d4cc7 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -44,6 +44,9 @@ docker-image: ${DOCKER_TARGETS}
# General rule for building docker images
docker-image-%: $(DOCKER_FILES_DIR)/%.docker
+ @if test "$@" = docker-image-debian-bootstrap -a -z "$(EXECUTABLE)";
then \
+ echo WARNING: EXECUTABLE is not set, debootstrap may fail. 2>&1
; \
+ fi
$(call quiet-command,\
$(SRC_PATH)/tests/docker/docker.py build qemu:$* $< \
$(if $V,,--quiet) $(if $(NOCACHE),--no-cache) \
--
1.9.1
[Qemu-devel] [PATCH v3 6/8] docker: make sure debootstrap is at least 1.0.67
debootstrap prior to 1.0.67 generated an empty sources.list during
foreign bootstraps (Debian#732255 [1]). Fall back to the git checkout
if the installed debootstrap version is too old.
[1] https://bugs.debian.org/732255
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
v2→v3:
- fix unbalanced white space around pipes
- replaced GNU-specific version sort option with POSIX compliant set
of options
tests/docker/dockerfiles/debian-bootstrap.pre | 10 ++
1 file changed, 10 insertions(+)
diff --git a/tests/docker/dockerfiles/debian-bootstrap.pre
b/tests/docker/dockerfiles/debian-bootstrap.pre
index 3d9f7f4..ee69e89 100755
--- a/tests/docker/dockerfiles/debian-bootstrap.pre
+++ b/tests/docker/dockerfiles/debian-bootstrap.pre
@@ -3,6 +3,8 @@
# Simple wrapper for debootstrap, run in the docker build context
#
FAKEROOT=`which fakeroot 2> /dev/null`
+# debootstrap < 1.0.67 generates empty sources.list, see Debian#732255
+MIN_DEBOOTSTRAP_VERSION=1.0.67
exit_and_skip()
{
@@ -40,9 +42,17 @@ fi
#
if [ -z $DEBOOTSTRAP_DIR ]; then
+NEED_DEBOOTSTRAP=false
DEBOOTSTRAP=`which debootstrap 2> /dev/null`
if [ -z $DEBOOTSTRAP ]; then
echo "No debootstrap installed, attempting to install from SCM"
+NEED_DEBOOTSTRAP=true
+elif ! (echo "${MIN_DEBOOTSTRAP_VERSION}" ; "${DEBOOTSTRAP}" --version \
+| cut -d ' ' -f 2) | sort -t . -n -k 1,1 -k 2,2 -k 3,3 -C; then
+echo "debootstrap too old, attempting to install from SCM"
+NEED_DEBOOTSTRAP=true
+fi
+if $NEED_DEBOOTSTRAP; then
DEBOOTSTRAP_SOURCE=https://anonscm.debian.org/git/d-i/debootstrap.git
git clone ${DEBOOTSTRAP_SOURCE} ./debootstrap.git
export DEBOOTSTRAP_DIR=./debootstrap.git
--
1.9.1
[Qemu-devel] [PATCH v3 7/8] docker: build debootstrap after cloning
When using the git version of debootstrap (because no usable version
of debootstrap was installed on the host), we need to run 'make' so
that devices.tar.gz gets built. Otherwise the first debootstrap stage
will fail without printing any error message.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
tests/docker/dockerfiles/debian-bootstrap.pre | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/docker/dockerfiles/debian-bootstrap.pre
b/tests/docker/dockerfiles/debian-bootstrap.pre
index ee69e89..4322fbb 100755
--- a/tests/docker/dockerfiles/debian-bootstrap.pre
+++ b/tests/docker/dockerfiles/debian-bootstrap.pre
@@ -57,6 +57,7 @@ if [ -z $DEBOOTSTRAP_DIR ]; then
git clone ${DEBOOTSTRAP_SOURCE} ./debootstrap.git
export DEBOOTSTRAP_DIR=./debootstrap.git
DEBOOTSTRAP=./debootstrap.git/debootstrap
+(cd "${DEBOOTSTRAP_DIR}" && "${FAKEROOT}" make )
fi
else
DEBOOTSTRAP=${DEBOOTSTRAP_DIR}/debootstrap
--
1.9.1
[Qemu-devel] [PATCH v3 0/8] docker tests fixes
A couple of fixes for issues encountered while trying out the new docker test support. v2→v3: - fix non-portable sort -V usage - send debootstrap.pre error messages to stderr - whitespace changes v1→v2: - found a good place to stick the warning about EXECUTABLE - additional fixes for the debian-debootstrap image Sascha Silbe (8): docker.py: don't hang on large docker output docker: avoid dependency on 'realpath' package docker: debian-bootstrap.pre: print error messages to stderr docker: debian-bootstrap.pre: print helpful message if DEB_ARCH/DEB_TYPE unset docker: print warning if EXECUTABLE is not set when building debootstrap image docker: make sure debootstrap is at least 1.0.67 docker: build debootstrap after cloning docker: silence debootstrap when --quiet is given tests/docker/Makefile.include | 5 - tests/docker/docker.py| 12 ++ tests/docker/dockerfiles/debian-bootstrap.pre | 32 +++ 3 files changed, 40 insertions(+), 9 deletions(-) -- 1.9.1
[Qemu-devel] [PATCH v3 1/8] docker.py: don't hang on large docker output
Unlike Popen.communicate(), subprocess.call() doesn't read from the
stdout file descriptor. If the child process produces more output than
fits into the pipe buffer, it will block indefinitely.
If we don't intend to consume the output, just send it straight to
/dev/null to avoid this issue.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
Reviewed-by: Janosch Frank <fran...@linux.vnet.ibm.com>
---
This fixes a hang for me when building the Ubuntu docker image (empty
docker image cache).
tests/docker/docker.py | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 222a105..efb2bf4 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -25,6 +25,10 @@ from tarfile import TarFile, TarInfo
from StringIO import StringIO
from shutil import copy, rmtree
+
+DEVNULL = open(os.devnull, 'wb')
+
+
def _text_checksum(text):
"""Calculate a digest string unique to the text content"""
return hashlib.sha1(text).hexdigest()
@@ -34,8 +38,7 @@ def _guess_docker_command():
commands = [["docker"], ["sudo", "-n", "docker"]]
for cmd in commands:
if subprocess.call(cmd + ["images"],
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE) == 0:
+ stdout=DEVNULL, stderr=DEVNULL) == 0:
return cmd
commands_txt = "\n".join([" " + " ".join(x) for x in commands])
raise Exception("Cannot find working docker command. Tried:\n%s" % \
@@ -98,7 +101,7 @@ class Docker(object):
def _do(self, cmd, quiet=True, infile=None, **kwargs):
if quiet:
-kwargs["stdout"] = subprocess.PIPE
+kwargs["stdout"] = DEVNULL
if infile:
kwargs["stdin"] = infile
return subprocess.call(self._command + cmd, **kwargs)
--
1.9.1
Re: [Qemu-devel] [PATCH v2 3/7] docker: debian-bootstrap.pre: print helpful message if DEB_ARCH/DEB_TYPE unset
Dear Fam,
Fam Zheng <f...@redhat.com> writes:
[tests/docker/dockerfiles/debian-bootstrap.pre]
[...]
>> +if [ -z "${DEB_ARCH}" ]; then
>> +echo "Please set DEB_ARCH to choose an architecture (e.g. armhf)"
>> +exit_and_skip
>> +
>> +fi
>> +
>> +if [ -z "${DEB_TYPE}" ]; then
>> +echo "Please set DEB_TYPE to a Debian archive name (e.g. testing)"
>> +exit_and_skip
>> +
>
> Can you also update the three 'echo' commands to output the message to stderr?
I've updated my patch to send output to stderr and added a new patch
that fixes up the existing messages that are "fatal" (i.e. those
directly prior to an exit_and_skip).
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH v2 5/7] docker: make sure debootstrap is at least 1.0.67
Dear Fam,
Fam Zheng <f...@redhat.com> writes:
> On Wed, 08/24 20:31, Sascha Silbe wrote:
[tests/docker/dockerfiles/debian-bootstrap.pre]
>> +# debootstrap < 1.0.67 generates empty sources.list, see Debian#732255
>> +MIN_DEBOOTSTRAP_VERSION=1.0.67
[...]
>> if [ -z $DEBOOTSTRAP_DIR ]; then
>> +NEED_DEBOOTSTRAP=false
>> DEBOOTSTRAP=`which debootstrap 2> /dev/null`
>> if [ -z $DEBOOTSTRAP ]; then
>> echo "No debootstrap installed, attempting to install from SCM"
>> +NEED_DEBOOTSTRAP=true
>> +elif ! (echo "${MIN_DEBOOTSTRAP_VERSION}" ; "${DEBOOTSTRAP}" --version
>> |cut -d ' ' -f 2) |sort -VC; then
>
> Unbalanced whitespaces around '|'?
Sorry, habit from interactive shell usage. This way Ctrl+W will erase
the command and the pipe. Will fix this up.
> "sort -VC" seems to be unavalable on OSX. Is there another way to write the
> check?
Ah, right, BSD user space. sort -VC was such an elegant solution but
unfortunately it's a GNU extension. I'll try to come it up with some sed
or cut based hack instead.
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH v2 1/7] docker.py: don't hang on large docker output
Dear Fam, Fam Zheng <f...@redhat.com> writes: > On Wed, 08/24 20:30, Sascha Silbe wrote: [tests/docker/docker.py] >> @@ -25,6 +25,10 @@ from tarfile import TarFile, TarInfo >> from StringIO import StringIO >> from shutil import copy, rmtree >> >> + >> +DEVNULL = open(os.devnull, 'wb') >> + >> + > > Too many blank lines? Otherwise looks good. You caught me sneaking in a PEP-8 compliance fix. ;) Is qemu using a custom code style for Python sources? If so, is it documented somewhere? Thanks for reviewing! Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH v3 1/3] qemu-nbd: Add --fork option
Dear Max,
thanks for taking the time to fix the race condition!
Max Reitz <mre...@redhat.com> writes:
> Using the --fork option, one can make qemu-nbd fork the worker process.
> The original process will exit on error of the worker or once the worker
> enters the main loop.
> @@ -773,7 +780,7 @@ int main(int argc, char **argv)
> return 0;
> }
>
> -if (device && !verbose) {
> +if ((device && !verbose) || fork_process) {
> int stderr_fd[2];
> pid_t pid;
> int ret;
Looking at the surrounding (unchanged) code I see that qemu-nbd already
implemented a daemon mode. It's just that it's completely undocumented
and hinges on both the --device and the --verbose option. Yuck.
It seems there are two things --verbose does (from a user point of
view):
1. Print "NBD device %s is now connected to %s" and keep stderr open.
Debug messages are always printed to stderr, but in non-verbose
daemon mode they end up at /dev/null.
This is more or less what one usually expects from an option named
--verbose. Except that it only affects daemon mode and messages are
always printed (but end up at /dev/null).
2. Disable daemon mode.
I might expect this for an option named --debug, but certainly not
for --verbose...
A clean way forward would be something like this:
1. Introduce --foreground / --daemon, --quiet
Default to daemon mode with silent output if --connect is given,
foreground mode with visible output otherwise. Set non-daemon mode
with visible output if --verbose is given. Let --foreground /
--daemon / --quiet any default or implicit value. Document that
--verbose implicitly enables daemon mode for compatibility with
previous versions and that future versions may stop doing so
(i.e. users should use either --verbose --foreground or --verbose
--daemon).
3. At some point in the future (qemu 3.0?) we can stop having --verbose
imply --foreground.
I can give it a try if it's out of scope for your current task.
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH for-2.7] wxx: Fix broken build (mkdtemp unavailable)
Dear Peter, Peter Maydell <peter.mayd...@linaro.org> writes: > On 25 August 2016 at 10:36, Peter Maydell <peter.mayd...@linaro.org> wrote: >> On 23 August 2016 at 16:01, Sascha Silbe <si...@linux.vnet.ibm.com> wrote: >>> Glad to hear. It would be possibly to support the combination of glib < >>> 2.30.0 AND windows, but only by copying a considerable amount of code >>> from glib. I'd prefer to avoid that if we can help it. >> >> If we want to raise the minimum glib version requirement for >> Windows we need to enforce this in configure. (We have had >> a higher minimum for Windows hosts in the past, so there's >> precedent for doing it.) > > Or we could arrange to skip this test if we're on windows > with an old glib I guess. In general I agree with you. In practice test-logging was completely broken on Windows since 2.6.0 (it hard-coded /tmp) and I don't have a suitable environment to test a Windows build, so I'd feel uncomfortable submitting patches addressing this issue myself. However that shouldn't stop anyone else (Stefan perhaps? :) ) from fixing the tests on Windows. I'll gladly review the effects of the corresponding patches on the POSIX side. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH v2] iotests: Do not rely on unavailable domains in 162
Dear Max, Max Reitz <mre...@redhat.com> writes: [tests/qemu-iotests/162] [...] > +while true; do > +port=$((RANDOM + 32768)) > +$QEMU_NBD -p $port -f raw null-co:// &> /dev/null & > +nbd_pid=$! > +sleep 0.5 > + > +# Check whether the process is still alive > +# (which is the case if the server has been created successfully) > +if kill -0 $nbd_pid &> /dev/null; then > +break > +fi > +done Apart from being inherently racy, the chosen sleep time of 0.5s is also rather short in practice when running tests on a busy or slow host. Since in this case 162 believes start-up worked, it will still run into the original problem. Traditionally daemons fork and wait for their child to finish initialisation. Only once the child succeeded (or failed) will they exit themselves. That solves exactly the race condition above. If we don't have / want to introduce a good way to discover whether qemu-nbd start-up was successful, we need to check and wait for either qemu-nbd exiting (⇒ failed, try different port) or qemu-nbd having successfully acquired the port (⇒ success, continue with test). Unfortunately all the ways to discover whether a specific process is listening on a port are non-portable (no pun intended). Sorry to be a bother, but qemu-iotests failing intermittently on slow hosts is really a PITA. It's hard to tell the difference between the test randomly failing because the test is racy (→ false positive, not a bug) and the test randomly failing because the implementation is racy (→ true positive, bug). Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
[Qemu-devel] [PATCH v2 5/7] docker: make sure debootstrap is at least 1.0.67
debootstrap prior to 1.0.67 generated an empty sources.list during
foreign bootstraps (Debian#732255 [1]). Fall back to the git checkout
if the installed debootstrap version is too old.
[1] https://bugs.debian.org/732255
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
Not sure if this used to work in even older debootstrap versions; I
don't remember running into this before. But then I needed to set up
several config files manually after a debootstrap anyway (both foreign
and native), so sources.list might just have been another one to fix
up manually.
tests/docker/dockerfiles/debian-bootstrap.pre | 9 +
1 file changed, 9 insertions(+)
diff --git a/tests/docker/dockerfiles/debian-bootstrap.pre
b/tests/docker/dockerfiles/debian-bootstrap.pre
index 2ae363f..3c3e781 100755
--- a/tests/docker/dockerfiles/debian-bootstrap.pre
+++ b/tests/docker/dockerfiles/debian-bootstrap.pre
@@ -3,6 +3,8 @@
# Simple wrapper for debootstrap, run in the docker build context
#
FAKEROOT=`which fakeroot 2> /dev/null`
+# debootstrap < 1.0.67 generates empty sources.list, see Debian#732255
+MIN_DEBOOTSTRAP_VERSION=1.0.67
exit_and_skip()
{
@@ -40,9 +42,16 @@ fi
#
if [ -z $DEBOOTSTRAP_DIR ]; then
+NEED_DEBOOTSTRAP=false
DEBOOTSTRAP=`which debootstrap 2> /dev/null`
if [ -z $DEBOOTSTRAP ]; then
echo "No debootstrap installed, attempting to install from SCM"
+NEED_DEBOOTSTRAP=true
+elif ! (echo "${MIN_DEBOOTSTRAP_VERSION}" ; "${DEBOOTSTRAP}" --version
|cut -d ' ' -f 2) |sort -VC; then
+echo "debootstrap too old, attempting to install from SCM"
+NEED_DEBOOTSTRAP=true
+fi
+if $NEED_DEBOOTSTRAP; then
DEBOOTSTRAP_SOURCE=https://anonscm.debian.org/git/d-i/debootstrap.git
git clone ${DEBOOTSTRAP_SOURCE} ./debootstrap.git
export DEBOOTSTRAP_DIR=./debootstrap.git
--
1.9.1
[Qemu-devel] [PATCH v2 2/7] docker: avoid dependency on 'realpath' package
The 'realpath' executable is shipped in a separate package that isn't installed by default on some distros. We already use 'readlink -e' (provided by GNU coreutils) in some other part of the code, so let's settle for that instead. Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com> --- Too bad there isn't a POSIX equivalent of this. tests/docker/Makefile.include | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include index 4f4707d..1b20db0 100644 --- a/tests/docker/Makefile.include +++ b/tests/docker/Makefile.include @@ -116,7 +116,7 @@ docker-run-%: docker-qemu-src -e EXTRA_CONFIGURE_OPTS=$(EXTRA_CONFIGURE_OPTS) \ -e V=$V -e J=$J -e DEBUG=$(DEBUG)\ -e CCACHE_DIR=/var/tmp/ccache \ - -v $$(realpath $(DOCKER_SRC_COPY)):/var/tmp/qemu:z$(COMMA)ro \ + -v $$(readlink -e $(DOCKER_SRC_COPY)):/var/tmp/qemu:z$(COMMA)ro \ -v $(DOCKER_CCACHE_DIR):/var/tmp/ccache:z \ qemu:$(IMAGE) \ /var/tmp/qemu/run \ -- 1.9.1
[Qemu-devel] [PATCH v2 6/7] docker: build debootstrap after cloning
When using the git version of debootstrap (because no usable version
of debootstrap was installed on the host), we need to run 'make' so
that devices.tar.gz gets built. Otherwise the first debootstrap stage
will fail without printing any error message.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
tests/docker/dockerfiles/debian-bootstrap.pre | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/docker/dockerfiles/debian-bootstrap.pre
b/tests/docker/dockerfiles/debian-bootstrap.pre
index 3c3e781..a633224 100755
--- a/tests/docker/dockerfiles/debian-bootstrap.pre
+++ b/tests/docker/dockerfiles/debian-bootstrap.pre
@@ -56,6 +56,7 @@ if [ -z $DEBOOTSTRAP_DIR ]; then
git clone ${DEBOOTSTRAP_SOURCE} ./debootstrap.git
export DEBOOTSTRAP_DIR=./debootstrap.git
DEBOOTSTRAP=./debootstrap.git/debootstrap
+(cd "${DEBOOTSTRAP_DIR}" && "${FAKEROOT}" make )
fi
else
DEBOOTSTRAP=${DEBOOTSTRAP_DIR}/debootstrap
--
1.9.1
[Qemu-devel] [PATCH v2 1/7] docker.py: don't hang on large docker output
Unlike Popen.communicate(), subprocess.call() doesn't read from the
stdout file descriptor. If the child process produces more output than
fits into the pipe buffer, it will block indefinitely.
If we don't intend to consume the output, just send it straight to
/dev/null to avoid this issue.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
Reviewed-by: Janosch Frank <fran...@linux.vnet.ibm.com>
---
This fixes a hang for me when building the Ubuntu docker image (empty
docker image cache).
tests/docker/docker.py | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 222a105..efb2bf4 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -25,6 +25,10 @@ from tarfile import TarFile, TarInfo
from StringIO import StringIO
from shutil import copy, rmtree
+
+DEVNULL = open(os.devnull, 'wb')
+
+
def _text_checksum(text):
"""Calculate a digest string unique to the text content"""
return hashlib.sha1(text).hexdigest()
@@ -34,8 +38,7 @@ def _guess_docker_command():
commands = [["docker"], ["sudo", "-n", "docker"]]
for cmd in commands:
if subprocess.call(cmd + ["images"],
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE) == 0:
+ stdout=DEVNULL, stderr=DEVNULL) == 0:
return cmd
commands_txt = "\n".join([" " + " ".join(x) for x in commands])
raise Exception("Cannot find working docker command. Tried:\n%s" % \
@@ -98,7 +101,7 @@ class Docker(object):
def _do(self, cmd, quiet=True, infile=None, **kwargs):
if quiet:
-kwargs["stdout"] = subprocess.PIPE
+kwargs["stdout"] = DEVNULL
if infile:
kwargs["stdin"] = infile
return subprocess.call(self._command + cmd, **kwargs)
--
1.9.1
[Qemu-devel] [PATCH v2 4/7] docker: print warning if EXECUTABLE is not set when building debootstrap image
Building the debian-debootstrap image will usually fail if EXECUTABLE
isn't set (when using the Makefile). Warn the user in this case so
they know why it's failing.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
tests/docker/Makefile.include | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 1b20db0..19d4cc7 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -44,6 +44,9 @@ docker-image: ${DOCKER_TARGETS}
# General rule for building docker images
docker-image-%: $(DOCKER_FILES_DIR)/%.docker
+ @if test "$@" = docker-image-debian-bootstrap -a -z "$(EXECUTABLE)";
then \
+ echo WARNING: EXECUTABLE is not set, debootstrap may fail. 2>&1
; \
+ fi
$(call quiet-command,\
$(SRC_PATH)/tests/docker/docker.py build qemu:$* $< \
$(if $V,,--quiet) $(if $(NOCACHE),--no-cache) \
--
1.9.1
[Qemu-devel] [PATCH v2 7/7] docker: silence debootstrap when --quiet is given
If we silence docker when --quiet is given, we should also silence the .pre script (i.e. debootstrap). Only discards stdout, so some diagnostics (e.g. from git clone) are still printed. Most of the verbose output is gone however and this way we still have a chance to see error messages. Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com> --- tests/docker/docker.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/docker/docker.py b/tests/docker/docker.py index efb2bf4..b85c165 100755 --- a/tests/docker/docker.py +++ b/tests/docker/docker.py @@ -239,8 +239,9 @@ class BuildCommand(SubCommand): # Is there a .pre file to run in the build context? docker_pre = os.path.splitext(args.dockerfile)[0]+".pre" if os.path.exists(docker_pre): +stdout = DEVNULL if args.quiet else None rc = subprocess.call(os.path.realpath(docker_pre), - cwd=docker_dir) + cwd=docker_dir, stdout=stdout) if rc == 3: print "Skip" return 0 -- 1.9.1
[Qemu-devel] [PATCH v2 0/7] docker tests fixes
A couple of fixes for issues encountered while trying out the new docker test support. As of v2 building the debian-debootstrap image now works on my laptop, too. Thanks for the docker test support, BTW. The centos6 test came in rather handy today for testing the glib < 2.30 compatibility code. v1→v2: - found a good place to stick the warning about EXECUTABLE - additional fixes for the debian-debootstrap image Sascha Silbe (7): docker.py: don't hang on large docker output docker: avoid dependency on 'realpath' package docker: debian-bootstrap.pre: print helpful message if DEB_ARCH/DEB_TYPE unset docker: print warning if EXECUTABLE is not set when building debootstrap image docker: make sure debootstrap is at least 1.0.67 docker: build debootstrap after cloning docker: silence debootstrap when --quiet is given tests/docker/Makefile.include | 5 - tests/docker/docker.py| 12 tests/docker/dockerfiles/debian-bootstrap.pre | 23 +++ 3 files changed, 35 insertions(+), 5 deletions(-) -- 1.9.1
[Qemu-devel] [PATCH v2 3/7] docker: debian-bootstrap.pre: print helpful message if DEB_ARCH/DEB_TYPE unset
The debian-bootstrap image doesn't choose a default architecture and
distribution version, instead the user has to set both DEB_ARCH and
DEB_TYPE in the environment. Print a reasonably helpful message if
either of them isn't set instead of complaining about "qemu-" being
missing or erroring out because we cannot cd to the mirror URL.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
tests/docker/dockerfiles/debian-bootstrap.pre | 13 +
1 file changed, 13 insertions(+)
diff --git a/tests/docker/dockerfiles/debian-bootstrap.pre
b/tests/docker/dockerfiles/debian-bootstrap.pre
index 5d9c8d5..2ae363f 100755
--- a/tests/docker/dockerfiles/debian-bootstrap.pre
+++ b/tests/docker/dockerfiles/debian-bootstrap.pre
@@ -15,6 +15,19 @@ exit_and_skip()
if [ -z $FAKEROOT ]; then
echo "Please install fakeroot to enable bootstraping"
exit_and_skip
+
+fi
+
+if [ -z "${DEB_ARCH}" ]; then
+echo "Please set DEB_ARCH to choose an architecture (e.g. armhf)"
+exit_and_skip
+
+fi
+
+if [ -z "${DEB_TYPE}" ]; then
+echo "Please set DEB_TYPE to a Debian archive name (e.g. testing)"
+exit_and_skip
+
fi
# We check in order for
--
1.9.1
Re: [Qemu-devel] [PATCH for-2.7] wxx: Fix broken build (mkdtemp unavailable)
Dear Stefan, Stefan Weil <s...@weilnetz.de> writes: > On 08/23/16 21:00, Sascha Silbe wrote: >> Stefan Weil <s...@weilnetz.de> writes: >> >>> Commit 50455700092412d90ffaf57ee5d00f38f7d1cc5b added new code which >>> does not compile for Windows. >> [...] >> This worked fine in my cross-build environment (mingw32-* on Fedora 22) >> as that has glib 2.44.0. Is there a specific reason you're using a glib >> version that's at least half a decade old (glib 2.30.0 was released in >> 2011) on Windows? AFAICT the MSYS2 installer recommended by glib >> upstream [1] has glib 2.41.1. > > My Debian build machine has glib 2.28.8 for cross compilation, obviously > unchanged since 2011, so it is indeed half a decade old. OK, so it's just your local build environment being outdated? No particular need to use that ancient version? > On my Debian notebook I use a cross glib 2.46.2, so cross compilation > will work there without that patch. > > That greatly reduces the need for the patch. Glad to hear. It would be possibly to support the combination of glib < 2.30.0 AND windows, but only by copying a considerable amount of code from glib. I'd prefer to avoid that if we can help it. >> As for your change: It may fix building qemu itself, but building >> test-logging should still be broken. Unlike some other tests, it isn't >> built on POSIX or Linux only. Did "make check" work before my patch in >> your environment? > > "make check" for Windows does not work in my cross environment. OK, I see. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH for-2.8] iotests: Do not rely on unavailable domains in 162
Dear Max, Max Reitz <mre...@redhat.com> writes: > On 2016-08-23 at 10:17, Sascha Silbe wrote: >> Max Reitz <mre...@redhat.com> writes: [tests/qemu-iotests/162] >>> Good idea. We can just let the script generate a random port; >>> $(($RANDOM+32768)) should do the trick. >> >> Which will fail just the same as the original version if anything (not >> just qemu-nbd) is already occupying the port you happened to >> choose. > > But I think figuring that out from the error log will be rather trivial: > > > +Failed to bind socket: Address already in use Once a human starts digging into the logs, yes. [...] > If you think this test failing is really an issue that we should fix, > then I'd rather put the qemu-nbd launch inside of a loop, retrying until > it succeeds with some random port. Seems to be the least ugly approach for now. And yeah, fixing it would be important IMO. Having automated tests and CI systems fail randomly is really annoying. People stop running the tests resp. ignore the CI mails pretty quickly if there are false positives. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH for-2.7] wxx: Fix broken build (mkdtemp unavailable)
Dear Stefan, Stefan Weil <s...@weilnetz.de> writes: > Commit 50455700092412d90ffaf57ee5d00f38f7d1cc5b added new code which > does not compile for Windows. [...] [include/glib-compat.h] > @@ -48,6 +48,7 @@ static inline gint64 qemu_g_get_monotonic_time(void) > gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout); > #endif > > +#if !defined(_WIN32) > #if !GLIB_CHECK_VERSION(2, 30, 0) > /* Not a 100% compatible implementation, but good enough for most > * cases. Placeholders are only supported at the end of the > @@ -65,8 +66,10 @@ static inline gchar *qemu_g_dir_make_tmp(gchar const > *tmpl, GError **error) > g_free(path); > return NULL; > } > + > #define g_dir_make_tmp(tmpl, error) qemu_g_dir_make_tmp(tmpl, error) > #endif /* glib 2.30 */ > +#endif /* !_WIN32 */ This worked fine in my cross-build environment (mingw32-* on Fedora 22) as that has glib 2.44.0. Is there a specific reason you're using a glib version that's at least half a decade old (glib 2.30.0 was released in 2011) on Windows? AFAICT the MSYS2 installer recommended by glib upstream [1] has glib 2.41.1. As for your change: It may fix building qemu itself, but building test-logging should still be broken. Unlike some other tests, it isn't built on POSIX or Linux only. Did "make check" work before my patch in your environment? Sascha [1] http://www.gtk.org/download/windows.php [2] https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-glib2/PKGBUILD -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH for-2.8] iotests: Do not rely on unavailable domains in 162
Dear Max, Max Reitz <mre...@redhat.com> writes: > On 2016-08-23 at 07:44, Sascha Silbe wrote: >> Max Reitz <mre...@redhat.com> writes: [tests/qemu-iotests/162] >> Using a fixed port number means multiple users won't be able to run this >> in parallel. That it's only open for a short time actually makes the >> issue a bit worse as it's hard to understand just why the test failed >> intermittently. >> >> Is there a way to have qemu-nbd use a random port and print the port >> number? > > Good idea. We can just let the script generate a random port; > $(($RANDOM+32768)) should do the trick. Which will fail just the same as the original version if anything (not just qemu-nbd) is already occupying the port you happened to choose. Maybe we should just fix this part: >>> +# (We need to set up a server here, because the error message for >>> "Connection >>> +# refused" does not contain the destination port) Including the port number in the "Connection refused" error message is a useful diagnostic, especially if it's a non-default port. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH 0/3] docker tests fixes
Dear Alex, Alex Bennée <alex.ben...@linaro.org> writes: > Sascha Silbe <si...@linux.vnet.ibm.com> writes: > >> A couple of fixes for issues encountered while trying out the new >> docker test support. The debian-bootstrap image still doesn't build >> for me, but that's a problem for another day. > > Can you offer any details? What host OS and what errors do you get? I'll assume you're talking about the remaining issue, rather than the ones fixed by this series. Haven't spent much time yet trying to figure it out. It looks like sources.list inside the image is missing "deb-src" lines. Running docker.py manually without --quiet (would be nice if DEBUG=1 did that, BTW) I get the following output: === Begin === silbe@oc4731375738:~/build/qemu-devel$ date; DEB_ARCH=armel DEB_TYPE=stable EXECUTABLE=/usr/bin/qemu-arm-static DEBUG=1 /home/silbe/recoverable/qemu/tests/docker/docker.py build qemu:debian-bootstrap /home/silbe/recoverable/qemu/tests/docker/dockerfiles/debian-bootstrap.docker --include-executable=/usr/bin/qemu-arm-static Tue Aug 23 15:39:00 CEST 2016 Building a rootfs using /usr/bin/fakeroot and /usr/sbin/debootstrap armel/stable W: Cannot check Release signature; keyring file not available /usr/share/keyrings/debian-archive-keyring.gpg [...] I: Extracting zlib1g... /usr/bin/qemu-arm-static had no associated libraries (static build?) Sending build context to Docker daemon 201.2 MB Sending build context to Docker daemon Step 0 : FROM scratch ---> Step 1 : ADD . / ---> 366e15a5b4d8 Removing intermediate container 865d34d36d2b Step 2 : RUN sed -i 's/in_target mount/echo not for docker in_target mount/g' /debootstrap/functions ---> Running in cf539dd3a3fd ---> 313acea8974a Removing intermediate container cf539dd3a3fd Step 3 : RUN /debootstrap/debootstrap --second-stage ---> Running in 7851b75bda73 I: Keyring file not available at /usr/share/keyrings/debian-archive-keyring.gpg; switching to https mirror https://mirrors.kernel.org/debian I: Installing core packages... [...] I: Configuring libc-bin... I: Base system installed successfully. ---> 68b8522ff2e7 Removing intermediate container 7851b75bda73 Step 4 : RUN cat /etc/apt/sources.list | sed "s/deb/deb-src/" >> /etc/apt/sources.list ---> Running in 4cc5be448b57 ---> 4fa669cb8511 Removing intermediate container 4cc5be448b57 Step 5 : RUN apt-get update ---> Running in 354a69607da0 Reading package lists... ---> 3025b4f79141 Removing intermediate container 354a69607da0 Step 6 : RUN apt-get -y build-dep qemu ---> Running in 81bfa9e9fce8 Reading package lists... Building dependency tree... E: You must put some 'source' URIs in your sources.list INFO[0184] The command [/bin/sh -c apt-get -y build-dep qemu] returned a non-zero code: 100 === End === Host is an x86_64 running Ubuntu 14.04. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH v2 1/2] glib: add compatibility implementation for g_dir_make_tmp()
Dear Peter,
Peter Maydell <peter.mayd...@linaro.org> writes:
[...]
>> +#if !GLIB_CHECK_VERSION(2, 30, 0)
>> +/* Not a 100% compatible implementation, but good enough for most
>> + * cases. Placeholders are only supported at the end of the
>> + * template. */
>> +static inline gchar *qemu_g_dir_make_tmp(gchar const *tmpl, GError **error)
>> +{
>> +gchar const *template = tmpl ? tmpl : ".XX";
>
> This doesn't build:
> In file included from /Users/pm215/src/qemu-for-merges/disas/arm-a64.cc:21:
> In file included from
> /Users/pm215/src/qemu-for-merges/include/qemu/osdep.h:107:
> /Users/pm215/src/qemu-for-merges/include/glib-compat.h:57:18: error:
> expected unqualified-id
> gchar const *template = tmpl ? tmpl : ".XX";
> ^
> /Users/pm215/src/qemu-for-merges/include/glib-compat.h:58:53: error:
> expected expression
> gchar *path = g_build_filename(g_get_tmp_dir(), template, NULL);
> ^
>
> because this header is included from some C++ files, where
> "template" is a reserved word. I've fixed this by squashing
> in the following change:
Curious, didn't hit this on my laptop (Ubuntu 14.04, x86_64) and neither
in the centos6 docker image. At least on my laptop arm-a64.cc gets
compiled and -Werror is enabled, too. Does this happen with a particular
tool chain only? clang perchance?
FWIW, I would have expected the C language linkage specification
(i.e. extern "C" {}) in disas/arm-a64.cc to take care of this kind of
issue. However after scanning through C++11 it seems the linkage
specification only affects external linkage names. Plus is doesn't seem
keywords are valid identifiers even for external C language linkage
names; there's only an exception for attributes, not for non-C++
language linkage.
[From second mail]
> Applied to master (with minor fixup to patch 1); thanks.
Thanks!
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH for-2.8] iotests: Do not rely on unavailable domains in 162
Dear Max,
Max Reitz <mre...@redhat.com> writes:
[tests/qemu-iotests/162]
[...]
> +# (We need to set up a server here, because the error message for "Connection
> +# refused" does not contain the destination port)
> +$QEMU_NBD -p 42424 -f raw null-co:// &
> +sleep 0.5
> +$QEMU_IMG info 'json:{"driver": "nbd", "host": "localhost", "port": 42424}' \
> +| grep '^image'
Using a fixed port number means multiple users won't be able to run this
in parallel. That it's only open for a short time actually makes the
issue a bit worse as it's hard to understand just why the test failed
intermittently.
Is there a way to have qemu-nbd use a random port and print the port
number?
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
[Qemu-devel] [PATCH 3/3] docker: debian-bootstrap.pre: print helpful message if DEB_ARCH/DEB_TYPE unset
The debian-bootstrap image doesn't choose a default architecture and
distribution version, instead the user has to set both DEB_ARCH and
DEB_TYPE in the environment. Print a reasonably helpful message if
either of them isn't set instead of complaining about "qemu-" being
missing or erroring out because we cannot cd to the mirror URL.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
I haven't figured out a good way to warn about qemu-user-* being
missing because EXECUTABLE isn't set. debian-bootstrap.pre runs before
docker.py copies the executable so I cannot check in
debian-bootstrap.pre whether the binfmt interpreter exists. The
EXECUTABLE environment variable needs to be set only when run via
make, so checking it in debian-bootstrap.pre is no good either. And an
additional docker-image-debian-bootstrap rule in the Makefile that
checks if EXECUTABLE is set would override the regular rule, not
enhance it.
tests/docker/dockerfiles/debian-bootstrap.pre | 13 +
1 file changed, 13 insertions(+)
diff --git a/tests/docker/dockerfiles/debian-bootstrap.pre
b/tests/docker/dockerfiles/debian-bootstrap.pre
index 5d9c8d5..2ae363f 100755
--- a/tests/docker/dockerfiles/debian-bootstrap.pre
+++ b/tests/docker/dockerfiles/debian-bootstrap.pre
@@ -15,6 +15,19 @@ exit_and_skip()
if [ -z $FAKEROOT ]; then
echo "Please install fakeroot to enable bootstraping"
exit_and_skip
+
+fi
+
+if [ -z "${DEB_ARCH}" ]; then
+echo "Please set DEB_ARCH to choose an architecture (e.g. armhf)"
+exit_and_skip
+
+fi
+
+if [ -z "${DEB_TYPE}" ]; then
+echo "Please set DEB_TYPE to a Debian archive name (e.g. testing)"
+exit_and_skip
+
fi
# We check in order for
--
1.9.1
[Qemu-devel] [PATCH 2/3] docker: avoid dependency on 'realpath' package
The 'realpath' executable is shipped in a separate package that isn't installed by default on some distros. We already use 'readlink -e' (provided by GNU coreutils) in some other part of the code, so let's settle for that instead. Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com> --- Too bad there isn't a POSIX equivalent of this. tests/docker/Makefile.include | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include index 4f4707d..1b20db0 100644 --- a/tests/docker/Makefile.include +++ b/tests/docker/Makefile.include @@ -116,7 +116,7 @@ docker-run-%: docker-qemu-src -e EXTRA_CONFIGURE_OPTS=$(EXTRA_CONFIGURE_OPTS) \ -e V=$V -e J=$J -e DEBUG=$(DEBUG)\ -e CCACHE_DIR=/var/tmp/ccache \ - -v $$(realpath $(DOCKER_SRC_COPY)):/var/tmp/qemu:z$(COMMA)ro \ + -v $$(readlink -e $(DOCKER_SRC_COPY)):/var/tmp/qemu:z$(COMMA)ro \ -v $(DOCKER_CCACHE_DIR):/var/tmp/ccache:z \ qemu:$(IMAGE) \ /var/tmp/qemu/run \ -- 1.9.1
[Qemu-devel] [PATCH 0/3] docker tests fixes
A couple of fixes for issues encountered while trying out the new docker test support. The debian-bootstrap image still doesn't build for me, but that's a problem for another day. Thanks for the docker test support, BTW. The centos6 test came in rather handy today for testing the glib < 2.30 compatibility code. Sascha Silbe (3): docker.py: don't hang on large docker output docker: avoid dependency on 'realpath' package docker: debian-bootstrap.pre: print helpful message if DEB_ARCH/DEB_TYPE unset tests/docker/Makefile.include | 2 +- tests/docker/docker.py| 9 ++--- tests/docker/dockerfiles/debian-bootstrap.pre | 13 + 3 files changed, 20 insertions(+), 4 deletions(-) -- 1.9.1
[Qemu-devel] [PATCH 1/3] docker.py: don't hang on large docker output
Unlike Popen.communicate(), subprocess.call() doesn't read from the
stdout file descriptor. If the child process produces more output than
fits into the pipe buffer, it will block indefinitely.
If we don't intend to consume the output, just send it straight to
/dev/null to avoid this issue.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
This fixes a hang for me when building the Ubuntu docker image (empty
docker image cache).
tests/docker/docker.py | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 222a105..efb2bf4 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -25,6 +25,10 @@ from tarfile import TarFile, TarInfo
from StringIO import StringIO
from shutil import copy, rmtree
+
+DEVNULL = open(os.devnull, 'wb')
+
+
def _text_checksum(text):
"""Calculate a digest string unique to the text content"""
return hashlib.sha1(text).hexdigest()
@@ -34,8 +38,7 @@ def _guess_docker_command():
commands = [["docker"], ["sudo", "-n", "docker"]]
for cmd in commands:
if subprocess.call(cmd + ["images"],
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE) == 0:
+ stdout=DEVNULL, stderr=DEVNULL) == 0:
return cmd
commands_txt = "\n".join([" " + " ".join(x) for x in commands])
raise Exception("Cannot find working docker command. Tried:\n%s" % \
@@ -98,7 +101,7 @@ class Docker(object):
def _do(self, cmd, quiet=True, infile=None, **kwargs):
if quiet:
-kwargs["stdout"] = subprocess.PIPE
+kwargs["stdout"] = DEVNULL
if infile:
kwargs["stdin"] = infile
return subprocess.call(self._command + cmd, **kwargs)
--
1.9.1
[Qemu-devel] [PATCH v2 1/2] glib: add compatibility implementation for g_dir_make_tmp()
We're going to make use of g_dir_make_tmp() in test-logging. Provide a
compatibility implementation of it for glib < 2.30.
May behave differently in some edge cases (e.g. pattern only at the
end of the template, the file name is not part of the error message),
but good enough in practice.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
v1→v2:
- new patch
include/glib-compat.h | 21 +
1 file changed, 21 insertions(+)
diff --git a/include/glib-compat.h b/include/glib-compat.h
index 01aa7b3..de64ac2 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -48,6 +48,27 @@ static inline gint64 qemu_g_get_monotonic_time(void)
gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout);
#endif
+#if !GLIB_CHECK_VERSION(2, 30, 0)
+/* Not a 100% compatible implementation, but good enough for most
+ * cases. Placeholders are only supported at the end of the
+ * template. */
+static inline gchar *qemu_g_dir_make_tmp(gchar const *tmpl, GError **error)
+{
+gchar const *template = tmpl ? tmpl : ".XX";
+gchar *path = g_build_filename(g_get_tmp_dir(), template, NULL);
+
+if (mkdtemp(path) != NULL) {
+return path;
+}
+/* Error occurred, clean up. */
+g_set_error(error, G_FILE_ERROR, g_file_error_from_errno(errno),
+"mkdtemp() failed");
+g_free(path);
+return NULL;
+}
+#define g_dir_make_tmp(tmpl, error) qemu_g_dir_make_tmp(tmpl, error)
+#endif /* glib 2.30 */
+
#if !GLIB_CHECK_VERSION(2, 31, 0)
/* before glib-2.31, GMutex and GCond was dynamic-only (there was a separate
* GStaticMutex, but it didn't work with condition variables).
--
1.9.1
[Qemu-devel] [PATCH v2 2/2] test-logging: don't hard-code paths in /tmp
Since f6880b7f [qemu-log: support simple pid substitution for logs],
test-logging creates files with hard-coded names in /tmp. In the best
case, this prevents multiple developers from running "make check" on
the same machine. In the worst case, it allows for symlink attacks,
enabling an attacker to overwrite files that are writable to the
developer running "make check".
Instead of hard-coding the paths, create a temporary directory using
g_dir_make_tmp() and clean it up afterwards.
Fixes: f6880b7f ("qemu-log: support simple pid substitution for logs")
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
v1→v2:
- Factor out g_build_filename() + qemu_set_log_filename() + g_free()
into helper set_log_path_tmp().
- Replace rmtree() spawning "rm -rf" with rmdir_full() using glib
directory handling (non-recursive). Private to test-logging for
now.
tests/test-logging.c | 48 +---
1 file changed, 41 insertions(+), 7 deletions(-)
diff --git a/tests/test-logging.c b/tests/test-logging.c
index cdf13c6..a12585f 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -25,6 +25,7 @@
*/
#include "qemu/osdep.h"
+#include
#include "qemu-common.h"
#include "qapi/error.h"
@@ -86,24 +87,57 @@ static void test_parse_range(void)
error_free_or_abort();
}
-static void test_parse_path(void)
+static void set_log_path_tmp(char const *dir, char const *tpl, Error **errp)
{
+gchar *file_path = g_build_filename(dir, tpl, NULL);
+
+qemu_set_log_filename(file_path, errp);
+g_free(file_path);
+}
+
+static void test_parse_path(gconstpointer data)
+{
+gchar const *tmp_path = data;
Error *err = NULL;
-qemu_set_log_filename("/tmp/qemu.log", _abort);
-qemu_set_log_filename("/tmp/qemu-%d.log", _abort);
-qemu_set_log_filename("/tmp/qemu.log.%d", _abort);
+set_log_path_tmp(tmp_path, "qemu.log", _abort);
+set_log_path_tmp(tmp_path, "qemu-%d.log", _abort);
+set_log_path_tmp(tmp_path, "qemu.log.%d", _abort);
-qemu_set_log_filename("/tmp/qemu-%d%d.log", );
+set_log_path_tmp(tmp_path, "qemu-%d%d.log", );
error_free_or_abort();
}
+/* Remove a directory and all its entries (non-recursive). */
+static void rmdir_full(gchar const *root)
+{
+GDir *root_gdir = g_dir_open(root, 0, NULL);
+gchar const *entry_name;
+
+g_assert_nonnull(root_gdir);
+while ((entry_name = g_dir_read_name(root_gdir)) != NULL) {
+gchar *entry_path = g_build_filename(root, entry_name, NULL);
+g_assert(g_remove(entry_path) == 0);
+g_free(entry_path);
+}
+g_dir_close(root_gdir);
+g_assert(g_rmdir(root) == 0);
+}
+
int main(int argc, char **argv)
{
+gchar *tmp_path = g_dir_make_tmp("qemu-test-logging.XX", NULL);
+int rc;
+
g_test_init(, , NULL);
+g_assert_nonnull(tmp_path);
g_test_add_func("/logging/parse_range", test_parse_range);
-g_test_add_func("/logging/parse_path", test_parse_path);
+g_test_add_data_func("/logging/parse_path", tmp_path, test_parse_path);
-return g_test_run();
+rc = g_test_run();
+
+rmdir_full(tmp_path);
+g_free(tmp_path);
+return rc;
}
--
1.9.1
[Qemu-devel] [PATCH for-v2.7 v2 0/2] test-logging: don't hard-code paths in /tmp
This version should be good enough for inclusion in 2.7. I kept the temporary directory removal function local to test-logging for now, only cleaning up a single directory level. We can still factor it out and make it more generic in the 2.8 cycle. For 2.7 I'd rather stick with a minimal approach as it's very late in the cycle. Tested successfully with the centos6 docker image. Apart from a hang (tests/test-qga) and a race condition (tests/acpi-test-disk.raw missing) that both happen even without my patches, it also works well on Ubuntu 14.04. Feel free to perform any additional fixup required to land this in rc4; I might not be around again until Tuesday. Sascha Silbe (2): glib: add compatibility implementation for g_dir_make_tmp() test-logging: don't hard-code paths in /tmp include/glib-compat.h | 21 + tests/test-logging.c | 48 +--- 2 files changed, 62 insertions(+), 7 deletions(-) -- 1.9.1
Re: [Qemu-devel] [PATCH] test-logging: don't hard-code paths in /tmp
Dear Peter, Peter Maydell <peter.mayd...@linaro.org> writes: > Are you planning to send a v2 of this patch? I was hoping we could > fix the non-deleted logfiles for qemu 2.7.0 but it's getting a bit > late in the cycle... I'll try cooking up a version that's good enough for 2.7. I expected it to land in 2.8 so I wasn't in a hurry. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH] test-logging: don't hard-code paths in /tmp
Dear Peter,
Peter Maydell <peter.mayd...@linaro.org> writes:
> On 15 July 2016 at 17:24, Sascha Silbe <si...@linux.vnet.ibm.com> wrote:
[...]
>> Instead of hard-coding the paths, create a temporary directory using
>> g_dir_make_tmp() and clean it up afterwards.
>>
>> Fixes: f6880b7f ("qemu-log: support simple pid substitution for logs")
>> Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
>
> Thanks for this patch -- I just noticed that the test was leaving
> temporary files not cleaned up, which brought me to this patch
> by searching the mail archives...
I have totally forgotten about it. Would probably have remembered the
next time "make check" failed on a shared machine. ;)
[tests/test-logging.c:test_parse_path()]
>> +gchar *plain_path = g_build_filename(tmp_path, "qemu.log", NULL);
>> +gchar *pid_infix_path = g_build_filename(tmp_path, "qemu-%d.log", NULL);
>> +gchar *pid_suffix_path = g_build_filename(tmp_path, "qemu.log.%d",
>> NULL);
>> +gchar *double_pid_path = g_build_filename(tmp_path, "qemu-%d%d.log",
>> NULL);
[...]
> This could usefully be refactored to have a helper function that does
> the g_build_filename/qemu_set_log_filename/g_free.
I wasn't sure about it but it actually ended up being a bit nicer than
what I had before.
>> +static void rmtree(gchar const *root)
[...]
> I don't really like spawning rm here for this. We know we
> don't have any subdirectories here so we can just
>gdir = g_dir_open(root, 0, NULL);
>for (;;) {
>const char *f = g_dir_read_name(gdir);
>if (!f) {
>break;
>}
>g_remove(f);
>}
>g_dir_close(gdir);
>g_rmdir(root);
>
> (add error handling to taste).
I don't really like the rm spawning solution either. But the above plus
error handling was a bit much for a single test for my taste.
Is there some place I could stick something like the above so it could
at least be reused by other tests in the future? (Even though I very
much hate the idea of implementing yet another rmtree()).
[...]
>> int main(int argc, char **argv)
>> {
>> +gchar *tmp_path = g_dir_make_tmp("qemu-test-logging.XX", NULL);
>
> Unfortunately g_dir_make_tmp() only came in in glib 2.30, and we have
> to be able to build with glib 2.22.
Bummer. The old interfaces were really awkward. Is there somewhere I
could put a compatibility wrapper, implementing g_dir_make_tmp() using
the old interfaces?
Thanks for the review!
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH 2/2] iotests: fix 109
Dear Max, Max Reitz <mre...@redhat.com> writes: > On 03.08.2016 17:22, Sascha Silbe wrote: [...] >> What are the exact semantics of the "offset" field for >> BLOCK_JOB_COMPLETED? >> >> docs/qmp-events.txt is rather vague. As an API consumer I'd have assumed >> that everything up to offset has been completed successfully. If that >> interpretation is correct, offset must be 0 for this test because the >> very first sector wasn't mirrored successfully. > > As far as I'm aware, it doesn't have any real semantics besides the fact > that $offset / $len is the progress of the block job; so it's the offset > "in the job", but not the offset in the source disk. Ok. Would be nice to mention that in docs/qmp-events.txt, to avoid API consumers relying on more exact semantics (resuming at the block after offset to step over error locations for example). The name 'offset' is pretty suggestive... Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH 2/2] iotests: fix 109
Dear Vladimir,
Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes:
> 109 iotest is broken for raw after 0965a41e998ab820b5
> [mirror: double performance of the bulk stage if the disc is full]
>
> The problem is with finishing block-job with error: before specified
> patch mirror was not very async and it created one big request at disk
> start, this request finished with error and qemu produced
> BLOCK_JOB_COMPLETED with zero progress.
>
> After 0965a41, mirror starts several smaller requests in parallel, when
> BLOCK_JOB_COMPLETED emited we have some successful non-zero progress.
[...]
> --- a/tests/qemu-iotests/109.out
> +++ b/tests/qemu-iotests/109.out
> @@ -135,7 +135,7 @@ Automatically detecting the format is dangerous for raw
> images, write operations
> Specify the 'raw' format explicitly to remove the restrictions.
> {"return": {}}
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action":
> "report"}}
> -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": 0,
> "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset":
> OFFSET, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
What are the exact semantics of the "offset" field for
BLOCK_JOB_COMPLETED?
docs/qmp-events.txt is rather vague. As an API consumer I'd have assumed
that everything up to offset has been completed successfully. If that
interpretation is correct, offset must be 0 for this test because the
very first sector wasn't mirrored successfully.
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved
Dear Eric, Eric Blake <ebl...@redhat.com> writes: >> +++ b/include/qapi/error.h >> @@ -170,6 +170,9 @@ void error_setg_internal(Error **errp, >> * Just like error_setg(), with @os_error info added to the message. >> * If @os_error is non-zero, ": " + strerror(os_error) is appended to >> * the human-readable error message. >> + * >> + * The value of errno (which usually can get clobbered by almost any >> + * function call) will be preserved. >> */ >> #define error_setg_errno(errp, os_error, fmt, ...) \ >> error_setg_errno_internal((errp), __FILE__, __LINE__, __func__, \ > > Do we need/want to make the guarantee of preserving errno across any of > the other functions and macros declared in error.h? It would be more consistent to have all error reporting functions promise this, even if they do not get passed the errno. In some cases the errno might not matter to the user (so error_setg_errno() isn't used), but still be passed on to the caller to signal an error (so clobbering it could be problematic). Can prepare a follow-up patch that makes sure error_setg(), error_propagate(), error_setg_file_open(), error_set() preserve errno. Optionally also the other functions listed in include/qapi/error.h and include/qemu/error-report.h. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno may be clobbered
Dear Markus, Markus Armbruster <arm...@redhat.com> writes: > Sascha Silbe <si...@linux.vnet.ibm.com> writes: > >> As a general policy, we want callers to save errno >> themselves. error_setg_internal() currently goes out of its way to >> preserve errno, > > In other words, "error_setg_errno(): errno may be clobbered" is a lie :) No, it's just standardese (though it should be in capital letters for that) for "do not rely on it, we may change our mind any time". :) >> so with the API documentation not mentioning it either >> way, callers might come to rely on the current behaviour of the >> implementation. > > According to Max, we rely on the errno-saving behavior in several places > already, and don't intend to change them. > >> Spell out that we don't want to make that promise. > > Well, we already did, tacitly. Let's make the promise explicit. Could > you post the patch? Sure [1]. Like a good lawyer, I can argue either way, even within the same case. ;) Sascha [1] mid:1469611466-31574-1-git-send-email-si...@linux.vnet.ibm.com "[PATCH] error: error_setg_errno(): errno gets preserved" by Sascha Silbe <si...@linux.vnet.ibm.com>, sent on 2016-07-27. -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
[Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved
C11 allows errno to be clobbered by pretty much any library function
call, so in general callers need to take care to save errno before
calling other functions.
However, for error reporting functions this is rather awkward and can
make the code on the caller side more complicated than
necessary. error_setg_errno() already takes care of preserving errno
and some functions rely on that, so just promise that we continue to
do so in the future.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
Alternative approach to "error: error_setg_errno(): errno may be
clobbered" [1].
[1] mid:1469558699-23314-1-git-send-email-si...@linux.vnet.ibm.com
"[Qemu-devel] [PATCH] error: error_setg_errno(): errno may be
clobbered" by Sascha Silbe <si...@linux.vnet.ibm.com>, sent on
2016-07-26.
include/qapi/error.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 0576659..7e532d0 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -170,6 +170,9 @@ void error_setg_internal(Error **errp,
* Just like error_setg(), with @os_error info added to the message.
* If @os_error is non-zero, ": " + strerror(os_error) is appended to
* the human-readable error message.
+ *
+ * The value of errno (which usually can get clobbered by almost any
+ * function call) will be preserved.
*/
#define error_setg_errno(errp, os_error, fmt, ...) \
error_setg_errno_internal((errp), __FILE__, __LINE__, __func__, \
--
1.9.1
Re: [Qemu-devel] [PATCH v2 1/1] block: improve error handling in raw_open
Dear Max, Max Reitz <mre...@redhat.com> writes: >> So how about: >> >> /* >> * Just like error_setg(), with @os_error info added to the message. >> * If @os_error is non-zero, ": " + strerror(os_error) is appended to >> * the human-readable error message. >> + * >> + * Reminder: errno may get clobbered by almost any function call. If >> + * you need the value of errno for another purpose, save it before >> + * invoking error_setg_errno() (or any other function for that >> + * matter). >> */ >> #define error_setg_errno(errp, os_error, fmt, ...) \ >> error_setg_errno_internal((errp), __FILE__, __LINE__, __func__, \ >>(os_error), (fmt), ## __VA_ARGS__) >> >> (I can prepare a proper patch if you agree with the above.) > > Thanks, that sounds good to me. Great, sent out as a patch [1]. Sascha [1] mid:1469558699-23314-1-git-send-email-si...@linux.vnet.ibm.com "[PATCH] error: error_setg_errno(): errno may be clobbered" by Sascha Silbe <si...@linux.vnet.ibm.com>, sent on 2016-07-26. -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
[Qemu-devel] [PATCH] error: error_setg_errno(): errno may be clobbered
As a general policy, we want callers to save errno themselves. error_setg_internal() currently goes out of its way to preserve errno, so with the API documentation not mentioning it either way, callers might come to rely on the current behaviour of the implementation. Spell out that we don't want to make that promise. Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com> --- This came up during review of Halil's patch "block: improve error handling in raw_open" [1]. [1] mid:1469532873-78542-1-git-send-email-pa...@linux.vnet.ibm.com "[Qemu-devel] [PATCH v2 1/1] block: improve error handling in raw_open" by Halil Pasic <pa...@linux.vnet.ibm.com>, sent on 2016-07-26. include/qapi/error.h | 5 + 1 file changed, 5 insertions(+) diff --git a/include/qapi/error.h b/include/qapi/error.h index 0576659..e5417e9 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -170,6 +170,11 @@ void error_setg_internal(Error **errp, * Just like error_setg(), with @os_error info added to the message. * If @os_error is non-zero, ": " + strerror(os_error) is appended to * the human-readable error message. + * + * Reminder: errno may get clobbered by almost any function call. If + * you need the value of errno for another purpose, save it before + * invoking error_setg_errno() (or any other function for that + * matter). */ #define error_setg_errno(errp, os_error, fmt, ...) \ error_setg_errno_internal((errp), __FILE__, __LINE__, __func__, \ -- 1.9.1
Re: [Qemu-devel] [PATCH v2 1/1] block: improve error handling in raw_open
Dear Max, Max Reitz <mre...@redhat.com> writes: > We don't guarantee that error_setg_errno() does not modify errno. (In > practice it should not, but we don't guarantee that.) I agree it's a good general policy to _not_ rely on the callee explicitly saving and restoring errno. Especially since C11 pretty much allows any "library function" (including e.g. va_start()) to clobber errno... In the case of error_setg_errno() it would be helpful to mention that in the API docs. The current implementation goes out of its way to preserve errno, so callers might come to rely on it. So how about: /* * Just like error_setg(), with @os_error info added to the message. * If @os_error is non-zero, ": " + strerror(os_error) is appended to * the human-readable error message. + * + * Reminder: errno may get clobbered by almost any function call. If + * you need the value of errno for another purpose, save it before + * invoking error_setg_errno() (or any other function for that + * matter). */ #define error_setg_errno(errp, os_error, fmt, ...) \ error_setg_errno_internal((errp), __FILE__, __LINE__, __func__, \ (os_error), (fmt), ## __VA_ARGS__) (I can prepare a proper patch if you agree with the above.) Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
[Qemu-devel] [PATCH] test-logging: don't hard-code paths in /tmp
Since f6880b7f [qemu-log: support simple pid substitution for logs],
test-logging creates files with hard-coded names in /tmp. In the best
case, this prevents multiple developers from running "make check" on
the same machine. In the worst case, it allows for symlink attacks,
enabling an attacker to overwrite files that are writable to the
developer running "make check".
Instead of hard-coding the paths, create a temporary directory using
g_dir_make_tmp() and clean it up afterwards.
Fixes: f6880b7f ("qemu-log: support simple pid substitution for logs")
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
tests/test-logging.c | 42 +++---
1 file changed, 35 insertions(+), 7 deletions(-)
diff --git a/tests/test-logging.c b/tests/test-logging.c
index cdf13c6..faebc35 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -86,24 +86,52 @@ static void test_parse_range(void)
error_free_or_abort();
}
-static void test_parse_path(void)
+static void test_parse_path(gconstpointer data)
{
+gchar const *tmp_path = data;
+gchar *plain_path = g_build_filename(tmp_path, "qemu.log", NULL);
+gchar *pid_infix_path = g_build_filename(tmp_path, "qemu-%d.log", NULL);
+gchar *pid_suffix_path = g_build_filename(tmp_path, "qemu.log.%d", NULL);
+gchar *double_pid_path = g_build_filename(tmp_path, "qemu-%d%d.log", NULL);
Error *err = NULL;
-qemu_set_log_filename("/tmp/qemu.log", _abort);
-qemu_set_log_filename("/tmp/qemu-%d.log", _abort);
-qemu_set_log_filename("/tmp/qemu.log.%d", _abort);
+qemu_set_log_filename(plain_path, _abort);
+qemu_set_log_filename(pid_infix_path, _abort);
+qemu_set_log_filename(pid_suffix_path, _abort);
-qemu_set_log_filename("/tmp/qemu-%d%d.log", );
+qemu_set_log_filename(double_pid_path, );
error_free_or_abort();
+
+g_free(double_pid_path);
+g_free(pid_suffix_path);
+g_free(pid_infix_path);
+g_free(plain_path);
+}
+
+static void rmtree(gchar const *root)
+{
+/* There should really be a g_rmtree(). Implementing it ourselves
+ * isn't really worth it just for a test, so let's just use rm. */
+gchar const *rm_args[] = { "rm", "-rf", root, NULL };
+g_spawn_sync(NULL, (gchar **)rm_args, NULL,
+ G_SPAWN_SEARCH_PATH, NULL, NULL,
+ NULL, NULL, NULL, NULL);
}
int main(int argc, char **argv)
{
+gchar *tmp_path = g_dir_make_tmp("qemu-test-logging.XX", NULL);
+int rc;
+
g_test_init(, , NULL);
+g_assert_nonnull(tmp_path);
g_test_add_func("/logging/parse_range", test_parse_range);
-g_test_add_func("/logging/parse_path", test_parse_path);
+g_test_add_data_func("/logging/parse_path", tmp_path, test_parse_path);
-return g_test_run();
+rc = g_test_run();
+
+rmtree(tmp_path);
+g_free(tmp_path);
+return rc;
}
--
1.9.1
Re: [Qemu-devel] [PATCH 1/1] Improve block job rate limiting for small bandwidth values
Dear Max,
Max Reitz <mre...@redhat.com> writes:
> [ Good signature by key: 0x58B381CE2DC89CF99730EE643BB14202E838ACAD ]
Feel free to drop by if you happen to be in the Stuttgart area some
time. PGP key signing, a beverage of your choice and optionally some
chatting about qemu and related topics. :)
> On 04.07.2016 16:30, Sascha Silbe wrote:
>> Max Reitz <mre...@redhat.com> writes:
[...]
[include/qemu/ratelimit.h]
>>>> static inline int64_t ratelimit_calculate_delay(RateLimit *limit,
>>>> uint64_t n)
>>>> {
>>>> int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>>>> +uint64_t delay_slices;
>>>>
>>>> -if (limit->next_slice_time < now) {
>>>> -limit->next_slice_time = now + limit->slice_ns;
>>>> +assert(limit->slice_quota && limit->slice_ns);
>>>> +
>>>> +if (limit->slice_end_time < now) {
>>>> +/* Previous, possibly extended, time slice finished; reset the
>>>> + * accounting. */
>>>> +limit->slice_start_time = now;
>>>> +limit->slice_end_time = now + limit->slice_ns;
>>>> limit->dispatched = 0;
>>>> }
>>>> -if (limit->dispatched == 0 || limit->dispatched + n <=
>>>> limit->slice_quota) {
>>>> -limit->dispatched += n;
>>>> +
>>>> +limit->dispatched += n;
>>>> +if (limit->dispatched < limit->slice_quota) {
>>>
>>> Nitpick: This should probably stay <=.
>>
>> This is a subtle edge case. Previously, when limit->dispatched ==
>> limit->slice_quota, we returned 0 so that the _current_ request (which
>> is still within quota) wouldn't be delayed. Now, we return a delay so
>> that the _next_ request (which would be over quota) will be delayed.
>
> Hm, but that depends on the size of the next request. Of course, if we
> get limit->dispatched == limit->slice_quota we know for sure that we
> need to delay the next request. But if we get limit->dispatched ==
> limit->slice_quota - 1... Then we probably also have to delay it, but we
> don't know for sure.
No matter where exactly we draw the line, due to the way the block job
rate limiting works (fixed size time slices, fixed size requests) there
will always be cases where we're off the target rate quite a bit, in one
or the other direction.
For rate limits where we can send an integer number of chunks per time
slice (i.e. some MiB/s sized value), the "<" condition is probably
better. We'll send out a couple of chunks that exactly match the quota,
then sleep for the rest of the time slice. If we'd use "<=", we'd send
out one extra chunk before we start sleeping.
But I don't care much either way, "<=" is fine with me, too.
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH RFC v3 1/5] tests: New make target check-source
Dear Markus, I looked into the s390x specific failures as it would be nice to get them fixed up right away. Fortunately it looks like all of them are related to known shortcomings of the header check rather than actual issues with the header files, so there's nothing for me to do. ;) Markus Armbruster <arm...@redhat.com> writes: [...] > Some headers fail the test because they need to be compiled with > -DNEED_CPU_H, and testing them is not yet implemented. They are > marked with a comment > > /* NOTE: May only be included into target-dependent code */ The following header files either include target-s390x/cpu.h themselves or expect it to have been included already so they should be marked with the above note rather than the FIXME: - hw/s390x/ipl.h - hw/s390x/s390-pci-inst.h - hw/s390x/s390-virtio.h [linux-headers/asm-s390/kvm.h] > +/* FIXME Does not pass make check-headers with CONFIG_WIN32, yet! */ Is this because of the linux/types.h include or is there some other issue? Should we even check these imported headers? [linux-user/host/*/hostdep.h] > +/* FIXME Does not pass make check-headers, yet! */ The "struct ucontext" definition used by hostdep.h differs between host architectures. Not sure if testing the headers for "foreign" host architectures is possible; we should probably just skip them instead of marking them with a FIXME. Many or even most other non-softmmu target failures smell like NEED_CPU_H. [*/helper.h] > +/* FIXME Does not pass make check-headers, yet! */ Probably never will. Looks like it's meant to be included only by some magic preprocessor code. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH] quorum: Only compile when supported
Dear Alberto, Alberto Garcia <be...@igalia.com> writes: > On Tue 05 Jul 2016 09:58:25 AM CEST, Sascha Silbe wrote: > >> The quorum driver needs SHA256 which was introduced in gnutls 2.11.1. > > Are you sure about that? > > * Version 1.7.4 (released 2007-02-05) > > [...] > > ** API and ABI modifications: > GNUTLS_MAC_SHA256, [...] You are right, I misinterpreted the gnutls 2.11.1 NEWS entry. It added the RSA_NULL_SHA256 cipher suite, not the SHA256 hash algorithm. So the existing configure check is good enough, no need to bump the required gnutls version. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH] quorum: Only compile when supported
Dear Fam (or Zheng?),
Fam Zheng <f...@redhat.com> writes:
> This was the only exceptional module init function that does something
> else than a simple list of bdrv_register() calls, in all the block
> drivers.
>
> The qcrypto_hash_supports is actually a static check, determined at
> compile time. Follow the block-job-$(CONFIG_FOO) convention for
> consistency.
Good idea.
[block/Makefile.objs]
> @@ -3,7 +3,7 @@ block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o
> qcow2-snapshot.o qcow2-c
> block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
> block-obj-y += qed-check.o
> block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
> -block-obj-y += quorum.o
> +block-obj-$(CONFIG_GNUTLS_HASH) += quorum.o
[...]
[block/quorum.c]
> static void bdrv_quorum_init(void)
> {
> -if (!qcrypto_hash_supports(QCRYPTO_HASH_ALG_SHA256)) {
> -/* SHA256 hash support is required for quorum device */
> -return;
> -}
> bdrv_register(_quorum);
The quorum driver needs SHA256 which was introduced in gnutls
2.11.1. However configure sets CONFIG_GNUTLS_HASH when gnutls 2.9.10+ is
present. You should either bump the version in configure or add an
explicit configure check for SHA256.
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH 1/1] Improve block job rate limiting for small bandwidth values
Dear Max,
Max Reitz <mre...@redhat.com> writes:
> On 28.06.2016 17:28, Sascha Silbe wrote:
[block/mirror.c]
>> @@ -416,7 +416,9 @@ static uint64_t coroutine_fn
>> mirror_iteration(MirrorBlockJob *s)
>> assert(io_sectors);
>> sector_num += io_sectors;
>> nb_chunks -= DIV_ROUND_UP(io_sectors, sectors_per_chunk);
>> -delay_ns += ratelimit_calculate_delay(>limit, io_sectors);
>> +if (s->common.speed) {
>> +delay_ns = ratelimit_calculate_delay(>limit, io_sectors);
>> +}
>
> Hmm... Was it a bug that ratelimit_calculate_delay() was called
> unconditionally before?
One could argue either way. It happened to work because
ratelimit_calculate_delay() only delayed the _second_ time (within one
time slice) the quota was exceeded. With zero duration time slices,
there never was a second time.
With the new implementation we would divide by zero when slice_quota is
0, so we need to guard against that. Most callers already did, only
mirror_iteration() needed to be adjusted.
[...]
[include/qemu/ratelimit.h]
>> static inline int64_t ratelimit_calculate_delay(RateLimit *limit, uint64_t
>> n)
>> {
>> int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>> +uint64_t delay_slices;
>>
>> -if (limit->next_slice_time < now) {
>> -limit->next_slice_time = now + limit->slice_ns;
>> +assert(limit->slice_quota && limit->slice_ns);
>> +
>> +if (limit->slice_end_time < now) {
>> +/* Previous, possibly extended, time slice finished; reset the
>> + * accounting. */
>> +limit->slice_start_time = now;
>> +limit->slice_end_time = now + limit->slice_ns;
>> limit->dispatched = 0;
>> }
>> -if (limit->dispatched == 0 || limit->dispatched + n <=
>> limit->slice_quota) {
>> -limit->dispatched += n;
>> +
>> +limit->dispatched += n;
>> +if (limit->dispatched < limit->slice_quota) {
>
> Nitpick: This should probably stay <=.
This is a subtle edge case. Previously, when limit->dispatched ==
limit->slice_quota, we returned 0 so that the _current_ request (which
is still within quota) wouldn't be delayed. Now, we return a delay so
that the _next_ request (which would be over quota) will be delayed.
[...]
>> +/* Quota exceeded. Calculate the next time slice we may start
>> + * sending data again. */
>> +delay_slices = (limit->dispatched + limit->slice_quota - 1) /
>> +limit->slice_quota;
>> +limit->slice_end_time = limit->slice_start_time +
>> +delay_slices * limit->slice_ns;
>
> I think it would make sense to make this a floating point calculation.
Then we'd have fully variable length time slices, instead of just
"occupying" multiple fixed-length time slices with a single
request. Maybe that would be even better, or maybe we'd cause other
interesting things to happen (think interactions with the scheduler). As
this code will hopefully disappear during the 2.8 time line anyway, I'd
prefer to go with the lowest risk option that is enough to fix the race
conditions encountered by the test suite.
> If you don't agree, though:
>
> Reviewed-by: Max Reitz <mre...@redhat.com>
Thanks for the review!
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH RFC] fixup! tests: New make target check-source
Dear Markus, Markus Armbruster <arm...@redhat.com> writes: > Markus Armbruster <arm...@redhat.com> writes: > >> Hmm, this demonstrates some of our headers may only be included when >> certain CONFIG_* are defined. [...] >> Regardless, we need to find the problemtatic headers. Perhaps you can >> find a few more with "make -k check-source". > > I think I tracked them down. Not too bad, just a dozen or so. Now I > have to make up my mind whether I prefer to document their configuration > requirements with comments or with ifdefs. FWIW, I'd prefer a solution that allows successfully running "make check-source" even on hosts where not all optional dependencies are installed. It would be useful as a sanity check before sending patches. If "make check-source" doesn't work on the developers primary machine because they cannot install all the latest experimental dependencies, fewer submitters will use it. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH RFC] fixup! tests: New make target check-source
Dear Markus, Markus Armbruster <arm...@redhat.com> writes: > --- > tests/header-test-template.c | 16 [...] Thanks, that helped, I get a bit further now. Is "make header-check" supposed to work on a host that doesn't have all optional dependencies installed? It fails for me because some OpenGL related header is missing. configure correctly detected that and didn't enable OpenGL support: $ make check-headers CCtests/headers/include/ui/shader.o In file included from tests/headers/include/ui/shader.c:14:0: ./include/ui/shader.h:6:22: fatal error: epoxy/gl.h: No such file or directory #include ^ compilation terminated. make: *** [tests/headers/include/ui/shader.o] Error 1 rm tests/headers/include/ui/shader.c $ grep OPENGL config-host.* Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
[Qemu-devel] [PATCH 1/1] Improve block job rate limiting for small bandwidth values
ratelimit_calculate_delay() previously reset the accounting every time
slice, no matter how much data had been processed before. This had (at
least) two consequences:
1. The minimum speed is rather large, e.g. 5 MiB/s for commit and stream.
Not sure if there are real-world use cases where this would be a
problem. Mirroring and backup over a slow link (e.g. DSL) would
come to mind, though.
2. Tests for block job operations (e.g. cancel) were rather racy
All block jobs currently use a time slice of 100ms. That's a
reasonable value to get smooth output during regular
operation. However this also meant that the state of block jobs
changed every 100ms, no matter how low the configured limit was. On
busy hosts, qemu often transferred additional chunks until the test
case had a chance to cancel the job.
Fix the block job rate limit code to delay for more than one time
slice to address the above issues. To make it easier to handle
oversized chunks we switch the semantics from returning a delay
_before_ the current request to a delay _after_ the current
request. If necessary, this delay consists of multiple time slice
units.
Since the mirror job sends multiple chunks in one go even if the rate
limit was exceeded in between, we need to keep track of the start of
the current time slice so we can correctly re-compute the delay for
the updated amount of data.
The minimum bandwidth now is 1 data unit per time slice. The block
jobs are currently passing the amount of data transferred in sectors
and using 100ms time slices, so this translates to 5120
bytes/second. With chunk sizes usually being O(512KiB), tests have
plenty of time (O(100s)) to operate on block jobs. The chance of a
race condition now is fairly remote, except possibly on insanely
loaded systems.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
---
block/commit.c | 13 +
block/mirror.c | 4 +++-
block/stream.c | 12
include/qemu/ratelimit.h | 43 ++-
4 files changed, 46 insertions(+), 26 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index 444333b..da48673 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -113,6 +113,7 @@ static void coroutine_fn commit_run(void *opaque)
CommitBlockJob *s = opaque;
CommitCompleteData *data;
int64_t sector_num, end;
+uint64_t delay_ns = 0;
int ret = 0;
int n = 0;
void *buf = NULL;
@@ -142,10 +143,8 @@ static void coroutine_fn commit_run(void *opaque)
buf = blk_blockalign(s->top, COMMIT_BUFFER_SIZE);
for (sector_num = 0; sector_num < end; sector_num += n) {
-uint64_t delay_ns = 0;
bool copy;
-wait:
/* Note that even when no rate limit is applied we need to yield
* with no pending I/O here so that bdrv_drain_all() returns.
*/
@@ -161,12 +160,6 @@ wait:
copy = (ret == 1);
trace_commit_one_iteration(s, sector_num, n, ret);
if (copy) {
-if (s->common.speed) {
-delay_ns = ratelimit_calculate_delay(>limit, n);
-if (delay_ns > 0) {
-goto wait;
-}
-}
ret = commit_populate(s->top, s->base, sector_num, n, buf);
bytes_written += n * BDRV_SECTOR_SIZE;
}
@@ -182,6 +175,10 @@ wait:
}
/* Publish progress */
s->common.offset += n * BDRV_SECTOR_SIZE;
+
+if (copy && s->common.speed) {
+delay_ns = ratelimit_calculate_delay(>limit, n);
+}
}
ret = 0;
diff --git a/block/mirror.c b/block/mirror.c
index a04ed9c..d9d70f0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -416,7 +416,9 @@ static uint64_t coroutine_fn
mirror_iteration(MirrorBlockJob *s)
assert(io_sectors);
sector_num += io_sectors;
nb_chunks -= DIV_ROUND_UP(io_sectors, sectors_per_chunk);
-delay_ns += ratelimit_calculate_delay(>limit, io_sectors);
+if (s->common.speed) {
+delay_ns = ratelimit_calculate_delay(>limit, io_sectors);
+}
}
return delay_ns;
}
diff --git a/block/stream.c b/block/stream.c
index c0efbda..8a09c48 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -95,6 +95,7 @@ static void coroutine_fn stream_run(void *opaque)
BlockDriverState *base = s->base;
int64_t sector_num = 0;
int64_t end = -1;
+uint64_t delay_ns = 0;
int error = 0;
int ret = 0;
int n = 0;
@@ -123,10 +124,8 @@ static void coroutine_fn stream_run(void *opaque)
}
for (sector_num = 0; sector_num < end; sector_num += n) {
-uint64_t delay_ns = 0;
bool copy;
-wait:
/* Note that even when no rate limit is applied we need to yield
* with no pending I/O here so that bdrv_drain_all() returns.
*/
@@ -156,12 +155,6 @@ wait:
[Qemu-devel] [PATCH 0/1] Fix iotests race condition by fixing block job rate limiting
qemu-iotests #141 is relying on the test being able to operate on a block job it just started before further progress is being made on this block job. This fails regularly on some hosts because the time slice is just 100ms and it often takes longer than that to start the additional processes required to trigger the operation. It's particularly easy to reproduce under 100% CPU load. I originally noticed and analysed this during 2.6 hard freeze. Eventually the legacy rate limiting code currently used by the block jobs will be replaced by the refactorings to use BlockBackends which have their own rate limiting implementation. There was some hope [1] this would land in 2.7, but since it's not in master yet (at least as of commit a01aef5d) I prepared an alternative fix that can go into 2.7. Sascha Silbe (1): Improve block job rate limiting for small bandwidth values block/commit.c | 13 + block/mirror.c | 4 +++- block/stream.c | 12 include/qemu/ratelimit.h | 43 ++- 4 files changed, 46 insertions(+), 26 deletions(-) [1] mid:20160408123115.gh4...@noname.redhat.com "Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qemu-iotests: 141: reduce likelihood of race condition on systems with fast IO" by Kevin Wolf <kw...@redhat.com> on 2016-04-08. -- 1.9.1
Re: [Qemu-devel] [PATCH RFC v2 0/5] Baby steps towards saner headers
Dear Paolo, Paolo Bonzini <pbonz...@redhat.com> writes: >> After applying your series on top of f12103af and running "./configure" >> in a clean working directory, I get the following errors for "make >> check-source": >> >> $ make check-source >> egrep: config-host.h: No such file or directory >> egrep: qmp-commands.h: No such file or directory [...] >> GEN aarch64-softmmu/config-devices.mak.tmp >> GEN aarch64-softmmu/config-devices.mak >> [...] >> GEN tests/test-qmp-introspect.h >> make: *** No rule to make target `tests/headers/audio/audio.o', needed by >> `check-headers'. Stop. > > Hi Sascha, > > these are all generated headers. In general, "make check" must be run > after "make", and I suppose the same holds for "make check-headers". Still no dice. The egrep error messages don't occur when running "make check-source" after a full "make" run, but the audio.o error still happens: $ make check-source make: *** No rule to make target `tests/headers/audio/audio.o', needed by `check-headers'. Stop. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH RFC v2 0/5] Baby steps towards saner headers
Dear Markus, Markus Armbruster <arm...@redhat.com> writes: > Some time ago, we discussed rules for headers, and these were > generally liked: [...] > Trouble is we're not exactly close to obeying 2. This series > demonstrates a possible path towards obeying it: enforce it in "make > check", except for known-bad headers [PATCH 2]. We start with a large > list of known-bad headers, then whittle it down. Some sample > whittling in PATCH 4+5. [...] Does this series depend on any other series that's not in master yet? After applying your series on top of f12103af and running "./configure" in a clean working directory, I get the following errors for "make check-source": $ make check-source egrep: config-host.h: No such file or directory egrep: qmp-commands.h: No such file or directory egrep: qapi-types.h: No such file or directory egrep: qapi-visit.h: No such file or directory egrep: qapi-event.h: No such file or directory egrep: qmp-introspect.h: No such file or directory egrep: trace/generated-events.h: No such file or directory egrep: trace/generated-tracers.h: No such file or directory egrep: trace/generated-tcg-tracers.h: No such file or directory egrep: trace/generated-helpers-wrappers.h: No such file or directory egrep: trace/generated-helpers.h: No such file or directory egrep: tests/test-qapi-types.h: No such file or directory egrep: tests/test-qapi-visit.h: No such file or directory egrep: tests/test-qmp-commands.h: No such file or directory egrep: tests/test-qapi-event.h: No such file or directory egrep: tests/test-qmp-introspect.h: No such file or directory GEN aarch64-softmmu/config-devices.mak.tmp GEN aarch64-softmmu/config-devices.mak [...] GEN tests/test-qmp-introspect.h make: *** No rule to make target `tests/headers/audio/audio.o', needed by `check-headers'. Stop. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] Channel paths
Dear Daniel, [CC += libvir-list, didn't notice before this thread is on qemu-devel only] "Daniel P. Berrange" <berra...@redhat.com> writes: > On Thu, Jun 02, 2016 at 09:27:45PM +0200, Sascha Silbe wrote: >> Since 71408079 [qemu: Don't bother user with libvirt-internal paths], >> the path chosen by libvirt isn't included in the domain XML anymore. So >> now I need to choose the path inside the application. The only safe way >> to do that is by using something in an application-managed namespace >> (e.g. /var/lib/myapp/foo or /tmp/myapp/foo); I certainly wouldn't want >> to second guess what paths would be safe inside the libvirt namespace >> (/var/lib/libvirt/qemu). Except now I hear that anything outside >> /var/lib/libvirt/qemu is not guaranteed to work due to e.g. the SELinux >> policy configured by libvirt... > > IIUC that change 71408079 ought to only apply to the persistent XML > configuration on disk. When the guest is running the live XML ought > to still report the path libvirt chose, so you should still be able > to see it while running. If that isn't the case then please raise a > bug, because we certainly expect apps to be able to discover the > path libvirt picked for exactly the reason you describe OK, this makes a lot more sense now (persistent vs. live). Just tried it with current libvirt master and it works, thanks. It even worked with 71408079 itself. I'm pretty sure things were broken some time in between, but a few quick probes didn't turn up a faulty version and I'm not curious enough to do a linear search. Cannot completely rule out other effects having played a role, either. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
[Qemu-devel] Channel paths (was: Re: [RFC Patch 0/3] Accept passed in socket 'fd' open from outside for unix socket)
Dear Daniel, "Daniel P. Berrange" <berra...@redhat.com> writes: > On Thu, Jun 02, 2016 at 09:41:56AM +0200, Michal Privoznik wrote: >> On 01.06.2016 18:16, Wei Xu wrote: >> > On 2016年06月01日 00:44, Daniel P. Berrange wrote: > There are plenty of other places where we allow arbitrary paths in the > XML, but which have restrictions imposed by the security drivers. Not > least the devices which have the exact same scenario as this > network device, and require use of /var/lib/libvirt/qemu as the directory > for the sockets. We certainly do not want to allow QEMU to create sockets > anywhere. Umm, how exactly is an application supposed to use (i.e. open) the channel devices defined in XML? Previously I deliberately left out the path in the XML to let libvirt choose the path and extracted it from the XML after defining the domain. This ensured qemu could access the path, plus it was the responsibility of libvirt to clean it up once the domain was undefined. Easy and simple. Since 71408079 [qemu: Don't bother user with libvirt-internal paths], the path chosen by libvirt isn't included in the domain XML anymore. So now I need to choose the path inside the application. The only safe way to do that is by using something in an application-managed namespace (e.g. /var/lib/myapp/foo or /tmp/myapp/foo); I certainly wouldn't want to second guess what paths would be safe inside the libvirt namespace (/var/lib/libvirt/qemu). Except now I hear that anything outside /var/lib/libvirt/qemu is not guaranteed to work due to e.g. the SELinux policy configured by libvirt... Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
Dear Jianjun,
Jianjun Duan <du...@linux.vnet.ibm.com> writes:
[include/migration/vmstate.h]
> @@ -185,6 +185,8 @@ enum VMStateFlags {
> * to determine the number of entries in the array. Only valid in
> * combination with one of VMS_VARRAY*. */
> VMS_MULTIPLY_ELEMENTS = 0x4000,
> +/* For fields which need customized handling, such as QTAILQ in queue.h*/
> +VMS_CSTM= 0x8000,
Can you describe (in the comment) how this customised handling is
performed, please? I.e. describe what exactly happens if the flag is
set, from the point of view of an API consumer.
Also, why do you need this flag at all? The only change I can see is
that you pass additional information to VMStateInfo.get() / .put(),
using NULL if it's not set. Why don't you just always pass the
additional information? If the additional information is not needed by
get() / put() the parameter will be unused anyway.
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH for-2.6?] qemu-iotests: iotests: fail hard if not run via "check"
Dear Kevin, Kevin Wolf <kw...@redhat.com> writes: > At this point in the 2.6 release cycle, this series is 2.7 material > anyway. It's critical fixes only now. Fair enough. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH for-2.6?] qemu-iotests: iotests: fail hard if not run via "check"
Dear Markus, Markus Armbruster <arm...@redhat.com> writes: > 'Please run this test via the "check" script' isn't an error message, > it's advice. Advice is appreciated, but an error message must come > first. I reiterate my proposal to use something like > > : TEST_DIR and QEMU_DEFAULT_MACHINE must be set in > environment > The easiest way to do that is running the test via the check script. But I don't know how the environment (again, broader sense than just environment variables) must be set up; that's part of the problem. I know that at least the TEST_DIR and QEMU_DEFAULT_MACHINE environment variables must be set, but listing those in the error message gives an impression that it's all the user needs to do. Bad advice is worse than no advice. We could write something like "The TEST_DIR and QEMU_DEFAULT_MACHINE environment variables must be set and there may be other things that the tests may expect to have been set up. See what the 'check' script does if you want to invoke the tests some other way than running 'check'.", but I don't really see the additional value to the user. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [for-2.6] Re: [PATCH] qmp-commands.hx: document minimum speed for block jobs
Dear Eric, Eric Blake <ebl...@redhat.com> writes: [...] > Ultimately, we want to make qmp-commands.hx go away, and have everything > generated from qapi/*.json. Marc-Andre has proposed some patches along > those lines (back in the 2.5 timeframe, but dependent on other qapi > patches that are still pending review). OK, thanks for the clarification! Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
[Qemu-devel] [PATCH for-2.6? v2] qemu-iotests: iotests: fail hard if not run via "check"
Running an iotests-based Python test directly might appear to work,
but may fail in subtle ways and is insecure:
- It creates files with predictable file names in a world-writable
location (/var/tmp).
- Tests expect the environment to be set up by check. E.g. 041 and 055
may take the wrong code paths if QEMU_DEFAULT_MACHINE is not
set. This can lead to false negatives.
Instead fail hard and tell the user we want to be run via "check".
The actual environment expected by the tests is currently only defined
by the implementation of "check". We use two of the environment
variables set by "check" as indication of whether we're being run via
"check". Anyone writing their own test runner (replacing "check") will
need to replicate the full environment (in a broader sense, not just
environment variables) provided by "check" anyway, including setting
the two environment variables we check. Whereas a regular developer
just trying to invoke the tests usually won't have both of these
defined in their environment so we can catch their mistake and give
out useful advice.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
Reviewed-by: Bo Tu <t...@linux.vnet.ibm.com>
---
v1→v2:
- Add comment indicating that it's not just TEST_DIR and
QEMU_DEFAULT_MACHINE that need to be set. Amend commit message in
a similar way.
@Tu Bo: I'm assuming your Reviewed-By: still stands as I only added
more information on the background of the change and the check.
---
tests/qemu-iotests/iotests.py | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0c0b533..054b482 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -46,7 +46,7 @@ if os.environ.get('QEMU_OPTIONS'):
imgfmt = os.environ.get('IMGFMT', 'raw')
imgproto = os.environ.get('IMGPROTO', 'file')
-test_dir = os.environ.get('TEST_DIR', '/var/tmp')
+test_dir = os.environ.get('TEST_DIR')
output_dir = os.environ.get('OUTPUT_DIR', '.')
cachemode = os.environ.get('CACHEMODE')
qemu_default_machine = os.environ.get('QEMU_DEFAULT_MACHINE')
@@ -441,6 +441,14 @@ def verify_quorum():
def main(supported_fmts=[], supported_oses=['linux']):
'''Run tests'''
+# We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
+# indicate that we're not being run via "check". There may be
+# other things set up by "check" that individual test cases rely
+# on.
+if test_dir is None or qemu_default_machine is None:
+sys.stderr.write('Please run this test via the "check" script\n')
+sys.exit(os.EX_USAGE)
+
debug = '-d' in sys.argv
verbosity = 1
verify_image_format(supported_fmts)
--
1.9.1
Re: [Qemu-devel] [PATCH for-2.6?] qemu-iotests: iotests: fail hard if not run via "check"
Dear Max,
Max Reitz <mre...@redhat.com> writes:
> On 19.04.2016 14:22, Sascha Silbe wrote:
[...]
>> # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
>> # indicate that we're not being run via "check". There may be
>> # other things set up by "check" that individual test cases rely
>> # on.
>> if test_dir is None or qemu_default_machine is None:
>> sys.stderr.write('Please run this test via the "check" script\n')
>> sys.exit(os.EX_USAGE)
>
> I'm OK with that. I can imagine Markus isn't, because it's implying that
> you should only run this test through "check", whereas Markus says that
> maybe you have your own script and that is fine, too.
[...]
> 2. If you do want your own script, I guess setting up the necessary
> environment for each test is complicated enough to require you to know
> what you're doing. And if you know what you're doing, you won't really
> care about the wording of this warning anyway.
>
> I think this is just a warning for unwary users who just try to execute
> a python test directly because they think that maybe they can. [...]
Seems we're on the same page. For those who want to write their own test
runners, the comment in the source indicates why we recommend running
via "check", implying that they'd need to set up the environment just
like "check" does. That's more or less the best we can do in the current
situation, so I'll spin up a v2 with the above code block. If Markus
would like it explicitly spelled out in the comment or the error
message, that'd be fine with me, too. Feel free to amend locally before
merging in that case.
[...]
> Regarding b): If you separate the build tree from the source tree, you
> have to run the check script from the build tree. During the build
> process, qemu will in fact automatically create a symlink in the build
> tree. Therefore, in that case, you don't want to execute "./check" in
> the same directory as the test is in.
Whereas there probably are no symlinks to the individual test
cases. Good point.
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [for-2.6] Re: [PATCH] qmp-commands.hx: document minimum speed for block jobs
Dear Kevin, Kevin Wolf <kw...@redhat.com> writes: [...] > Isn't the JSON schema (qapi/block-core.json) considered the > authoritative source for interface specifications these days? I think if > we want to document the shortcomings for the time being, we should > document it in both places. Interesting. What exactly is the relationship between qmp-commands.hx and qapi/block-core.json? At any rate, you're right that we should update both. See v2. (And I've forgot the for-2.6 prefix for the patch itself again. Bah.) Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
[Qemu-devel] [PATCH v2] QMP: document minimum speed for block jobs
The current rate limit implementation for block jobs is ineffective below a certain minimum rate. It will permit writes at least once per time slice. The resulting minimum write speed (assuming source and sink are fast enough in the first place) is high enough that it may surprise some users, so document it. Mention that this will be fixed in the future, otherwise some users might misguidedly rely on it or clamp their configuration settings to the documented value. Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com> --- v1→v2: - Change qapi/block-core.json, too. --- qapi/block-core.json | 36 +++- qmp-commands.hx | 36 2 files changed, 55 insertions(+), 17 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index 1d09079..1bc78cc 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -882,7 +882,10 @@ # @mode: #optional whether and how QEMU should create a new image, default is #'absolute-paths'. # -# @speed: #optional the maximum speed, in bytes per second +# @speed: #optional The maximum speed, in bytes per second. Note: The current +# implementation will write at a minimum speed that depends on device +# and format, even if a lower speed is configured. This will be fixed +# in the future. # # @bitmap: #optional the name of dirty bitmap if sync is "incremental". # Must be present if sync is "incremental", must NOT be present @@ -920,8 +923,10 @@ #(all the disk, only the sectors allocated in the topmost image, or #only new I/O). # -# @speed: #optional the maximum speed, in bytes per second. The default is 0, -# for unlimited. +# @speed: #optional The maximum speed, in bytes per second. The default is 0, +# for unlimited. Note: The current implementation will write at a +# minimum speed that depends on device and format, even if a lower +# speed is configured. This will be fixed in the future. # # @on-source-error: #optional the action to take on an error on the source, # default 'report'. 'stop' and 'enospc' can only be used @@ -1042,7 +1047,9 @@ #size of the smaller top, you can safely truncate it #yourself once the commit operation successfully completes. # -# @speed: #optional the maximum speed, in bytes per second +# @speed: #optional The maximum speed, in bytes per second. Note: In the +# current implementation, at least 5MiB/s will be written, even if a +# lower speed has been set. This will be fixed in the future. # # Returns: Nothing on success # If commit or stream is already active on this device, DeviceInUse @@ -1127,7 +1134,11 @@ # @mode: #optional whether and how QEMU should create a new image, default is #'absolute-paths'. # -# @speed: #optional the maximum speed, in bytes per second +# @speed: #optional The maximum speed, in bytes per second. Note: In the +# current implementation, the buffer will be written at least once per +# 100ms. So with the default buffer size of 10MiB, at least 10MiB/s +# will be written, even if a lower speed is configured. This will be +# fixed in the future. # # @sync: what parts of the disk image should be copied to the destination #(all the disk, only the sectors allocated in the topmost image, or @@ -1252,7 +1263,11 @@ #image when a whole image copy is done. This can be used to repair #broken Quorum files. # -# @speed: #optional the maximum speed, in bytes per second +# @speed: #optional The maximum speed, in bytes per second. Note: In the +# current implementation, the buffer will be written at least once per +# 100ms. So with the default buffer size of 10MiB, at least 10MiB/s +# will be written, even if a lower speed is configured. This will be +# fixed in the future. # # @sync: what parts of the disk image should be copied to the destination #(all the disk, only the sectors allocated in the topmost image, or @@ -1432,7 +1447,9 @@ # protocol. # (Since 2.1) # -# @speed: #optional the maximum speed, in bytes per second +# @speed: #optional The maximum speed, in bytes per second. Note: In the +# current implementation, at least 5MiB/s will be written, even if a +# lower speed has been set. This will be fixed in the future. # # @on-error: #optional the action to take on an error (default report). #'stop' and 'enospc' can only be used if the block device @@ -1458,8 +1475,9 @@ # # @device: the device name # -# @speed: the maximum speed, in bytes per second, or 0 for unlimited. -# Defaults to 0. +# @speed: The maximum speed, in bytes per second, or 0 for unlimited. Defaults +# to 0. See the documentation of the i
Re: [Qemu-devel] [PATCH for-2.6?] qemu-iotests: iotests: fail hard if not run via "check"
Dear Markus, Markus Armbruster <arm...@redhat.com> writes: > Say you had an accurate way to find out whether we're running under > "check". You could then reject any attempt to run the test directly. > I'd oppose that. > > It's okay to have test wrapper scripts to configure the tests just so. > It's okay to tell people to use them. But "you can't do that, Dave" is > not okay. [...] AFAICT the environment in which the individual test cases run isn't well-defined. Currently it's indirectly defined by whatever "check" does. The goal of the patch is to catch unwary developers invoking the tests directly from the command line, providing them with useful advice. If somebody wants to write another test runner (in place of "check"), it's their responsibility to set up the environment appropriately. (They could even set an environment variable "I_AM_CHECK=yes" if that's part of the environment the tests expect). I'd be perfectly fine with defining the environment more clearly and possibly extending the implementation to allow individual test cases to be invoked directly (without a test runner like "check"). But that would be 2.7 material. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH for-2.6?] qemu-iotests: iotests: fail hard if not run via "check"
Dear Max,
Max Reitz <mre...@redhat.com> writes:
> On 14.04.2016 13:32, Sascha Silbe wrote:
[tests/qemu-iotests/iotests.py]
[...]
>> def main(supported_fmts=[], supported_oses=['linux']):
>> '''Run tests'''
>>
>> +if test_dir is None or qemu_default_machine is None:
>
> I think checking test_dir would have been sufficient; this makes it look
> like it might want to be a complete list of variables that have to be
> set, but then "cachemode" is missing.
Markus Armbruster basically pointed out the same issue, so how about
this version:
# We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
# indicate that we're not being run via "check". There may be
# other things set up by "check" that individual test cases rely
# on.
if test_dir is None or qemu_default_machine is None:
sys.stderr.write('Please run this test via the "check" script\n')
sys.exit(os.EX_USAGE)
>> +sys.stderr.write('Please run this test via ./check\n')
>
> Or not ./, for people who have separate build and source trees, you
> don't want to invoke check in the source tree. ;-)
Yeah, was thinking about that, but ultimately considered ./check to be
the best indication of a) "check" being the name of a script and b)
residing in the same directory as the test. But I don't care much about
this, so see the above version.
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH for-2.6?] qemu-iotests: iotests: fail hard if not run via "check"
Dear Markus,
Markus Armbruster <arm...@redhat.com> writes:
>> Running an iotests-based Python test directly might appear to work,
>> but may fail in subtle ways and is insecure:
>>
>> - It creates files with predictable file names in a world-writable
>> location (/var/tmp).
>>
>> - Tests expect the environment to be set up by check. E.g. 041 and 055
>> may take the wrong code paths if QEMU_DEFAULT_MACHINE is not
>> set. This can lead to false negatives.
>>
>> Instead fail hard and tell the user we want to be run via "check".
>
> Are the problems serious enough to warrant rejecting the attempt to run
> the tests directly outright?
>
> Judging from your description, I'm inclined to "no".
Having tests do unexpected things and / or fail silently is "serious" in
my book. After all, what's the value of tests if they don't yell when
something is broken?
[tests/qemu-iotests/iotests.py]
[...]
>> +if test_dir is None or qemu_default_machine is None:
>> +sys.stderr.write('Please run this test via ./check\n')
>> +sys.exit(os.EX_USAGE)
>> +
>
> Aha, you're not actually rejecting the attempt to run the tests
> directly, you're merely requiring two environment variables. That's
> okay with me. [...]
I should probably add a comment that these two environment variables are
merely used as proxies that indicate we haven't been run via
"check". They are the ones where there isn't any useful default value we
could fall back, but without a full audit we can't tell what else some
of the tests may expect that would normally be set up by "check" (or at
least I can't).
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
Re: [Qemu-devel] [PATCH for-2.6 0/4] qemu-iotests: don't use /tmp
Dear Max, Max Reitz <mre...@redhat.com> writes: [...] > Thanks Sascha, I applied the series to my block tree: Thanks! I've just sent out the last patch from my local queue (fixing =/var/tmp= usage). Barring further discoveries during testing, I'm done for 2.6 now. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
[Qemu-devel] [PATCH for-2.6?] qemu-iotests: iotests: fail hard if not run via "check"
Running an iotests-based Python test directly might appear to work,
but may fail in subtle ways and is insecure:
- It creates files with predictable file names in a world-writable
location (/var/tmp).
- Tests expect the environment to be set up by check. E.g. 041 and 055
may take the wrong code paths if QEMU_DEFAULT_MACHINE is not
set. This can lead to false negatives.
Instead fail hard and tell the user we want to be run via "check".
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
Reviewed-by: Bo Tu <t...@linux.vnet.ibm.com>
---
It's possible to fix iotests.py to work even outside of check, but
that requires reimplementing parts of what check currently does. I'd
prefer not to do that this late in the cycle.
It would be nice to have this in 2.6, just in case someone even tries
running a Python-based test directly instead of using ./check like for
the shell-based tests. But it's not crucial.
---
tests/qemu-iotests/iotests.py | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0c0b533..8c7138f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -46,7 +46,7 @@ if os.environ.get('QEMU_OPTIONS'):
imgfmt = os.environ.get('IMGFMT', 'raw')
imgproto = os.environ.get('IMGPROTO', 'file')
-test_dir = os.environ.get('TEST_DIR', '/var/tmp')
+test_dir = os.environ.get('TEST_DIR')
output_dir = os.environ.get('OUTPUT_DIR', '.')
cachemode = os.environ.get('CACHEMODE')
qemu_default_machine = os.environ.get('QEMU_DEFAULT_MACHINE')
@@ -441,6 +441,10 @@ def verify_quorum():
def main(supported_fmts=[], supported_oses=['linux']):
'''Run tests'''
+if test_dir is None or qemu_default_machine is None:
+sys.stderr.write('Please run this test via ./check\n')
+sys.exit(os.EX_USAGE)
+
debug = '-d' in sys.argv
verbosity = 1
verify_image_format(supported_fmts)
--
1.9.1
[Qemu-devel] [for-2.6] Re: [PATCH] qmp-commands.hx: document minimum speed for block jobs
Dear developers, Sascha Silbe <si...@linux.vnet.ibm.com> writes: > The current rate limit implementation for block jobs is ineffective > below a certain minimum rate. It will permit writes at least once per > time slice. The resulting minimum write speed (assuming source and > sink are fast enough in the first place) is high enough that it may > surprise some users, so document it. Mention that this will be fixed > in the future, otherwise some users might misguidedly rely on it or > clamp their configuration settings to the documented value. Forgot to mention: This is 2.6 material as it documents a known shortcoming that will hopefully be fixed in 2.7. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
[Qemu-devel] [PATCH] qmp-commands.hx: document minimum speed for block jobs
The current rate limit implementation for block jobs is ineffective below a certain minimum rate. It will permit writes at least once per time slice. The resulting minimum write speed (assuming source and sink are fast enough in the first place) is high enough that it may surprise some users, so document it. Mention that this will be fixed in the future, otherwise some users might misguidedly rely on it or clamp their configuration settings to the documented value. Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com> --- Noticed this while figuring out why qemu-iotests #141 failed on one of my systems. I for one was quite surprised, so I went ahead and documented it. qmp-commands.hx | 36 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/qmp-commands.hx b/qmp-commands.hx index de896a5..e0e519a 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1107,7 +1107,10 @@ Arguments: obvious choice. Care should be taken when specifying the string, to specify a valid filename or protocol. (json-string, optional) (Since 2.1) -- "speed": the maximum speed, in bytes per second (json-int, optional) +- "speed": The maximum speed, in bytes per second. Note: In the current + implementation, at least 5MiB/s will be written, even if a lower + speed has been set. This will be fixed in the future. (json-int, + optional) - "on-error": the action to take on an error (default 'report'). 'stop' and 'enospc' can only be used if the block device supports io-status. (json-string, optional) (Since 2.1) @@ -1172,7 +1175,10 @@ Arguments: size of the smaller top, you can safely truncate it yourself once the commit operation successfully completes. (json-string) -- "speed": the maximum speed, in bytes per second (json-int, optional) +- "speed": The maximum speed, in bytes per second. Note: In the current + implementation, at least 5MiB/s will be written, even if a lower + speed has been set. This will be fixed in the future. (json-int, + optional) Example: @@ -1219,7 +1225,10 @@ Arguments: is "incremental", must NOT be present otherwise. - "mode": whether and how QEMU should create a new image (NewImageMode, optional, default 'absolute-paths') -- "speed": the maximum speed, in bytes per second (json-int, optional) +- "speed": The maximum speed, in bytes per second. Note: The current + implementation will write at a minimum speed that depends on device + and format, even if a lower speed is configured. This will be fixed + in the future. (json-int, optional) - "on-source-error": the action to take on an error on the source, default 'report'. 'stop' and 'enospc' can only be used if the block device supports io-status. @@ -1260,7 +1269,10 @@ Arguments: possibilities include "full" for all the disk, "top" for only the sectors allocated in the topmost image, or "none" to only replicate new I/O (MirrorSyncMode). -- "speed": the maximum speed, in bytes per second (json-int, optional) +- "speed": The maximum speed, in bytes per second. Note: The current + implementation will write at a minimum speed that depends on device + and format, even if a lower speed is configured. This will be fixed + in the future. (json-int, optional) - "on-source-error": the action to take on an error on the source, default 'report'. 'stop' and 'enospc' can only be used if the block device supports io-status. @@ -1659,8 +1671,12 @@ Arguments: (json-string, optional) - "mode": how an image file should be created into the target file/device (NewImageMode, optional, default 'absolute-paths') -- "speed": maximum speed of the streaming job, in bytes per second - (json-int) +- "speed": The maximum speed, in bytes per second. Note: In the + current implementation, the buffer will be written at least + once per 100ms. So with the default buffer size of 10MiB, + at least 10MiB/s will be written, even if a lower speed is + configured. This will be fixed in the future. (json-int, + optional) - "granularity": granularity of the dirty bitmap, in bytes (json-int, optional) - "buf-size": maximum amount of data in flight from source to target, in bytes (json-int, default 10M) @@ -1712,8 +1728,12 @@ Arguments: - "target": device name to mirror to (json-string) - "replaces": the block driver node name to replace when finis
[Qemu-devel] [PATCH 3/4] qemu-iotests: tests: do not set unused tmp variable
The previous commit removed the last usage of ${tmp} inside the tests
themselves; the only remaining users are sourced by check. So we can now
drop this variable from the tests.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
Reviewed-by: Bo Tu <t...@linux.vnet.ibm.com>
---
tests/qemu-iotests/001 | 1 -
tests/qemu-iotests/002 | 1 -
tests/qemu-iotests/003 | 1 -
tests/qemu-iotests/004 | 1 -
tests/qemu-iotests/005 | 1 -
tests/qemu-iotests/007 | 1 -
tests/qemu-iotests/008 | 1 -
tests/qemu-iotests/009 | 1 -
tests/qemu-iotests/010 | 1 -
tests/qemu-iotests/011 | 1 -
tests/qemu-iotests/012 | 1 -
tests/qemu-iotests/013 | 1 -
tests/qemu-iotests/014 | 1 -
tests/qemu-iotests/015 | 1 -
tests/qemu-iotests/017 | 1 -
tests/qemu-iotests/018 | 1 -
tests/qemu-iotests/019 | 1 -
tests/qemu-iotests/020 | 1 -
tests/qemu-iotests/021 | 1 -
tests/qemu-iotests/022 | 1 -
tests/qemu-iotests/023 | 1 -
tests/qemu-iotests/024 | 1 -
tests/qemu-iotests/025 | 1 -
tests/qemu-iotests/026 | 1 -
tests/qemu-iotests/027 | 1 -
tests/qemu-iotests/028 | 1 -
tests/qemu-iotests/029 | 1 -
tests/qemu-iotests/031 | 1 -
tests/qemu-iotests/032 | 1 -
tests/qemu-iotests/033 | 1 -
tests/qemu-iotests/034 | 1 -
tests/qemu-iotests/035 | 1 -
tests/qemu-iotests/036 | 1 -
tests/qemu-iotests/037 | 1 -
tests/qemu-iotests/038 | 1 -
tests/qemu-iotests/039 | 1 -
tests/qemu-iotests/042 | 1 -
tests/qemu-iotests/043 | 1 -
tests/qemu-iotests/046 | 1 -
tests/qemu-iotests/047 | 1 -
tests/qemu-iotests/049 | 1 -
tests/qemu-iotests/050 | 1 -
tests/qemu-iotests/051 | 1 -
tests/qemu-iotests/052 | 1 -
tests/qemu-iotests/053 | 1 -
tests/qemu-iotests/054 | 1 -
tests/qemu-iotests/058 | 1 -
tests/qemu-iotests/059 | 1 -
tests/qemu-iotests/060 | 1 -
tests/qemu-iotests/061 | 1 -
tests/qemu-iotests/062 | 1 -
tests/qemu-iotests/063 | 1 -
tests/qemu-iotests/064 | 1 -
tests/qemu-iotests/066 | 1 -
tests/qemu-iotests/067 | 1 -
tests/qemu-iotests/068 | 1 -
tests/qemu-iotests/069 | 1 -
tests/qemu-iotests/070 | 1 -
tests/qemu-iotests/071 | 1 -
tests/qemu-iotests/072 | 1 -
tests/qemu-iotests/073 | 1 -
tests/qemu-iotests/075 | 1 -
tests/qemu-iotests/076 | 1 -
tests/qemu-iotests/077 | 1 -
tests/qemu-iotests/078 | 1 -
tests/qemu-iotests/079 | 1 -
tests/qemu-iotests/080 | 1 -
tests/qemu-iotests/081 | 1 -
tests/qemu-iotests/082 | 1 -
tests/qemu-iotests/083 | 1 -
tests/qemu-iotests/084 | 1 -
tests/qemu-iotests/086 | 1 -
tests/qemu-iotests/087 | 1 -
tests/qemu-iotests/088 | 1 -
tests/qemu-iotests/089 | 1 -
tests/qemu-iotests/090 | 1 -
tests/qemu-iotests/092 | 1 -
tests/qemu-iotests/094 | 1 -
tests/qemu-iotests/097 | 1 -
tests/qemu-iotests/098 | 1 -
tests/qemu-iotests/099 | 1 -
tests/qemu-iotests/100 | 1 -
tests/qemu-iotests/101 | 1 -
tests/qemu-iotests/102 | 1 -
tests/qemu-iotests/103 | 1 -
tests/qemu-iotests/104 | 1 -
tests/qemu-iotests/105 | 1 -
tests/qemu-iotests/107 | 1 -
tests/qemu-iotests/108 | 1 -
tests/qemu-iotests/109 | 1 -
tests/qemu-iotests/110 | 1 -
tests/qemu-iotests/111 | 1 -
tests/qemu-iotests/112 | 1 -
tests/qemu-iotests/113 | 1 -
tests/qemu-iotests/114 | 1 -
tests/qemu-iotests/115 | 1 -
tests/qemu-iotests/116 | 1 -
tests/qemu-iotests/117 | 1 -
tests/qemu-iotests/119 | 1 -
tests/qemu-iotests/120 | 1 -
tests/qemu-iotests/121 | 1 -
tests/qemu-iotests/122 | 1 -
tests/qemu-iotests/123 | 1 -
tests/qemu-iotests/128 | 1 -
tests/qemu-iotests/130 | 1 -
tests/qemu-iotests/131 | 1 -
tests/qemu-iotests/133 | 1 -
tests/qemu-iotests/134 | 1 -
tests/qemu-iotests/135 | 1 -
tests/qemu-iotests/137 | 1 -
tests/qemu-iotests/138 | 1 -
tests/qemu-iotests/140 | 1 -
tests/qemu-iotests/141 | 1 -
tests/qemu-iotests/142 | 1 -
tests/qemu-iotests/143 | 1 -
tests/qemu-iotests/145 | 1 -
tests/qemu-iotests/150 | 1 -
117 files changed, 117 deletions(-)
diff --git a/tests/qemu-iotests/001 b/tests/qemu-iotests/001
index 4e16469..ffd14e2 100755
--- a/tests/qemu-iotests/001
+++ b/tests/qemu-iotests/001
@@ -25,7 +25,6 @@ seq=`basename $0`
echo "QA output created by $seq"
here=`pwd`
-tmp=/tmp/$$
status=1 # failure is the default!
_cleanup()
diff --git a/tests/qemu-iotests/002 b/tests/qemu-iotests/002
index 6a865aa..d4f8e91 100755
--- a/tests/qemu-iotests/002
+++ b/tests/qemu-iotests/002
@@ -25,7 +25,6 @@ seq=`basename $0`
echo "QA output created by $seq"
here=`pwd`
-tmp=/tmp/$$
status=1 # failure is the default!
_cleanup()
diff --git a/tests/qemu-iotests/003 b/tests/qemu-iotests/003
index 98638d4..19889b9 100755
--- a/tests/qemu-iotests/003
+++ b/tests/qemu-iotests/003
@@ -25,7 +25,6 @@ seq=`basename $0`
echo "QA output created by $seq"
here=`pwd`
-tmp=/tmp/$$
status=1 # failure is the default!
_cleanup()
diff --git a/tests/qemu-iotests/004 b/tests/qemu-iotests/004
index 2ad77ed..67e1beb 100755
--- a/tests/qemu-iotests/004
+++ b/tests/qemu-iotests/004
@@ -25,7 +25,6 @@ seq=`basename $0`
echo "QA output create
[Qemu-devel] [PATCH for-2.6 0/4] qemu-iotests: don't use /tmp
During review of my other qemu-iotests fixes, Max Reitz noticed a couple of additional places where qemu-iotests hardcode /tmp. This is both a security issue and cumbersome when running multiple instances of qemu-iotests (e.g. different users on a shared development machine like tuxmaker). Fix them up. Checked (/var)/tmp usage using "sudo chown 000 /tmp /var/tmp". "tac" and "qemu -snapshot" fail in that case. I'll presume they create the temporary files in a secure manner. There is one place left that may use /var/tmp in theory; will address that in a separate patch. This series mostly removes dead code and addresses a potential security issue, all of that in the test suite rather than in production code. It should be applicable even during hard freeze. Sascha Silbe (4): qemu-iotests: drop unused _within_tolerance() filter qemu-iotests: common.rc: drop unused _do() qemu-iotests: tests: do not set unused tmp variable qemu-iotests: place valgrind log file in scratch dir tests/qemu-iotests/001 | 1 - tests/qemu-iotests/002 | 1 - tests/qemu-iotests/003 | 1 - tests/qemu-iotests/004 | 1 - tests/qemu-iotests/005 | 1 - tests/qemu-iotests/007 | 1 - tests/qemu-iotests/008 | 1 - tests/qemu-iotests/009 | 1 - tests/qemu-iotests/010 | 1 - tests/qemu-iotests/011 | 1 - tests/qemu-iotests/012 | 1 - tests/qemu-iotests/013 | 1 - tests/qemu-iotests/014 | 1 - tests/qemu-iotests/015 | 1 - tests/qemu-iotests/017 | 1 - tests/qemu-iotests/018 | 1 - tests/qemu-iotests/019 | 1 - tests/qemu-iotests/020 | 1 - tests/qemu-iotests/021 | 1 - tests/qemu-iotests/022 | 1 - tests/qemu-iotests/023 | 1 - tests/qemu-iotests/024 | 1 - tests/qemu-iotests/025 | 1 - tests/qemu-iotests/026 | 1 - tests/qemu-iotests/027 | 1 - tests/qemu-iotests/028 | 1 - tests/qemu-iotests/029 | 1 - tests/qemu-iotests/031 | 1 - tests/qemu-iotests/032 | 1 - tests/qemu-iotests/033 | 1 - tests/qemu-iotests/034 | 1 - tests/qemu-iotests/035 | 1 - tests/qemu-iotests/036 | 1 - tests/qemu-iotests/037 | 1 - tests/qemu-iotests/038 | 1 - tests/qemu-iotests/039 | 1 - tests/qemu-iotests/042 | 1 - tests/qemu-iotests/043 | 1 - tests/qemu-iotests/046 | 1 - tests/qemu-iotests/047 | 1 - tests/qemu-iotests/049 | 1 - tests/qemu-iotests/050 | 1 - tests/qemu-iotests/051 | 1 - tests/qemu-iotests/052 | 1 - tests/qemu-iotests/053 | 1 - tests/qemu-iotests/054 | 1 - tests/qemu-iotests/058 | 1 - tests/qemu-iotests/059 | 1 - tests/qemu-iotests/060 | 1 - tests/qemu-iotests/061 | 1 - tests/qemu-iotests/062 | 1 - tests/qemu-iotests/063 | 1 - tests/qemu-iotests/064 | 1 - tests/qemu-iotests/066 | 1 - tests/qemu-iotests/067 | 1 - tests/qemu-iotests/068 | 1 - tests/qemu-iotests/069 | 1 - tests/qemu-iotests/070 | 1 - tests/qemu-iotests/071 | 1 - tests/qemu-iotests/072 | 1 - tests/qemu-iotests/073 | 1 - tests/qemu-iotests/075 | 1 - tests/qemu-iotests/076 | 1 - tests/qemu-iotests/077 | 1 - tests/qemu-iotests/078 | 1 - tests/qemu-iotests/079 | 1 - tests/qemu-iotests/080 | 1 - tests/qemu-iotests/081 | 1 - tests/qemu-iotests/082 | 1 - tests/qemu-iotests/083 | 1 - tests/qemu-iotests/084 | 1 - tests/qemu-iotests/086 | 1 - tests/qemu-iotests/087 | 1 - tests/qemu-iotests/088 | 1 - tests/qemu-iotests/089 | 1 - tests/qemu-iotests/090 | 1 - tests/qemu-iotests/092 | 1 - tests/qemu-iotests/094 | 1 - tests/qemu-iotests/097 | 1 - tests/qemu-iotests/098 | 1 - tests/qemu-iotests/099 | 1 - tests/qemu-iotests/100 | 1 - tests/qemu-iotests/101 | 1 - tests/qemu-iotests/102 | 1 - tests/qemu-iotests/103 | 1 - tests/qemu-iotests/104 | 1 - tests/qemu-iotests/105 | 1 - tests/qemu-iotests/107 | 1 - tests/qemu-iotests/108 | 1 - tests/qemu-iotests/109 | 1 - tests/qemu-iotests/110 | 1 - tests/qemu-iotests/111 | 1 - tests/qemu-iotests/112 | 1 - tests/qemu-iotests/113 | 1 - tests/qemu-iotest
[Qemu-devel] [PATCH 4/4] qemu-iotests: place valgrind log file in scratch dir
Do not place the valgrind log file at a predictable path in a
world-writable location. Use the common scratch directory (${TEST_DIR})
instead.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
Reviewed-by: Bo Tu <t...@linux.vnet.ibm.com>
---
tests/qemu-iotests/common.config | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index 60bfabf..f824651 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -122,7 +122,7 @@ _qemu_img_wrapper()
_qemu_io_wrapper()
{
-local VALGRIND_LOGFILE=/tmp/$$.valgrind
+local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
local RETVAL
(
if [ "${VALGRIND_QEMU}" == "y" ]; then
--
1.9.1
[Qemu-devel] [PATCH 1/4] qemu-iotests: drop unused _within_tolerance() filter
_within_tolerance() isn't used anymore and possibly creates temporary
files at predictable, world-writable locations. Get rid of it.
If it's needed again in the future it can be revived easily and fixed up
to use TEST_DIR and / or safely created temporary files.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
Reviewed-by: Bo Tu <t...@linux.vnet.ibm.com>
---
tests/qemu-iotests/common.filter | 101 ---
1 file changed, 101 deletions(-)
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 84b7434..8a6e1b5 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -19,107 +19,6 @@
# standard filters
#
-# Checks that given_value is in range of correct_value +/- tolerance.
-# Tolerance can be an absolute value or a percentage of the correct value
-# (see examples with tolerances below).
-# Outputs suitable message to stdout if it's not in range.
-#
-# A verbose option, -v, may be used as the LAST argument
-#
-# e.g.
-# foo: 0.0298 = 0.03 +/- 5%
-# _within_tolerance "foo" 0.0298 0.03 5%
-#
-# foo: 0.0298 = 0.03 +/- 0.01
-# _within_tolerance "foo" 0.0298 0.03 0.01
-#
-# foo: 0.0298 = 0.03 -0.01 +0.002
-# _within_tolerance "foo" 0.0298 0.03 0.01 0.002
-#
-# foo: verbose output of 0.0298 = 0.03 +/- 5%
-# _within_tolerance "foo" 0.0298 0.03 5% -v
-_within_tolerance()
-{
- _name=$1
- _given_val=$2
- _correct_val=$3
- _mintol=$4
- _maxtol=$_mintol
- _verbose=0
- _debug=false
-
- # maxtol arg is optional
- # verbose arg is optional
- if [ $# -ge 5 ]
- then
- if [ "$5" = "-v" ]
- then
-_verbose=1
- else
-_maxtol=$5
- fi
- fi
- if [ $# -ge 6 ]
- then
- [ "$6" = "-v" ] && _verbose=1
- fi
-
- # find min with or without %
- _mintolerance=`echo $_mintol | sed -e 's/%//'`
- if [ $_mintol = $_mintolerance ]
- then
- _min=`echo "scale=5; $_correct_val-$_mintolerance" | bc`
- else
- _min=`echo "scale=5; $_correct_val-$_mintolerance*0.01*$_correct_val" |
bc`
- fi
-
- # find max with or without %
- _maxtolerance=`echo $_maxtol | sed -e 's/%//'`
- if [ $_maxtol = $_maxtolerance ]
- then
- _max=`echo "scale=5; $_correct_val+$_maxtolerance" | bc`
- else
- _max=`echo "scale=5; $_correct_val+$_maxtolerance*0.01*$_correct_val" |
bc`
- fi
-
- $_debug && echo "min = $_min"
- $_debug && echo "max = $_max"
-
- cat <$tmp.bc.1
-scale=5;
-if ($_min <= $_given_val) 1;
-if ($_min > $_given_val) 0;
-EOF
-
- cat <$tmp.bc.2
-scale=5;
-if ($_given_val <= $_max) 1;
-if ($_given_val > $_max) 0;
-EOF
-
- _above_min=`bc <$tmp.bc.1`
- _below_max=`bc <$tmp.bc.2`
-
- rm -f $tmp.bc.[12]
-
- _in_range=`expr $_above_min \& $_below_max`
-
- # fix up min, max precision for output
- # can vary for 5.3, 6.2
- _min=`echo $_min | sed -e 's/0*$//'` # get rid of trailling zeroes
- _max=`echo $_max | sed -e 's/0*$//'` # get rid of trailling zeroes
-
- if [ $_in_range -eq 1 ]
- then
-[ $_verbose -eq 1 ] && echo $_name is in range
-return 0
- else
-[ $_verbose -eq 1 ] && echo $_name has value of $_given_val
-[ $_verbose -eq 1 ] && echo $_name is NOT in range $_min .. $_max
-return 1
- fi
-}
-
# ctime(3) dates
#
_filter_date()
--
1.9.1
[Qemu-devel] [PATCH 2/4] qemu-iotests: common.rc: drop unused _do()
_do() was never used and possibly creates temporary files at
predictable, world-writable locations. Get rid of it.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
Reviewed-by: Bo Tu <t...@linux.vnet.ibm.com>
---
tests/qemu-iotests/common.rc | 46
1 file changed, 46 deletions(-)
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index d9913f8..5249ec5 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -287,52 +287,6 @@ _need_to_be_root()
fi
}
-
-# Do a command, log it to $seq.full, optionally test return status
-# and die if command fails. If called with one argument _do executes the
-# command, logs it, and returns its exit status. With two arguments _do
-# first prints the message passed in the first argument, and then "done"
-# or "fail" depending on the return status of the command passed in the
-# second argument. If the command fails and the variable _do_die_on_error
-# is set to "always" or the two argument form is used and _do_die_on_error
-# is set to "message_only" _do will print an error message to
-# $seq.out and exit.
-
-_do()
-{
-if [ $# -eq 1 ]; then
-_cmd=$1
-elif [ $# -eq 2 ]; then
-_note=$1
-_cmd=$2
-echo -n "$_note... "
-else
-echo "Usage: _do [note] cmd" 1>&2
-status=1; exit
-fi
-
-(eval "echo '---' \"$_cmd\"") >>"$OUTPUT_DIR/$seq.full"
-(eval "$_cmd") >$tmp._out 2>&1; ret=$?
-cat $tmp._out >>"$OUTPUT_DIR/$seq.full"
-if [ $# -eq 2 ]; then
-if [ $ret -eq 0 ]; then
-echo "done"
-else
-echo "fail"
-fi
-fi
-if [ $ret -ne 0 ] \
-&& [ "$_do_die_on_error" = "always" \
--o \( $# -eq 2 -a "$_do_die_on_error" = "message_only" \) ]
-then
-[ $# -ne 2 ] && echo
-eval "echo \"$_cmd\" failed \(returned $ret\): see $seq.full"
-status=1; exit
-fi
-
-return $ret
-}
-
# bail out, setting up .notrun file
#
_notrun()
--
1.9.1
Re: [Qemu-devel] [Qemu-block] [PATCH 0/7] next round of qemu-iotests fixes
Dear Max, Max Reitz <mre...@redhat.com> writes: > On 05.04.2016 11:21, Sascha Silbe wrote: >> With these fixes, qemu-iotests complete successfully on my test >> systems (s390x, x86_64) when used with QCOW2 or raw image formats. >> >> These are purely bug fixes for tests and most are trivial, so they >> should be safe even for hard freeze. > > Thanks, applied everything except for patch 6 to my block tree: > > https://github.com/XanClic/qemu/commits/block Thanks! I've cooked up patches for the remaining /tmp usage and documenting the minimum block speed speed; will send them out next week. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qemu-iotests: 141: reduce likelihood of race condition on systems with fast IO
Dear Kevin, Kevin Wolf <kw...@redhat.com> writes: > Am 08.04.2016 um 14:01 hat Sascha Silbe geschrieben: [...] >> The best approach probably would be to fix up the rate limit code to >> delay for multiple time slices if necessary. We should get rid of the >> artificial BDRV_SECTOR_SIZE granularity at the same time, always using >> bytes as the quantum unit. > > In the 2.7 time frame we might actually be able to reuse the normal I/O > throttling code for block jobs as the jobs will be using their own > BlockBackend and can therefore set their own throttling limits. Sounds good. Then I suggest we just drop this patch for now. I've added a reminder to check the situation again at the end of June (i.e. before soft feature freeze of 2.7). Will give documenting the minimum rate a try, but that shouldn't hold up the fixes. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qemu-iotests: 141: reduce likelihood of race condition on systems with fast IO
Dear Max, Sascha Silbe <si...@linux.vnet.ibm.com> writes: > @Max: From a cursory glance at the code, maybe your 1 *byte* per second > rate limit is being rounded down to 0 *blocks* per second, with 0 > meaning no limit? See e.g. mirror_set_speed(). Though I must admit I > don't understand how speed=0 translates to unlimited (like > qapi/block-core.json:block-job-set-speed says). My understanding of > ratelimit_calculate_delay() is that speed=0 means "1 quantum per time > slice", with time slice usually being 100ms; not sure about the > quantum. I think I've understood the issue now. The backup, commit, mirror and stream actions operate in on full chunks, with chunk size depending on the action and backing device. For e.g. commit that means it always bursts at least 0.5MiB; that's where the value the reference output comes from. ratelimit_calculate_delay() lets through at least one burst per time slice. This means the minimum rate is chunk size per time slice (always 100ms). So for commit and stream one will always get at least 5 MiB/s. A surprisingly large value for something specified in bytes per second, BTW. (I.e. it should probably be documented in qmp-commands.hx if it stays this way). On a busy or slow host, it may take the shell longer than the time slice of 100ms to send the cancel command to qemu. When that happens, additional chunks will get written before the job gets cancelled. That's why I sometimes see 1 or even 1.5 MiB as offset, especially when running CPU intensive workloads in parallel. The best approach probably would be to fix up the rate limit code to delay for multiple time slices if necessary. We should get rid of the artificial BDRV_SECTOR_SIZE granularity at the same time, always using bytes as the quantum unit. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [Qemu-block] [PATCH 3/7] qemu-iotests: iotests.VM: remove qtest socket on error
Dear Max, Max Reitz <mre...@redhat.com> writes: > On 05.04.2016 11:21, Sascha Silbe wrote: [...] [_remove_if_exists()] > Not sure about the leading underscore (it appears to be the only such > function in iotests.py besides __init__()), but I guess it's a test so > it doesn't really matter anyway. I considered it an internal implementation detail of VM.launch(), hence the underscore. Thinking about it some more, it may be a useful helper for the tests themselves. But we can easily promote it whenever one of the tests actually wants to make use of it. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qemu-iotests: 141: reduce likelihood of race condition on systems with fast IO
Dear Max, Max Reitz <mre...@redhat.com> writes: > On 05.04.2016 11:21, Sascha Silbe wrote: >> On systems with fast IO, qemu-io may write more than 1 MiB before >> receiving the block-job-cancel command, causing the test case to fail. >> >> 141 is inherently racy, but we can at least reduce the likelihood of the >> job completing before the cancel command arrives by bumping the size of >> the data getting written; we'll try 32 MiB for a start. > > Hm, interesting. I tried to prevent this by setting the block jobs' > speed to 1, which should make it stop after the block job has processed > the first block of data. Hmm, seems like I didn't quite understand the way the test works then. @Kevin: Please do NOT queue this patch. @Max: From a cursory glance at the code, maybe your 1 *byte* per second rate limit is being rounded down to 0 *blocks* per second, with 0 meaning no limit? See e.g. mirror_set_speed(). Though I must admit I don't understand how speed=0 translates to unlimited (like qapi/block-core.json:block-job-set-speed says). My understanding of ratelimit_calculate_delay() is that speed=0 means "1 quantum per time slice", with time slice usually being 100ms; not sure about the quantum. Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
Re: [Qemu-devel] [Qemu-block] [PATCH 1/7] qemu-iotests: check: don't place files with predictable names in /tmp
Dear Max, Max Reitz <mre...@redhat.com> writes: > You decide whether you want to drop the tmp=/tmp/$$ lines in the tests > in a dedicated (follow-up) patch or include it here. Let's land this one first; I'll address the other occurrences in a separate patch. I've put it on my todo list. Thanks for the reviews! Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
[Qemu-devel] [PATCH 6/7] qemu-iotests: 141: reduce likelihood of race condition on systems with fast IO
On systems with fast IO, qemu-io may write more than 1 MiB before
receiving the block-job-cancel command, causing the test case to fail.
141 is inherently racy, but we can at least reduce the likelihood of the
job completing before the cancel command arrives by bumping the size of
the data getting written; we'll try 32 MiB for a start.
Once we actually move enough data around for the block job not to
complete prematurely, the test will still fail because the offset value
in the BLOCK_JOB_CANCELLED event will vary. Since this is more or less
inherent to the nature of this event, we just replace it with a fixed
value globally (in _filter_qmp), the same way we handle timestamps.
Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com>
Reviewed-by: Bo Tu <t...@linux.vnet.ibm.com>
---
tests/qemu-iotests/141 | 11 ++-
tests/qemu-iotests/141.out | 24
tests/qemu-iotests/common.filter | 1 +
3 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
index f7c28b4..a06dc72 100755
--- a/tests/qemu-iotests/141
+++ b/tests/qemu-iotests/141
@@ -83,9 +83,10 @@ test_blockjob()
}
-TEST_IMG="$TEST_DIR/b.$IMGFMT" _make_test_img 1M
-TEST_IMG="$TEST_DIR/m.$IMGFMT" _make_test_img -b "$TEST_DIR/b.$IMGFMT" 1M
-_make_test_img -b "$TEST_DIR/m.$IMGFMT" 1M
+IMGSIZE=32M
+TEST_IMG="$TEST_DIR/b.$IMGFMT" _make_test_img "${IMGSIZE}"
+TEST_IMG="$TEST_DIR/m.$IMGFMT" _make_test_img -b "$TEST_DIR/b.$IMGFMT"
"${IMGSIZE}"
+_make_test_img -b "$TEST_DIR/m.$IMGFMT" "${IMGSIZE}"
_launch_qemu -nodefaults
@@ -147,7 +148,7 @@ echo
# immediately, send a BLOCK_JOB_COMPLETED and ejecting the BDS would work just
# fine without the block job still running.
-$QEMU_IO -c 'write 0 1M' "$TEST_DIR/m.$IMGFMT" | _filter_qemu_io
+$QEMU_IO -c "write 0 ${IMGSIZE}" "$TEST_DIR/m.$IMGFMT" | _filter_qemu_io
test_blockjob \
"{'execute': 'block-commit',
@@ -165,7 +166,7 @@ echo
# immediately, send a BLOCK_JOB_COMPLETED and ejecting the BDS would work just
# fine without the block job still running.
-$QEMU_IO -c 'write 0 1M' "$TEST_DIR/b.$IMGFMT" | _filter_qemu_io
+$QEMU_IO -c "write 0 ${IMGSIZE}" "$TEST_DIR/b.$IMGFMT" | _filter_qemu_io
# With some data to stream (and @speed set to 1), block-stream will not
complete
# until we send the block-job-cancel command. Therefore, no event other than
diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
index adceac1..14e457f 100644
--- a/tests/qemu-iotests/141.out
+++ b/tests/qemu-iotests/141.out
@@ -1,23 +1,23 @@
QA output created by 141
-Formatting 'TEST_DIR/b.IMGFMT', fmt=IMGFMT size=1048576
-Formatting 'TEST_DIR/m.IMGFMT', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/b.IMGFMT
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/m.IMGFMT
+Formatting 'TEST_DIR/b.IMGFMT', fmt=IMGFMT size=33554432
+Formatting 'TEST_DIR/m.IMGFMT', fmt=IMGFMT size=33554432
backing_file=TEST_DIR/b.IMGFMT
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33554432
backing_file=TEST_DIR/m.IMGFMT
{"return": {}}
=== Testing drive-backup ===
{"return": {}}
-Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
+Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=33554432
backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
{"return": {}}
{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device
is in use by block job: backup"}}
{"return": {}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 0,
"speed": 0, "type": "backup"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 33554432, "offset":
OFFSET, "speed": 0, "type": "backup"}}
{"return": {}}
=== Testing drive-mirror ===
{"return": {}}
-Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
+Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=33554432
backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, &qu
[Qemu-devel] [PATCH 0/7] next round of qemu-iotests fixes
With these fixes, qemu-iotests complete successfully on my test systems (s390x, x86_64) when used with QCOW2 or raw image formats. These are purely bug fixes for tests and most are trivial, so they should be safe even for hard freeze. Sascha Silbe (7): qemu-iotests: check: don't place files with predictable names in /tmp qemu-iotests: fix 051 on non-PC architectures qemu-iotests: iotests.VM: remove qtest socket on error qemu-iotests: 148: properly skip test if quorum support is missing qemu-iotests: 068: don't require KVM qemu-iotests: 141: reduce likelihood of race condition on systems with fast IO qemu-iotests: iotests.py: get rid of __all__ tests/qemu-iotests/051.out | 10 +- tests/qemu-iotests/068 | 2 +- tests/qemu-iotests/141 | 11 ++- tests/qemu-iotests/141.out | 24 tests/qemu-iotests/148 | 4 +--- tests/qemu-iotests/check | 21 +++-- tests/qemu-iotests/common.filter | 1 + tests/qemu-iotests/iotests.py| 22 +- 8 files changed, 54 insertions(+), 41 deletions(-) -- 1.9.1
[Qemu-devel] [PATCH 5/7] qemu-iotests: 068: don't require KVM
None of the other test cases explicitly enable KVM and there's no obvious reason for 068 to require it. Drop this so all test cases can be executed in environments where KVM is not available (e.g. because the user doesn't have sufficient permissions to access /dev/kvm). Signed-off-by: Sascha Silbe <si...@linux.vnet.ibm.com> Reviewed-by: Bo Tu <t...@linux.vnet.ibm.com> --- tests/qemu-iotests/068 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068 index 58d1d80..7562dd7 100755 --- a/tests/qemu-iotests/068 +++ b/tests/qemu-iotests/068 @@ -53,7 +53,7 @@ _make_test_img $IMG_SIZE case "$QEMU_DEFAULT_MACHINE" in s390-ccw-virtio) - platform_parm="-no-shutdown -machine accel=kvm" + platform_parm="-no-shutdown" ;; *) platform_parm="" -- 1.9.1
