Re: [Qemu-devel] [PATCH] vnc: fix segfault in closed connection handling
On 01/31/2018 04:16 PM, Marc-André Lureau wrote:
Hi
On Wed, Jan 31, 2018 at 2:06 PM, Klim Kireev wrote:
On one of our client's node, due to trying to read from closed ioc,
a segmentation fault occured. Corresponding backtrace:
Oops, you probably forgot an extra space before the # interpreted as comment.
Thanks, fixed
Do you have a reproducer?
Unfortunately, no
Having analyzed the coredump, I understood that the reason is that
ioc_tag is reset on vnc_disconnect_start and ioc is cleaned
in vnc_disconnect_finish. Between these two events due to some
reasons the ioc_tag was set again and after vnc_disconnect_finish
the handler is running with freed ioc,
which led to the segmentation fault.
I suggest to check ioc_tag in vnc_disconnect_finish to prevent such
an occurrence.
Signed-off-by: Klim Kireev
---
ui/vnc.c | 4
1 file changed, 4 insertions(+)
diff --git a/ui/vnc.c b/ui/vnc.c
index 33b087221f..b8bf0180cb 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1270,6 +1270,10 @@ void vnc_disconnect_finish(VncState *vs)
}
g_free(vs->lossy_rect);
+if (vs->ioc_tag) {
+g_source_remove(vs->ioc_tag);
+vs->ioc_tag = 0;
+}
object_unref(OBJECT(vs->ioc));
vs->ioc = NULL;
object_unref(OBJECT(vs->sioc));
--
2.14.3
Re: [Qemu-devel] [PATCH] chardev/char-socket: add POLLHUP handler
On 01/16/2018 08:25 PM, Paolo Bonzini wrote:
On 10/01/2018 14:18, Klim Kireev wrote:
The following behavior was observed for QEMU configured by libvirt
to use guest agent as usual for the guests without virtio-serial
driver (Windows or the guest remaining in BIOS stage).
In QEMU on first connect to listen character device socket
the listen socket is removed from poll just after the accept().
virtio_serial_guest_ready() returns 0 and the descriptor
of the connected Unix socket is removed from poll and it will
not be present in poll() until the guest will initialize the driver
and change the state of the serial to "guest connected".
In libvirt connect() to guest agent is performed on restart and
is run under VM state lock. Connect() is blocking and can
wait forever.
In this case libvirt can not perform ANY operation on that VM.
The bug can be easily reproduced this way:
Terminal 1:
qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev
socket,id=serial1,path=/tmp/console.sock,server,nowait
(virtio-serial and isa-serial also fit)
Terminal 2:
minicom -D unix\#/tmp/console.sock
(type something and pres enter)
C-a x (to exit)
Do 3 times:
minicom -D unix\#/tmp/console.sock
C-a x
It needs 4 connections, because the first one is accepted by QEMU, then two are
queued by
the kernel, and the 4th blocks.
The problem is that QEMU doesn't add a read watcher after succesful read
until the guest device wants to acquire recieved data, so
I propose to install a separate pullhup watcher regardless of
whether the device waits for data or not. After closing the connection,
the guest has a capability to read the data within timeout.
I don't understand the timeout part.
The timeout is important, because of following situation:
client sends to the guest big bunch of data and closes his end of socket.
if we just destroy the connection on the qemu's side, the guest can't
read remaining data in channel.
(which can be much more than guest's buffer), although he would do it later.
For this case timeout after POLLHUP is added. It is set after receiving
POLLHUP and reset after each reading.
Apart from that, maybe the bug is in io_watch_poll_prepare, which needs
to _always_ set up a "G_IO_ERR | G_IO_HUP | G_IO_NVAL" watch. Only
G_IO_IN depends on iwp->fd_can_read(...) > 0.
So the change would start with something like this:
diff --git a/chardev/char-io.c b/chardev/char-io.c
index f81052481a..a5e65d4e7c 100644
--- a/chardev/char-io.c
+++ b/chardev/char-io.c
@@ -29,6 +29,7 @@ typedef struct IOWatchPoll {
QIOChannel *ioc;
GSource *src;
+GIOCondition cond;
IOCanReadHandler *fd_can_read;
GSourceFunc fd_read;
@@ -41,25 +42,32 @@ static IOWatchPoll *io_watch_poll_from_source(GSource
*source)
return container_of(source, IOWatchPoll, parent);
}
+static void io_watch_poll_destroy_source(IOWatchPoll *iwp)
+{
+if (iwp->src) {
+g_source_destroy(iwp->src);
+g_source_unref(iwp->src);
+iwp->src = NULL;
+iwp->cond = 0;
+}
+}
+
static gboolean io_watch_poll_prepare(GSource *source,
gint *timeout)
{
IOWatchPoll *iwp = io_watch_poll_from_source(source);
bool now_active = iwp->fd_can_read(iwp->opaque) > 0;
-bool was_active = iwp->src != NULL;
-if (was_active == now_active) {
-return FALSE;
+GIOCondition cond = G_IO_ERR | G_IO_HUP | G_IO_NVAL;
+if (now_active) {
+cond |= G_IO_IN;
}
-if (now_active) {
-iwp->src = qio_channel_create_watch(
-iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
+if (iwp->cond != cond) {
+io_watch_poll_destroy_source(iwp);
+iwp->cond = cond;
+iwp->src = qio_channel_create_watch(iwp->ioc, cond);
g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
g_source_attach(iwp->src, iwp->context);
-} else {
-g_source_destroy(iwp->src);
-g_source_unref(iwp->src);
-iwp->src = NULL;
}
return FALSE;
}
@@ -131,11 +139,7 @@ static void io_remove_watch_poll(GSource *source)
IOWatchPoll *iwp;
iwp = io_watch_poll_from_source(source);
-if (iwp->src) {
-g_source_destroy(iwp->src);
-g_source_unref(iwp->src);
-iwp->src = NULL;
-}
+io_watch_poll_destroy_source(iwp);
g_source_destroy(&iwp->parent);
}
Thanks,
Paolo
Re: [Qemu-devel] [PATCH] chardev/char-socket: add POLLHUP handler
On 01/18/2018 12:44 PM, Paolo Bonzini wrote: On 18/01/2018 10:41, klim wrote: I propose to install a separate pullhup watcher regardless of whether the device waits for data or not. After closing the connection, the guest has a capability to read the data within timeout. I don't understand the timeout part. The timeout is important, because of following situation: client sends to the guest big bunch of data and closes his end of socket. if we just destroy the connection on the qemu's side, the guest can't read remaining data in channel. (which can be much more than guest's buffer), although he would do it later. For this case timeout after POLLHUP is added. It is set after receiving POLLHUP and reset after each reading. But why would one second be enough? One second was merely a suggestion. Maybe 10 seconds would be better. Paolo
Re: [Qemu-devel] [PATCH v2] chardev/char-socket: add POLLHUP handler
On 01/18/2018 06:49 PM, Marc-André Lureau wrote:
Hi
On Thu, Jan 18, 2018 at 3:33 PM, Klim Kireev wrote:
The following behavior was observed for QEMU configured by libvirt
to use guest agent as usual for the guests without virtio-serial
driver (Windows or the guest remaining in BIOS stage).
In QEMU on first connect to listen character device socket
the listen socket is removed from poll just after the accept().
virtio_serial_guest_ready() returns 0 and the descriptor
of the connected Unix socket is removed from poll and it will
not be present in poll() until the guest will initialize the driver
and change the state of the serial to "guest connected".
In libvirt connect() to guest agent is performed on restart and
is run under VM state lock. Connect() is blocking and can
wait forever.
In this case libvirt can not perform ANY operation on that VM.
The bug can be easily reproduced this way:
Terminal 1:
qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev
socket,id=serial1,path=/tmp/console.sock,server,nowait
(virtio-serial and isa-serial also fit)
Terminal 2:
minicom -D unix\#/tmp/console.sock
(type something and press enter)
C-a x (to exit)
Do 3 times:
minicom -D unix\#/tmp/console.sock
C-a x
It needs 4 connections, because the first one is accepted by QEMU, then two are
queued by
the kernel, and the 4th blocks.
The problem is that QEMU doesn't add a read watcher after succesful read
until the guest device wants to acquire recieved data, so
I propose to install a separate pullhup watcher regardless of
whether the device waits for data or not.
Signed-off-by: Klim Kireev
---
Changelog:
v2: Remove timer as a redundant feature
chardev/char-socket.c | 29 -
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 77cdf487eb..d3fe903ab6 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -42,6 +42,7 @@ typedef struct {
QIOChannel *ioc; /* Client I/O channel */
QIOChannelSocket *sioc; /* Client master channel */
QIONetListener *listener;
+guint hup_tag;
QCryptoTLSCreds *tls_creds;
int connected;
int max_size;
@@ -352,6 +353,11 @@ static void tcp_chr_free_connection(Chardev *chr)
s->read_msgfds_num = 0;
}
+if (s->hup_tag != 0) {
+g_source_remove(s->hup_tag);
+s->hup_tag = 0;
+}
+
tcp_set_msgfds(chr, NULL, 0);
remove_fd_in_watch(chr);
object_unref(OBJECT(s->sioc));
@@ -455,6 +461,19 @@ static gboolean tcp_chr_read(QIOChannel *chan,
GIOCondition cond, void *opaque)
return TRUE;
}
+static gboolean tcp_chr_hup(QIOChannel *channel,
+ GIOCondition cond,
+ void *opaque)
+{
+Chardev *chr = CHARDEV(opaque);
+SocketChardev *s = SOCKET_CHARDEV(chr);
+tcp_chr_read(channel, cond, opaque);
+if (s->connected != 0) {
tcp_chr_read() shouldn't be called unless frontend is ready to read.
qemu_chr_be_can_write() is regularly updated with tcp_chr_read_poll()
but this may create some race here (if it read all it could read
previously for example)
If frontend can't read, s->connected won't be updated, so you'll busy
loop in the source callback, not good.
I think it needs further rework of how s->connected is updated.
Why call tcp_chr_read() if you received HUP event ? could it call
tcp_chr_free_connection()?
The reason is that:
if client sends data and closes the socket between two ppoll(), POLLHUP
handler is called
and data in channel is lost, so read is used to pass it to guest.
if there is no data in channel, tcp_chr_recv() returns 0
and tcp_chr_read() calls tcp_chr_disconnect() which calls
tcp_chr_free_connection().
If there is some data in channel it calls qemu_chr_be_write() and then
in tcp_chr_disconnect()
tcp_free_connection() will be called.
In any case connection will be closed, so where is busy loop?
+tcp_chr_disconnect(chr);
+}
+return TRUE;
please use G_SOURCE_CONTINUE/REMOVE (I know it's not being used
widely, but we have define now, and it is much clearer)
+}
+
static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -528,6 +547,10 @@ static void tcp_chr_connect(void *opaque)
tcp_chr_read,
chr, chr->gcontext);
}
+if (s->hup_tag == 0) {
+s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP,
+ tcp_chr_hup, chr, NULL);
+}
qemu_chr_be_event(chr, CHR_EVENT_OPENED);
}
@@ -546,7 +569,11 @@ static void tcp_chr_update_read_handler(Chardev *chr)
tcp_chr_read, chr,
chr->gcontext);
Re: [Qemu-devel] [PATCH 0/5 v3] preparation for Parallels Disk xml driver
On 01/12/2018 12:01 PM, Klim Kireev wrote: ping Parallels Desktop and Parallels Cloud Server uses images glued with the bundle description in XML format. This series contains very basic description of this XML files and makes preparations for actual implementation to be followed. Signed-off-by: Edgar Kaziakhmedov Signed-off-by: Klim Kireev Signed-off-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Denis V. Lunev Changelog: v2: PATCH 1/5: Fix some places noticed by Stefan Hajnoczi PATCH 2/5: Rebase to upstream PATCH 3/5: Fix includes v3: PATCH 1/5: Fix the place about GUID, add emails of the authors
Re: [Qemu-devel] [Qemu-block] [PATCH 2/5] configure: add dependency
On 12/22/2017 03:38 PM, Roman Kagan wrote:
On Mon, Dec 18, 2017 at 02:09:08PM +0300, Denis V. Lunev wrote:
From: Klim Kireev
This dependency is required for adequate Parallels images support.
Typically the disk consists of several images which are glued by
XML disk descriptor. Also XML hides inside several important parameters
which are not available in the image header.
The patch also adds clause to checkpatch.pl to understand libxml2 types.
Can't you get by with glib's GMarkup, to avoid extra dependencies?
https://developer.gnome.org/glib/stable/glib-Simple-XML-Subset-Parser.html
Roman.
Signed-off-by: Denis V. Lunev
Signed-off-by: Klim Kireev
Signed-off-by: Edgar Kaziakhmedov
CC: Stefan Hajnoczi
---
configure | 27 +++
block/Makefile.objs | 2 ++
scripts/checkpatch.pl | 1 +
3 files changed, 30 insertions(+)
diff --git a/configure b/configure
index 0c6e757..e988fd0 100755
--- a/configure
+++ b/configure
@@ -422,6 +422,7 @@ tcmalloc="no"
jemalloc="no"
replication="yes"
vxhs=""
+libxml2=""
supported_cpu="no"
supported_os="no"
@@ -1275,6 +1276,10 @@ for opt do
;;
--enable-numa) numa="yes"
;;
+ --disable-libxml2) libxml2="no"
+ ;;
+ --enable-libxml2) libxml2="yes"
+ ;;
--disable-tcmalloc) tcmalloc="no"
;;
--enable-tcmalloc) tcmalloc="yes"
@@ -1548,6 +1553,7 @@ disabled with --disable-FEATURE, default is enabled if
available:
tpm TPM support
libssh2 ssh block device support
numalibnuma support
+ libxml2 for Parallels image format
tcmalloctcmalloc support
jemallocjemalloc support
replication replication support
@@ -1592,6 +1598,20 @@ if test "$ARCH" = "unknown"; then
fi
fi
+# check for libxml2
+if test "$libxml2" != "no" ; then
+if $pkg_config --exists libxml-2.0; then
+libxml2="yes"
+libxml2_cflags=$($pkg_config --cflags libxml-2.0)
+libxml2_libs=$($pkg_config --libs libxml-2.0)
+else
+if test "$libxml2" = "yes"; then
+feature_not_found "libxml2" "Install libxml2 devel"
+fi
+libxml2="no"
+fi
+fi
+
# Consult white-list to determine whether to enable werror
# by default. Only enable by default for git builds
if test -z "$werror" ; then
@@ -5549,6 +5569,7 @@ echo "lzo support $lzo"
echo "snappy support$snappy"
echo "bzip2 support $bzip2"
echo "NUMA host support $numa"
+echo "libxml2 $libxml2"
echo "tcmalloc support $tcmalloc"
echo "jemalloc support $jemalloc"
echo "avx2 optimization $avx2_opt"
@@ -6208,6 +6229,12 @@ if test "$have_rtnetlink" = "yes" ; then
echo "CONFIG_RTNETLINK=y" >> $config_host_mak
fi
+if test "$libxml2" = "yes" ; then
+ echo "CONFIG_LIBXML2=y" >> $config_host_mak
+ echo "LIBXML2_CFLAGS=$libxml2_cflags" >> $config_host_mak
+ echo "LIBXML2_LIBS=$libxml2_libs" >> $config_host_mak
+fi
+
if test "$replication" = "yes" ; then
echo "CONFIG_REPLICATION=y" >> $config_host_mak
fi
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 6eaf78a..a73387f 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -47,3 +47,5 @@ block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
dmg-bz2.o-libs := $(BZIP2_LIBS)
qcow.o-libs:= -lz
linux-aio.o-libs := -laio
+parallels.o-cflags := $(LIBXML2_CFLAGS)
+parallels.o-libs := $(LIBXML2_LIBS)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 34df753..e76cc85 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -265,6 +265,7 @@ our @typeList = (
qr{${Ident}_handler_fn},
qr{target_(?:u)?long},
qr{hwaddr},
+ qr{xml${Ident}},
);
# This can be modified by sub possible. Since it can be empty, be careful
--
2.7.4
It is inconvenient, because GMarkup sees XML files as series of event,
while for our purposes the tree model is much more preferable.
Also libvirt depends on libxml2, so it is installed in most cases.
Re: [Qemu-devel] [PATCH 1/5] docs/interop/prl-xml: description of Parallels Disk format
On 01/04/2018 02:34 PM, Stefan Hajnoczi wrote: On Mon, Dec 18, 2017 at 02:09:07PM +0300, Denis V. Lunev wrote: From: Klim Kireev This patch adds main information about Parallels Disk format, which consists of DiskDescriptor.xml and other files. Signed-off-by: Edgar Kaziakhmedov Signed-off-by: Klim Kireev Signed-off-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Denis V. Lunev CC: Stefan Hajnoczi --- docs/interop/prl-xml.txt | 155 +++ 1 file changed, 155 insertions(+) create mode 100644 docs/interop/prl-xml.txt diff --git a/docs/interop/prl-xml.txt b/docs/interop/prl-xml.txt new file mode 100644 index 000..8ccb91a --- /dev/null +++ b/docs/interop/prl-xml.txt @@ -0,0 +1,155 @@ += License = + +Copyright (c) 2015 Denis Lunev +Copyright (c) 2015 Vladimir Sementsov-Ogievskiy +Copyright (c) 2016-2017 Klim Kireev +Copyright (c) 2016-2017 Edgar Kaziakhmedov + +This work is licensed under the terms of the GNU GPL, version 2 or later. +See the COPYING file in the top-level directory. + +This specification contains minimal information about Parallels Disk Format, +which is enough to proper work with QEMU. Nevertheless, Parallels Cloud Server +and Parallels Desktop are able to add some unspecified nodes to xml and use +them, but they are for internal work and don't affect functionality. Also it +uses auxiliary xml "Snapshot.xml", which allows to store optional snapshot +information, but it doesn't influence open/read/write functionality. QEMU and +other software should not use unspecified here fields and Snapshot.xml file s/unspecified here fields/fields not covered in this document/ +and must leave them as is. + += Parallels Disk Format = + +Parallels disk consists of two parts: the set of snapshots and the disk +descriptor file, which stores information about all files and snapshots. + +== Definitions == +Snapshot a record of the contents captured at a particular time, + capable of storing current state. A snapshot has UUID and + parent UUID. + + Snapshot imagean overlay representing the difference between this + snapshot and some earlier snapshot + +Overlayan image storing the different sectors between two captured + states. + + Root image snapshot image without parent, the root of snapshot tree. s/without parent/with no parent/ + +Storagea special conception of data, which consists of disk + parameters and a list of images. One of this image is root, + others are overlays. Images must be expandable (parallels + image file), however root image could be expandable or + plain. There may be more then one storage in the Parallels s/then/than/ + disk and it is defined as a split image. I was having trouble understanding this paragraph. At this point I can begin to see that split images consist of multiple storages. That makes the concept of storage clearer. Perhaps rephrase this paragraph as: the backing storage for a subset of the virtual disk. When there is more than one storage in a Parallels disk then that is referred to as a split image. Each storage consists of disk parameters and a list of images. The list of images always contains a root image and may also contain overlays. The root image can be an expandable Parallels image file or plain. Overlays must be expandable. + In this case every storage covers specific address + space area of the disk and has its particular root image. + Split images are not considered here and isn't supported s/isn't/aren't/ + in QEMU. + + Description DiskDescriptor.xml stores information about disk parameters, + file snapshots, storages. + + Top The overlay between actual state and some previous snapshot. + SnapshotIt is not a snapshot in classical meaning. To make this clearer the last line could be: It is not a snapshot in the classical sense because it serves as the active image that the guest writes to. + +Sector a 512-byte data chunk. + +== Description file == +All information is placed in single XML section Parallels_disk_image. +The section has only one attribute "Version", that must be 1.0. +General structure of DiskDescriptor.xml: + + + +... + + +... + + +... + + + +== Disk_Parameters section == +The Disk_Parameters section describes the physical layout of the virtual disk +and some general settings. + +The Disk_Parameters section MUST contain the following subsections: +* Disk_size - number of sectors in the disk, + desired size of the disk +* Cylinders - number of the disk cylinders +* Heads - number of the disk heads +
Re: [Qemu-devel] "make check -j4" hangs
On 02/09/2018 02:55 PM, Paolo Bonzini wrote: On 08/02/2018 22:54, Alistair Francis wrote: On Thu, Feb 8, 2018 at 1:12 PM, Peter Maydell wrote: On 8 February 2018 at 19:13, Thomas Huth wrote: I'm currently facing some issues with "make check -j4" (i.e. running the tests in parallel). Git bisect blames this commit - though I'm not sure whether this is really the right one ... Starting with this commit, I saw hangs in test-filter-redirector, but git master rather seems to hang with vhost-user-test instead. Stefan also had issues with "make check -j4" today, so it's apparently not only me ... can somebody else reproduce the problem with the git master branch? Yeah, I was seeing odd make check hangs with parallelization enabled too, on vhost-user-test... (my builds for merge tests are almost all non-parallelized I think). I don't only see hangs I also am seeing this: qemu-system-i386: -chardev socket,id=chr-test,path=/tmp/vhost-test-wxuOIX/test.sock: Failed to connect socket /tmp/vhost-test-wxuOIX/test.sock: No such file or directory socket_accept failed: Resource temporarily unavailable ** ERROR:tests/libqtest.c:218:qtest_init_without_qmp_handshake: assertion failed: (s->fd >= 0 && s->qmp_fd >= 0) GTester: last random seed: R02Sa67ba85090274c01deb9c2d15b044c10 It isn't 100% reproducible though. It may be something like commit 8f6d701044bc87197aac8aa2a627d70ce8471726. In the meanwhile we should revert it. Paolo I just have reverted my 2 commits and after that make check -j32 hangs with GTester: last random seed: R02Sb95a3bf6ab4c05540cec188081a7cc2a in vhost-user-test so it is not my fault
Re: [Qemu-devel] [PATCH v2] vnc: fix segfault in closed connection handling
On 01/31/2018 07:44 PM, Daniel P. Berrangé wrote:
On Wed, Jan 31, 2018 at 07:25:21PM +0300, Klim Kireev wrote:
On one of our client's node, due to trying to read from closed ioc,
a segmentation fault occured. Corresponding backtrace:
0 object_get_class (obj=obj@entry=0x0)
1 qio_channel_readv_full (ioc=0x0, iov=0x7ffe55277180 ...
2 qio_channel_read (ioc= ...
3 vnc_client_read_buf (vs=vs@entry=0x55625f3c6000, ...
4 vnc_client_read_plain (vs=0x55625f3c6000)
5 vnc_client_read (vs=0x55625f3c6000)
6 vnc_client_io (ioc=, condition=G_IO_IN, ...
7 g_main_dispatch (context=0x556251568a50)
8 g_main_context_dispatch (context=context@entry=0x556251568a50)
9 glib_pollfds_poll ()
10 os_host_main_loop_wait (timeout=)
11 main_loop_wait (nonblocking=nonblocking@entry=0)
12 main_loop () at vl.c:1909
13 main (argc=, argv=, ...
Having analyzed the coredump, I understood that the reason is that
ioc_tag is reset on vnc_disconnect_start and ioc is cleaned
in vnc_disconnect_finish. Between these two events due to some
reasons the ioc_tag was set again and after vnc_disconnect_finish
the handler is running with freed ioc,
which led to the segmentation fault.
The patch checks vs->disconnecting in places where we call
qio_channel_add_watch to prevent such an occurrence.
Signed-off-by: Klim Kireev
---
Changelog:
v2: Attach the backtrace
v3: Change checks
ui/vnc.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index 33b087221f..708204fa7e 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1407,13 +1407,19 @@ static void vnc_client_write_locked(VncState *vs)
} else
#endif /* CONFIG_VNC_SASL */
{
-vnc_client_write_plain(vs);
+if (vs->disconnecting == FALSE) {
+vnc_client_write_plain(vs);
+} else {
+if (vs->ioc_tag != 0) {
+g_source_remove(vs->ioc_tag);
+vs->ioc_tag = 0;
+}
+}
}
}
I'm not sure it is safe to only do the check in the else{} branch
of this code. If this code is reachable, then I think the SASL
branch will cause the same crash problems too. I think probably
need to push the checks up a level or two in the caller stack...
static void vnc_client_write(VncState *vs)
{
-
vnc_lock_output(vs);
if (vs->output.offset) {
vnc_client_write_locked(vs);
@@ -1421,8 +1427,12 @@ static void vnc_client_write(VncState *vs)
if (vs->ioc_tag) {
g_source_remove(vs->ioc_tag);
}
-vs->ioc_tag = qio_channel_add_watch(
-vs->ioc, G_IO_IN, vnc_client_io, vs, NULL);
+if (vs->disconnecting == FALSE) {
+vs->ioc_tag = qio_channel_add_watch(
+vs->ioc, G_IO_IN, vnc_client_io, vs, NULL);
+} else {
+vs->ioc_tag = 0;
+}
}
vnc_unlock_output(vs);
}
...I think perhaps we should do the check in the vnc_client_io()
method, and also in the vnc_flush() method.
I think we also need to put a check in the vnc_jobs_consume_buffer()
method, which can be triggered from a bottom-half.
Thank you for your advice,
in both places there is a check vs->ioc != NULL, so we don't need check
it there
Regards,
Daniel
Re: [Qemu-devel] [PATCH v2] vnc: fix segfault in closed connection handling
On 02/07/2018 11:42 AM, klim wrote:
On 01/31/2018 07:44 PM, Daniel P. Berrangé wrote:
On Wed, Jan 31, 2018 at 07:25:21PM +0300, Klim Kireev wrote:
On one of our client's node, due to trying to read from closed ioc,
a segmentation fault occured. Corresponding backtrace:
0 object_get_class (obj=obj@entry=0x0)
1 qio_channel_readv_full (ioc=0x0, iov=0x7ffe55277180 ...
2 qio_channel_read (ioc= ...
3 vnc_client_read_buf (vs=vs@entry=0x55625f3c6000, ...
4 vnc_client_read_plain (vs=0x55625f3c6000)
5 vnc_client_read (vs=0x55625f3c6000)
6 vnc_client_io (ioc=, condition=G_IO_IN, ...
7 g_main_dispatch (context=0x556251568a50)
8 g_main_context_dispatch (context=context@entry=0x556251568a50)
9 glib_pollfds_poll ()
10 os_host_main_loop_wait (timeout=)
11 main_loop_wait (nonblocking=nonblocking@entry=0)
12 main_loop () at vl.c:1909
13 main (argc=, argv=, ...
Having analyzed the coredump, I understood that the reason is that
ioc_tag is reset on vnc_disconnect_start and ioc is cleaned
in vnc_disconnect_finish. Between these two events due to some
reasons the ioc_tag was set again and after vnc_disconnect_finish
the handler is running with freed ioc,
which led to the segmentation fault.
The patch checks vs->disconnecting in places where we call
qio_channel_add_watch to prevent such an occurrence.
Signed-off-by: Klim Kireev
---
Changelog:
v2: Attach the backtrace
v3: Change checks
ui/vnc.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index 33b087221f..708204fa7e 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1407,13 +1407,19 @@ static void vnc_client_write_locked(VncState
*vs)
} else
#endif /* CONFIG_VNC_SASL */
{
- vnc_client_write_plain(vs);
+ if (vs->disconnecting == FALSE) {
+ vnc_client_write_plain(vs);
+ } else {
+ if (vs->ioc_tag != 0) {
+ g_source_remove(vs->ioc_tag);
+ vs->ioc_tag = 0;
+ }
+ }
}
}
I'm not sure it is safe to only do the check in the else{} branch
of this code. If this code is reachable, then I think the SASL
branch will cause the same crash problems too. I think probably
need to push the checks up a level or two in the caller stack...
static void vnc_client_write(VncState *vs)
{
-
vnc_lock_output(vs);
if (vs->output.offset) {
vnc_client_write_locked(vs);
@@ -1421,8 +1427,12 @@ static void vnc_client_write(VncState *vs)
if (vs->ioc_tag) {
g_source_remove(vs->ioc_tag);
}
- vs->ioc_tag = qio_channel_add_watch(
- vs->ioc, G_IO_IN, vnc_client_io, vs, NULL);
+ if (vs->disconnecting == FALSE) {
+ vs->ioc_tag = qio_channel_add_watch(
+ vs->ioc, G_IO_IN, vnc_client_io, vs, NULL);
+ } else {
+ vs->ioc_tag = 0;
+ }
}
vnc_unlock_output(vs);
}
...I think perhaps we should do the check in the vnc_client_io()
method, and also in the vnc_flush() method.
I think we also need to put a check in the vnc_jobs_consume_buffer()
method, which can be triggered from a bottom-half.
Thank you for your advice,
in both places there is a check vs->ioc != NULL, so we don't need
check it there
But ioc_tag can be set in those places, so probably you are right and we
need to check it there
Regards,
Daniel
[Qemu-devel] [PATCH v4] chardev/char-socket: add POLLHUP handler
The following behavior was observed for QEMU configured by libvirt
to use guest agent as usual for the guests without virtio-serial
driver (Windows or the guest remaining in BIOS stage).
In QEMU on first connect to listen character device socket
the listen socket is removed from poll just after the accept().
virtio_serial_guest_ready() returns 0 and the descriptor
of the connected Unix socket is removed from poll and it will
not be present in poll() until the guest will initialize the driver
and change the state of the serial to "guest connected".
In libvirt connect() to guest agent is performed on restart and
is run under VM state lock. Connect() is blocking and can
wait forever.
In this case libvirt can not perform ANY operation on that VM.
The bug can be easily reproduced this way:
Terminal 1:
qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev
socket,id=serial1,path=/tmp/console.sock,server,nowait
(virtio-serial and isa-serial also fit)
Terminal 2:
minicom -D unix\#/tmp/console.sock
(type something and press enter)
C-a x (to exit)
Do 3 times:
minicom -D unix\#/tmp/console.sock
C-a x
It needs 4 connections, because the first one is accepted by QEMU, then two are
queued by
the kernel, and the 4th blocks.
The problem is that QEMU doesn't add a read watcher after succesful read
until the guest device wants to acquire recieved data, so
I propose to install a separate pullhup watcher regardless of
whether the device waits for data or not.
Signed-off-by: Klim Kireev
---
Changelog:
v2: Remove timer as a redundant feature
v3: Remove read call and return G_SOURCE_REMOVE
v4: Move to GSource API
chardev/char-socket.c | 22 ++
dtc | 2 +-
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 77cdf487eb..062d131079 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -42,6 +42,7 @@ typedef struct {
QIOChannel *ioc; /* Client I/O channel */
QIOChannelSocket *sioc; /* Client master channel */
QIONetListener *listener;
+GSource *hup_source;
QCryptoTLSCreds *tls_creds;
int connected;
int max_size;
@@ -352,6 +353,12 @@ static void tcp_chr_free_connection(Chardev *chr)
s->read_msgfds_num = 0;
}
+if (s->hup_source != NULL) {
+g_source_destroy(s->hup_source);
+g_source_unref(s->hup_source);
+s->hup_source = NULL;
+}
+
tcp_set_msgfds(chr, NULL, 0);
remove_fd_in_watch(chr);
object_unref(OBJECT(s->sioc));
@@ -455,6 +462,15 @@ static gboolean tcp_chr_read(QIOChannel *chan,
GIOCondition cond, void *opaque)
return TRUE;
}
+static gboolean tcp_chr_hup(QIOChannel *channel,
+ GIOCondition cond,
+ void *opaque)
+{
+Chardev *chr = CHARDEV(opaque);
+tcp_chr_disconnect(chr);
+return G_SOURCE_REMOVE;
+}
+
static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -528,6 +544,12 @@ static void tcp_chr_connect(void *opaque)
tcp_chr_read,
chr, chr->gcontext);
}
+
+s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
+g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
+ chr, NULL);
+g_source_attach(s->hup_source, chr->gcontext);
+
qemu_chr_be_event(chr, CHR_EVENT_OPENED);
}
diff --git a/dtc b/dtc
index e54388015a..558cd81bdd 16
--- a/dtc
+++ b/dtc
@@ -1 +1 @@
-Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42
+Subproject commit 558cd81bdd432769b59bff01240c44f82cfb1a9d
--
2.13.6
[Qemu-devel] [PATCH v5] chardev/char-socket: add POLLHUP handler
The following behavior was observed for QEMU configured by libvirt
to use guest agent as usual for the guests without virtio-serial
driver (Windows or the guest remaining in BIOS stage).
In QEMU on first connect to listen character device socket
the listen socket is removed from poll just after the accept().
virtio_serial_guest_ready() returns 0 and the descriptor
of the connected Unix socket is removed from poll and it will
not be present in poll() until the guest will initialize the driver
and change the state of the serial to "guest connected".
In libvirt connect() to guest agent is performed on restart and
is run under VM state lock. Connect() is blocking and can
wait forever.
In this case libvirt can not perform ANY operation on that VM.
The bug can be easily reproduced this way:
Terminal 1:
qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev
socket,id=serial1,path=/tmp/console.sock,server,nowait
(virtio-serial and isa-serial also fit)
Terminal 2:
minicom -D unix\#/tmp/console.sock
(type something and press enter)
C-a x (to exit)
Do 3 times:
minicom -D unix\#/tmp/console.sock
C-a x
It needs 4 connections, because the first one is accepted by QEMU, then two are
queued by
the kernel, and the 4th blocks.
The problem is that QEMU doesn't add a read watcher after succesful read
until the guest device wants to acquire recieved data, so
I propose to install a separate pullhup watcher regardless of
whether the device waits for data or not.
Signed-off-by: Klim Kireev
---
Changelog:
v2: Remove timer as a redundant feature
v3: Remove read call and return G_SOURCE_REMOVE
v4: Move to GSource API
v5: Fix git typos
chardev/char-socket.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 77cdf487eb..a340af6cd3 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -42,6 +42,7 @@ typedef struct {
QIOChannel *ioc; /* Client I/O channel */
QIOChannelSocket *sioc; /* Client master channel */
QIONetListener *listener;
+GSource *hup_source;
QCryptoTLSCreds *tls_creds;
int connected;
int max_size;
@@ -352,6 +353,12 @@ static void tcp_chr_free_connection(Chardev *chr)
s->read_msgfds_num = 0;
}
+if (s->hup_source != NULL) {
+g_source_destroy(s->hup_source);
+g_source_unref(s->hup_source);
+s->hup_source = NULL;
+}
+
tcp_set_msgfds(chr, NULL, 0);
remove_fd_in_watch(chr);
object_unref(OBJECT(s->sioc));
@@ -455,6 +462,15 @@ static gboolean tcp_chr_read(QIOChannel *chan,
GIOCondition cond, void *opaque)
return TRUE;
}
+static gboolean tcp_chr_hup(QIOChannel *channel,
+ GIOCondition cond,
+ void *opaque)
+{
+Chardev *chr = CHARDEV(opaque);
+tcp_chr_disconnect(chr);
+return G_SOURCE_REMOVE;
+}
+
static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -528,6 +544,12 @@ static void tcp_chr_connect(void *opaque)
tcp_chr_read,
chr, chr->gcontext);
}
+
+s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
+g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
+ chr, NULL);
+g_source_attach(s->hup_source, chr->gcontext);
+
qemu_chr_be_event(chr, CHR_EVENT_OPENED);
}
--
2.13.6
[Qemu-devel] [PATCH] vnc: fix segfault in closed connection handling
On one of our client's node, due to trying to read from closed ioc,
a segmentation fault occured. Corresponding backtrace:
Having analyzed the coredump, I understood that the reason is that
ioc_tag is reset on vnc_disconnect_start and ioc is cleaned
in vnc_disconnect_finish. Between these two events due to some
reasons the ioc_tag was set again and after vnc_disconnect_finish
the handler is running with freed ioc,
which led to the segmentation fault.
I suggest to check ioc_tag in vnc_disconnect_finish to prevent such
an occurrence.
Signed-off-by: Klim Kireev
---
ui/vnc.c | 4
1 file changed, 4 insertions(+)
diff --git a/ui/vnc.c b/ui/vnc.c
index 33b087221f..b8bf0180cb 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1270,6 +1270,10 @@ void vnc_disconnect_finish(VncState *vs)
}
g_free(vs->lossy_rect);
+if (vs->ioc_tag) {
+g_source_remove(vs->ioc_tag);
+vs->ioc_tag = 0;
+}
object_unref(OBJECT(vs->ioc));
vs->ioc = NULL;
object_unref(OBJECT(vs->sioc));
--
2.14.3
[Qemu-devel] [PATCH v2] vnc: fix segfault in closed connection handling
On one of our client's node, due to trying to read from closed ioc,
a segmentation fault occured. Corresponding backtrace:
0 object_get_class (obj=obj@entry=0x0)
1 qio_channel_readv_full (ioc=0x0, iov=0x7ffe55277180 ...
2 qio_channel_read (ioc= ...
3 vnc_client_read_buf (vs=vs@entry=0x55625f3c6000, ...
4 vnc_client_read_plain (vs=0x55625f3c6000)
5 vnc_client_read (vs=0x55625f3c6000)
6 vnc_client_io (ioc=, condition=G_IO_IN, ...
7 g_main_dispatch (context=0x556251568a50)
8 g_main_context_dispatch (context=context@entry=0x556251568a50)
9 glib_pollfds_poll ()
10 os_host_main_loop_wait (timeout=)
11 main_loop_wait (nonblocking=nonblocking@entry=0)
12 main_loop () at vl.c:1909
13 main (argc=, argv=, ...
Having analyzed the coredump, I understood that the reason is that
ioc_tag is reset on vnc_disconnect_start and ioc is cleaned
in vnc_disconnect_finish. Between these two events due to some
reasons the ioc_tag was set again and after vnc_disconnect_finish
the handler is running with freed ioc,
which led to the segmentation fault.
I suggest to check ioc_tag in vnc_disconnect_finish to prevent such
an occurrence.
Signed-off-by: Klim Kireev
---
Changelog:
v2: Apply the backtrace
ui/vnc.c | 4
1 file changed, 4 insertions(+)
diff --git a/ui/vnc.c b/ui/vnc.c
index 33b087221f..b8bf0180cb 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1270,6 +1270,10 @@ void vnc_disconnect_finish(VncState *vs)
}
g_free(vs->lossy_rect);
+if (vs->ioc_tag) {
+g_source_remove(vs->ioc_tag);
+vs->ioc_tag = 0;
+}
object_unref(OBJECT(vs->ioc));
vs->ioc = NULL;
object_unref(OBJECT(vs->sioc));
--
2.14.3
[Qemu-devel] [PATCH v2] vnc: fix segfault in closed connection handling
On one of our client's node, due to trying to read from closed ioc,
a segmentation fault occured. Corresponding backtrace:
0 object_get_class (obj=obj@entry=0x0)
1 qio_channel_readv_full (ioc=0x0, iov=0x7ffe55277180 ...
2 qio_channel_read (ioc= ...
3 vnc_client_read_buf (vs=vs@entry=0x55625f3c6000, ...
4 vnc_client_read_plain (vs=0x55625f3c6000)
5 vnc_client_read (vs=0x55625f3c6000)
6 vnc_client_io (ioc=, condition=G_IO_IN, ...
7 g_main_dispatch (context=0x556251568a50)
8 g_main_context_dispatch (context=context@entry=0x556251568a50)
9 glib_pollfds_poll ()
10 os_host_main_loop_wait (timeout=)
11 main_loop_wait (nonblocking=nonblocking@entry=0)
12 main_loop () at vl.c:1909
13 main (argc=, argv=, ...
Having analyzed the coredump, I understood that the reason is that
ioc_tag is reset on vnc_disconnect_start and ioc is cleaned
in vnc_disconnect_finish. Between these two events due to some
reasons the ioc_tag was set again and after vnc_disconnect_finish
the handler is running with freed ioc,
which led to the segmentation fault.
The patch checks vs->disconnecting in places where we call
qio_channel_add_watch to prevent such an occurrence.
Signed-off-by: Klim Kireev
---
Changelog:
v2: Attach the backtrace
v3: Change checks
ui/vnc.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index 33b087221f..708204fa7e 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1407,13 +1407,19 @@ static void vnc_client_write_locked(VncState *vs)
} else
#endif /* CONFIG_VNC_SASL */
{
-vnc_client_write_plain(vs);
+if (vs->disconnecting == FALSE) {
+vnc_client_write_plain(vs);
+} else {
+if (vs->ioc_tag != 0) {
+g_source_remove(vs->ioc_tag);
+vs->ioc_tag = 0;
+}
+}
}
}
static void vnc_client_write(VncState *vs)
{
-
vnc_lock_output(vs);
if (vs->output.offset) {
vnc_client_write_locked(vs);
@@ -1421,8 +1427,12 @@ static void vnc_client_write(VncState *vs)
if (vs->ioc_tag) {
g_source_remove(vs->ioc_tag);
}
-vs->ioc_tag = qio_channel_add_watch(
-vs->ioc, G_IO_IN, vnc_client_io, vs, NULL);
+if (vs->disconnecting == FALSE) {
+vs->ioc_tag = qio_channel_add_watch(
+vs->ioc, G_IO_IN, vnc_client_io, vs, NULL);
+} else {
+vs->ioc_tag = 0;
+}
}
vnc_unlock_output(vs);
}
--
2.14.3
[Qemu-devel] [PATCH] tests/test-filter-redirector: move close()
Since we have separate handler on POLLHUP, which drops data after closing the connection we need to fix this test, because it sends data and instantly close the socket creating race condition. In some cases on other end of socket client closes it faster than reads data. To prevent it I suggest to close socket after recieving. Signed-off-by: Klim Kireev --- tests/test-filter-redirector.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c index f2566144cf..fbaf19bbd8 100644 --- a/tests/test-filter-redirector.c +++ b/tests/test-filter-redirector.c @@ -186,7 +186,6 @@ static void test_redirector_rx(void) ret = iov_send(send_sock, iov, 2, 0, sizeof(size) + sizeof(send_buf)); g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size)); -close(send_sock); ret = qemu_recv(backend_sock[0], &len, sizeof(len), 0); g_assert_cmpint(ret, ==, sizeof(len)); @@ -197,6 +196,7 @@ static void test_redirector_rx(void) ret = qemu_recv(backend_sock[0], recv_buf, len, 0); g_assert_cmpstr(recv_buf, ==, send_buf); +close(send_sock); g_free(recv_buf); unlink(sock_path0); unlink(sock_path1); -- 2.14.3
[Qemu-devel] [PATCH] qemu-img: add the simplest format recognition
Now, if you type something like
qemu-img create disk.qcow2 1G
or
qemu-img dd if=/dev/sda of=disk.qcow2
it creates a raw image and if you need you should
manually specify an image format with -f qcow2. It would
be more convenient if it could be detected from an extension.
This patch adds a simple heuristic to recognize the image format
for qcow, qcow2, vmdk, vhdx, vdi
Signed-off-by: Klim Kireev
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
qemu-img.c | 24 ++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 02a6e27beb..0a659d2257 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -421,11 +421,25 @@ static int64_t cvtnum(const char *s)
return value;
}
+static const char *get_format(const char *filename)
+{
+const char *fmt = strrchr(filename, '.');
+if (fmt == NULL) {
+return "raw";
+}
+fmt++;
+if (bdrv_find_format(fmt) != NULL) {
+return fmt;
+} else {
+return "raw";
+}
+}
+
static int img_create(int argc, char **argv)
{
int c;
uint64_t img_size = -1;
-const char *fmt = "raw";
+const char *fmt = NULL;
const char *base_fmt = NULL;
const char *filename;
const char *base_filename = NULL;
@@ -496,6 +510,9 @@ static int img_create(int argc, char **argv)
/* Get the filename */
filename = (optind < argc) ? argv[optind] : NULL;
+if (fmt == NULL) {
+fmt = get_format(filename);
+}
if (options && has_help_option(options)) {
g_free(options);
return print_block_option_help(filename, fmt);
@@ -4181,7 +4198,7 @@ static int img_dd(int argc, char **argv)
Error *local_err = NULL;
bool image_opts = false;
int c, i;
-const char *out_fmt = "raw";
+const char *out_fmt = NULL;
const char *fmt = NULL;
int64_t size = 0;
int64_t block_count = 0, out_pos, in_pos;
@@ -4308,6 +4325,9 @@ static int img_dd(int argc, char **argv)
goto out;
}
+if (out_fmt == NULL) {
+out_fmt = get_format(out.filename);
+}
drv = bdrv_find_format(out_fmt);
if (!drv) {
error_report("Unknown file format");
--
2.13.6
[Qemu-devel] [PATCH v2] qemu-img: add the simplest format recognition
Now, if you type something like
qemu-img create disk.qcow2 1G
or
qemu-img dd if=/dev/sda of=disk.qcow2
it creates a raw image and if you need you should
manually specify an image format with -f qcow2. It would
be more convenient if it could be detected from an extension.
This patch adds a simple heuristic to recognize the image format
for qcow, qcow2, vmdk, vhdx, vdi
It warns users about guessed format and informs them about '-f' option.
Signed-off-by: Klim Kireev
---
qemu-img.c | 21 +++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 02a6e27beb..ac1adf1582 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -421,11 +421,22 @@ static int64_t cvtnum(const char *s)
return value;
}
+static const char *get_format(const char *filename)
+{
+const char *fmt = strrchr(filename, '.');
+if (fmt == NULL || bdrv_find_format(++fmt) == NULL) {
+fmt = "raw";
+}
+printf("!!! %s format was detected.\n"
+ "!!! If you meant another format, specify it with -f.\n", fmt);
+return fmt;
+}
+
static int img_create(int argc, char **argv)
{
int c;
uint64_t img_size = -1;
-const char *fmt = "raw";
+const char *fmt = NULL;
const char *base_fmt = NULL;
const char *filename;
const char *base_filename = NULL;
@@ -496,6 +507,9 @@ static int img_create(int argc, char **argv)
/* Get the filename */
filename = (optind < argc) ? argv[optind] : NULL;
+if (fmt == NULL) {
+fmt = get_format(filename);
+}
if (options && has_help_option(options)) {
g_free(options);
return print_block_option_help(filename, fmt);
@@ -4181,7 +4195,7 @@ static int img_dd(int argc, char **argv)
Error *local_err = NULL;
bool image_opts = false;
int c, i;
-const char *out_fmt = "raw";
+const char *out_fmt = NULL;
const char *fmt = NULL;
int64_t size = 0;
int64_t block_count = 0, out_pos, in_pos;
@@ -4308,6 +4322,9 @@ static int img_dd(int argc, char **argv)
goto out;
}
+if (out_fmt == NULL) {
+out_fmt = get_format(out.filename);
+}
drv = bdrv_find_format(out_fmt);
if (!drv) {
error_report("Unknown file format");
--
2.13.6
[Qemu-devel] [PATCH v3] qemu-img: add the simplest format recognition
Now, if you type something like
qemu-img create disk.qcow2 1G
or
qemu-img dd if=/dev/sda of=disk.qcow2
it creates a raw image and if you need you should
manually specify an image format with -f qcow2. It would
be more convenient if it could be assumed from an extension.
This patch adds a simple heuristic to recognize the image format
for qcow, qcow2, vmdk, vhdx, vdi
It warns users about guessed format and informs them about '-f' option.
Signed-off-by: Klim Kireev
---
qemu-img.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 02a6e27beb..4ec04f5c86 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -421,11 +421,21 @@ static int64_t cvtnum(const char *s)
return value;
}
+static const char *get_format(const char *filename)
+{
+const char *fmt = strrchr(filename, '.');
+if (fmt == NULL || bdrv_find_format(++fmt) == NULL) {
+fmt = "raw";
+}
+warn_report("No format specified with -f, assuming %s.", fmt);
+return fmt;
+}
+
static int img_create(int argc, char **argv)
{
int c;
uint64_t img_size = -1;
-const char *fmt = "raw";
+const char *fmt = NULL;
const char *base_fmt = NULL;
const char *filename;
const char *base_filename = NULL;
@@ -496,6 +506,9 @@ static int img_create(int argc, char **argv)
/* Get the filename */
filename = (optind < argc) ? argv[optind] : NULL;
+if (fmt == NULL) {
+fmt = get_format(filename);
+}
if (options && has_help_option(options)) {
g_free(options);
return print_block_option_help(filename, fmt);
@@ -4181,7 +4194,7 @@ static int img_dd(int argc, char **argv)
Error *local_err = NULL;
bool image_opts = false;
int c, i;
-const char *out_fmt = "raw";
+const char *out_fmt = NULL;
const char *fmt = NULL;
int64_t size = 0;
int64_t block_count = 0, out_pos, in_pos;
@@ -4308,6 +4321,9 @@ static int img_dd(int argc, char **argv)
goto out;
}
+if (out_fmt == NULL) {
+out_fmt = get_format(out.filename);
+}
drv = bdrv_find_format(out_fmt);
if (!drv) {
error_report("Unknown file format");
--
2.13.6
[Qemu-devel] [PATCH v2] chardev/char-socket: add POLLHUP handler
The following behavior was observed for QEMU configured by libvirt
to use guest agent as usual for the guests without virtio-serial
driver (Windows or the guest remaining in BIOS stage).
In QEMU on first connect to listen character device socket
the listen socket is removed from poll just after the accept().
virtio_serial_guest_ready() returns 0 and the descriptor
of the connected Unix socket is removed from poll and it will
not be present in poll() until the guest will initialize the driver
and change the state of the serial to "guest connected".
In libvirt connect() to guest agent is performed on restart and
is run under VM state lock. Connect() is blocking and can
wait forever.
In this case libvirt can not perform ANY operation on that VM.
The bug can be easily reproduced this way:
Terminal 1:
qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev
socket,id=serial1,path=/tmp/console.sock,server,nowait
(virtio-serial and isa-serial also fit)
Terminal 2:
minicom -D unix\#/tmp/console.sock
(type something and press enter)
C-a x (to exit)
Do 3 times:
minicom -D unix\#/tmp/console.sock
C-a x
It needs 4 connections, because the first one is accepted by QEMU, then two are
queued by
the kernel, and the 4th blocks.
The problem is that QEMU doesn't add a read watcher after succesful read
until the guest device wants to acquire recieved data, so
I propose to install a separate pullhup watcher regardless of
whether the device waits for data or not.
Signed-off-by: Klim Kireev
---
Changelog:
v2: Remove timer as a redundant feature
chardev/char-socket.c | 29 -
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 77cdf487eb..d3fe903ab6 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -42,6 +42,7 @@ typedef struct {
QIOChannel *ioc; /* Client I/O channel */
QIOChannelSocket *sioc; /* Client master channel */
QIONetListener *listener;
+guint hup_tag;
QCryptoTLSCreds *tls_creds;
int connected;
int max_size;
@@ -352,6 +353,11 @@ static void tcp_chr_free_connection(Chardev *chr)
s->read_msgfds_num = 0;
}
+if (s->hup_tag != 0) {
+g_source_remove(s->hup_tag);
+s->hup_tag = 0;
+}
+
tcp_set_msgfds(chr, NULL, 0);
remove_fd_in_watch(chr);
object_unref(OBJECT(s->sioc));
@@ -455,6 +461,19 @@ static gboolean tcp_chr_read(QIOChannel *chan,
GIOCondition cond, void *opaque)
return TRUE;
}
+static gboolean tcp_chr_hup(QIOChannel *channel,
+ GIOCondition cond,
+ void *opaque)
+{
+Chardev *chr = CHARDEV(opaque);
+SocketChardev *s = SOCKET_CHARDEV(chr);
+tcp_chr_read(channel, cond, opaque);
+if (s->connected != 0) {
+tcp_chr_disconnect(chr);
+}
+return TRUE;
+}
+
static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -528,6 +547,10 @@ static void tcp_chr_connect(void *opaque)
tcp_chr_read,
chr, chr->gcontext);
}
+if (s->hup_tag == 0) {
+s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP,
+ tcp_chr_hup, chr, NULL);
+}
qemu_chr_be_event(chr, CHR_EVENT_OPENED);
}
@@ -546,7 +569,11 @@ static void tcp_chr_update_read_handler(Chardev *chr)
tcp_chr_read, chr,
chr->gcontext);
}
-}
+if (s->hup_tag == 0) {
+s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP,
+ tcp_chr_hup, chr, NULL);
+}
+ }
typedef struct {
Chardev *chr;
--
2.14.3
[Qemu-devel] [PATCH v3] chardev/char-socket: add POLLHUP handler
The following behavior was observed for QEMU configured by libvirt
to use guest agent as usual for the guests without virtio-serial
driver (Windows or the guest remaining in BIOS stage).
In QEMU on first connect to listen character device socket
the listen socket is removed from poll just after the accept().
virtio_serial_guest_ready() returns 0 and the descriptor
of the connected Unix socket is removed from poll and it will
not be present in poll() until the guest will initialize the driver
and change the state of the serial to "guest connected".
In libvirt connect() to guest agent is performed on restart and
is run under VM state lock. Connect() is blocking and can
wait forever.
In this case libvirt can not perform ANY operation on that VM.
The bug can be easily reproduced this way:
Terminal 1:
qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev
socket,id=serial1,path=/tmp/console.sock,server,nowait
(virtio-serial and isa-serial also fit)
Terminal 2:
minicom -D unix\#/tmp/console.sock
(type something and press enter)
C-a x (to exit)
Do 3 times:
minicom -D unix\#/tmp/console.sock
C-a x
It needs 4 connections, because the first one is accepted by QEMU, then two are
queued by
the kernel, and the 4th blocks.
The problem is that QEMU doesn't add a read watcher after succesful read
until the guest device wants to acquire recieved data, so
I propose to install a separate pullhup watcher regardless of
whether the device waits for data or not.
Signed-off-by: Klim Kireev
---
Changelog:
v2: Remove timer as a redundant feature
v3: Remove read call and return G_SOURCE_REMOVE
chardev/char-socket.c | 25 -
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 77cdf487eb..83fa7b70f0 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -42,6 +42,7 @@ typedef struct {
QIOChannel *ioc; /* Client I/O channel */
QIOChannelSocket *sioc; /* Client master channel */
QIONetListener *listener;
+guint hup_tag;
QCryptoTLSCreds *tls_creds;
int connected;
int max_size;
@@ -352,6 +353,11 @@ static void tcp_chr_free_connection(Chardev *chr)
s->read_msgfds_num = 0;
}
+if (s->hup_tag != 0) {
+g_source_remove(s->hup_tag);
+s->hup_tag = 0;
+}
+
tcp_set_msgfds(chr, NULL, 0);
remove_fd_in_watch(chr);
object_unref(OBJECT(s->sioc));
@@ -455,6 +461,15 @@ static gboolean tcp_chr_read(QIOChannel *chan,
GIOCondition cond, void *opaque)
return TRUE;
}
+static gboolean tcp_chr_hup(QIOChannel *channel,
+ GIOCondition cond,
+ void *opaque)
+{
+Chardev *chr = CHARDEV(opaque);
+tcp_chr_disconnect(chr);
+return G_SOURCE_REMOVE;
+}
+
static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -528,6 +543,10 @@ static void tcp_chr_connect(void *opaque)
tcp_chr_read,
chr, chr->gcontext);
}
+if (s->hup_tag == 0) {
+s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP,
+ tcp_chr_hup, chr, NULL);
+}
qemu_chr_be_event(chr, CHR_EVENT_OPENED);
}
@@ -546,7 +565,11 @@ static void tcp_chr_update_read_handler(Chardev *chr)
tcp_chr_read, chr,
chr->gcontext);
}
-}
+if (s->hup_tag == 0) {
+s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP,
+ tcp_chr_hup, chr, NULL);
+}
+ }
typedef struct {
Chardev *chr;
--
2.14.3
[Qemu-devel] [PATCH] chardev/char-socket: add POLLHUP handler
The following behavior was observed for QEMU configured by libvirt
to use guest agent as usual for the guests without virtio-serial
driver (Windows or the guest remaining in BIOS stage).
In QEMU on first connect to listen character device socket
the listen socket is removed from poll just after the accept().
virtio_serial_guest_ready() returns 0 and the descriptor
of the connected Unix socket is removed from poll and it will
not be present in poll() until the guest will initialize the driver
and change the state of the serial to "guest connected".
In libvirt connect() to guest agent is performed on restart and
is run under VM state lock. Connect() is blocking and can
wait forever.
In this case libvirt can not perform ANY operation on that VM.
The bug can be easily reproduced this way:
Terminal 1:
qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev
socket,id=serial1,path=/tmp/console.sock,server,nowait
(virtio-serial and isa-serial also fit)
Terminal 2:
minicom -D unix\#/tmp/console.sock
(type something and pres enter)
C-a x (to exit)
Do 3 times:
minicom -D unix\#/tmp/console.sock
C-a x
It needs 4 connections, because the first one is accepted by QEMU, then two are
queued by
the kernel, and the 4th blocks.
The problem is that QEMU doesn't add a read watcher after succesful read
until the guest device wants to acquire recieved data, so
I propose to install a separate pullhup watcher regardless of
whether the device waits for data or not. After closing the connection,
the guest has a capability to read the data within timeout.
Signed-off-by: Klim Kireev
---
chardev/char-socket.c | 55 ++-
1 file changed, 54 insertions(+), 1 deletion(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 630a7f2995..eb120b2aab 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -37,6 +37,8 @@
#define TCP_MAX_FDS 16
+#define DATA_TIMEOUT 1
+
typedef struct {
Chardev parent;
QIOChannel *ioc; /* Client I/O channel */
@@ -57,6 +59,9 @@ typedef struct {
bool is_telnet;
bool is_tn3270;
+guint data_timer;
+guint destroy_tag;
+
guint reconnect_timer;
int64_t reconnect_time;
bool connect_err_reported;
@@ -341,6 +346,15 @@ static void tcp_chr_free_connection(Chardev *chr)
s->read_msgfds_num = 0;
}
+if (s->destroy_tag != 0) {
+g_source_remove(s->destroy_tag);
+s->destroy_tag = 0;
+}
+if (s->data_timer != 0) {
+g_source_remove(s->data_timer);
+s->data_timer = 0;
+}
+
tcp_set_msgfds(chr, NULL, 0);
remove_fd_in_watch(chr);
object_unref(OBJECT(s->sioc));
@@ -444,6 +458,37 @@ static gboolean tcp_chr_read(QIOChannel *chan,
GIOCondition cond, void *opaque)
return TRUE;
}
+static gboolean tcp_chr_data_timeout(gpointer opaque)
+{
+Chardev *chr = CHARDEV(opaque);
+
+if (tcp_chr_read_poll(chr) <= 0) {
+tcp_chr_disconnect(chr);
+return TRUE;
+} else {
+tcp_chr_read(NULL, 0, opaque);
+return TRUE;
+}
+}
+
+static gboolean tcp_chr_destroy(QIOChannel *channel,
+ GIOCondition cond,
+ void *opaque)
+{
+Chardev *chr = CHARDEV(opaque);
+SocketChardev *s = SOCKET_CHARDEV(chr);
+tcp_chr_read(channel, cond, opaque);
+if (s->connected != 0) {
+s->data_timer = g_timeout_add_seconds(DATA_TIMEOUT,
+ tcp_chr_data_timeout, chr);
+if (s->destroy_tag != 0) {
+g_source_remove(s->destroy_tag);
+s->destroy_tag = 0;
+}
+}
+return TRUE;
+}
+
static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -517,6 +562,10 @@ static void tcp_chr_connect(void *opaque)
tcp_chr_read,
chr, chr->gcontext);
}
+if (s->destroy_tag == 0) {
+s->destroy_tag = qio_channel_add_watch(s->ioc, G_IO_HUP,
+ tcp_chr_destroy, chr, NULL);
+}
qemu_chr_be_event(chr, CHR_EVENT_OPENED);
}
@@ -535,7 +584,11 @@ static void tcp_chr_update_read_handler(Chardev *chr)
tcp_chr_read, chr,
chr->gcontext);
}
-}
+if (s->destroy_tag == 0) {
+s->destroy_tag = qio_channel_add_watch(s->ioc, G_IO_HUP,
+ tcp_chr_destroy, chr, NULL);
+}
+ }
typedef struct {
Chardev *chr;
--
2.14.3
[Qemu-devel] [PATCH 0/5 v2] preparation for Parallels Disk xml driver
Parallels Desktop and Parallels Cloud Server uses images glued with the bundle description in XML format. This series contains very basic description of this XML files and makes preparations for actual implementation to be followed. Signed-off-by: Edgar Kaziakhmedov Signed-off-by: Klim Kireev Signed-off-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Denis V. Lunev Changelog: v2: PATCH 1/5: Fix some places noticed by Stefan Hajnoczi PATCH 2/5: Rebase to upstream PATCH 3/5: Fix includes
[Qemu-devel] [PATCH 3/5] block/parallels: move some structures into header
To implement xml format, some defines and structures
from parallels.c are required.
Signed-off-by: Klim Kireev
Reviewed-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Denis V. Lunev
CC: Stefan Hajnoczi
---
block/parallels.c | 53 +-
block/parallels.h | 86 +++
2 files changed, 87 insertions(+), 52 deletions(-)
create mode 100644 block/parallels.h
diff --git a/block/parallels.c b/block/parallels.c
index 9545761f49..f9a3b999ea 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -36,6 +36,7 @@
#include "qemu/bswap.h"
#include "qemu/bitmap.h"
#include "migration/blocker.h"
+#include "parallels.h"
/**/
@@ -45,30 +46,6 @@
#define HEADER_INUSE_MAGIC (0x746F6E59)
#define MAX_PARALLELS_IMAGE_FACTOR (1ull << 32)
-#define DEFAULT_CLUSTER_SIZE 1048576/* 1 MiB */
-
-
-// always little-endian
-typedef struct ParallelsHeader {
-char magic[16]; // "WithoutFreeSpace"
-uint32_t version;
-uint32_t heads;
-uint32_t cylinders;
-uint32_t tracks;
-uint32_t bat_entries;
-uint64_t nb_sectors;
-uint32_t inuse;
-uint32_t data_off;
-char padding[12];
-} QEMU_PACKED ParallelsHeader;
-
-
-typedef enum ParallelsPreallocMode {
-PRL_PREALLOC_MODE_FALLOCATE = 0,
-PRL_PREALLOC_MODE_TRUNCATE = 1,
-PRL_PREALLOC_MODE__MAX = 2,
-} ParallelsPreallocMode;
-
static QEnumLookup prealloc_mode_lookup = {
.array = (const char *const[]) {
"falloc",
@@ -77,34 +54,6 @@ static QEnumLookup prealloc_mode_lookup = {
.size = PRL_PREALLOC_MODE__MAX
};
-typedef struct BDRVParallelsState {
-/** Locking is conservative, the lock protects
- * - image file extending (truncate, fallocate)
- * - any access to block allocation table
- */
-CoMutex lock;
-
-ParallelsHeader *header;
-uint32_t header_size;
-bool header_unclean;
-
-unsigned long *bat_dirty_bmap;
-unsigned int bat_dirty_block;
-
-uint32_t *bat_bitmap;
-unsigned int bat_size;
-
-int64_t data_end;
-uint64_t prealloc_size;
-ParallelsPreallocMode prealloc_mode;
-
-unsigned int tracks;
-
-unsigned int off_multiplier;
-Error *migration_blocker;
-} BDRVParallelsState;
-
-
#define PARALLELS_OPT_PREALLOC_MODE "prealloc-mode"
#define PARALLELS_OPT_PREALLOC_SIZE "prealloc-size"
diff --git a/block/parallels.h b/block/parallels.h
new file mode 100644
index 00..71183c0c8e
--- /dev/null
+++ b/block/parallels.h
@@ -0,0 +1,86 @@
+/*
+* Block driver for Parallels disk image format
+*
+* Copyright (c) 2015-2017 Virtuozzo, Inc.
+* Authors:
+* 2016-2017 Klim S. Kireev
+* 2015 Denis V. Lunev
+*
+* This code was originally based on comparing different disk images created
+* by Parallels. Currently it is based on opened OpenVZ sources
+* available at
+* https://github.com/OpenVZ/ploop
+*
+* Permission is hereby granted, free of charge, to any person obtaining a copy
+* of this software and associated documentation files (the "Software"), to deal
+* in the Software without restriction, including without limitation the rights
+* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+* copies of the Software, and to permit persons to whom the Software is
+* furnished to do so, subject to the following conditions:
+*
+* The above copyright notice and this permission notice shall be included in
+* all copies or substantial portions of the Software.
+*
+* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+* THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+* THE SOFTWARE.
+*/
+#ifndef BLOCK_PARALLELS_H
+#define BLOCK_PARALLELS_H
+#include "qemu/coroutine.h"
+#include "qemu/typedefs.h"
+
+#define DEFAULT_CLUSTER_SIZE 1048576/* 1 MiB */
+
+/* always little-endian */
+typedef struct ParallelsHeader {
+char magic[16]; /* "WithoutFreeSpace" */
+uint32_t version;
+uint32_t heads;
+uint32_t cylinders;
+uint32_t tracks;
+uint32_t bat_entries;
+uint64_t nb_sectors;
+uint32_t inuse;
+uint32_t data_off;
+char padding[12];
+} QEMU_PACKED ParallelsHeader;
+
+typedef enum ParallelsPreallocMode {
+PRL_PREALLOC_MODE_FALLOCATE = 0,
+PRL_PREALLOC_MODE_TRUNCATE = 1,
+PRL_PREALLOC_MODE__MAX = 2,
+} ParallelsPreallocMode;
+
+typedef struct BDRVParallelsState {
+/** Locking is conservative, the lock pro
[Qemu-devel] [PATCH 1/5] docs/interop/prl-xml: description of Parallels Disk format
This patch adds main information about Parallels Disk format, which consists of DiskDescriptor.xml and other files. Signed-off-by: Edgar Kaziakhmedov Signed-off-by: Klim Kireev Signed-off-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Denis V. Lunev CC: Stefan Hajnoczi --- docs/interop/prl-xml.txt | 156 +++ 1 file changed, 156 insertions(+) create mode 100644 docs/interop/prl-xml.txt diff --git a/docs/interop/prl-xml.txt b/docs/interop/prl-xml.txt new file mode 100644 index 00..57bffc67ae --- /dev/null +++ b/docs/interop/prl-xml.txt @@ -0,0 +1,156 @@ += License = + +Copyright (c) 2015-2017, Virtuozzo, Inc. +Authors: +2015 Denis Lunev +2015 Vladimir Sementsov-Ogievskiy +2016-2017 Klim Kireev +2016-2017 Edgar Kaziakhmedov + +This work is licensed under the terms of the GNU GPL, version 2 or later. +See the COPYING file in the top-level directory. + +This specification contains minimal information about Parallels Disk Format, +which is enough to proper work with QEMU. Nevertheless, Parallels Cloud Server +and Parallels Desktop are able to add some unspecified nodes to xml and use +them, but they are for internal work and don't affect functionality. Also it +uses auxiliary xml "Snapshot.xml", which allows to store optional snapshot +information, but it doesn't influence open/read/write functionality. QEMU and +other software should not use fields not covered in this document and +Snapshot.xml file and must leave them as is. + += Parallels Disk Format = + +Parallels disk consists of two parts: the set of snapshots and the disk +descriptor file, which stores information about all files and snapshots. + +== Definitions == +Snapshot a record of the contents captured at a particular time, + capable of storing current state. A snapshot has UUID and + parent UUID. + + Snapshot imagean overlay representing the difference between this + snapshot and some earlier snapshot. + +Overlayan image storing the different sectors between two captured + states. + + Root image snapshot image with no parent, the root of snapshot tree. + +Storagethe backing storage for a subset of the virtual disk. When + there is more than one storage in a Parallels disk then that + is referred to as a split image. In this case every storage + covers specific address space area of the disk and has its + particular root image. Split images are not considered here + and are not supported. Each storage consists of disk + parameters and a list of images. The list of images always + contains a root image and may also contain overlays. The + root image can be an expandable Parallels image file or + plain. Overlays must be expandable. + + Description DiskDescriptor.xml stores information about disk parameters, + file snapshots, storages. + + Top The overlay between actual state and some previous snapshot. + SnapshotIt is not a snapshot in the classical sense because it + serves as the active image that the guest writes to. + +Sector a 512-byte data chunk. + +== Description file == +All information is placed in a single XML element Parallels_disk_image. +The element has only one attribute "Version", that must be 1.0. +Schema of DiskDescriptor.xml: + + + +... + + +... + + +... + + + +== Disk_Parameters element == +The Disk_Parameters element describes the physical layout of the virtual disk +and some general settings. + +The Disk_Parameters element MUST contain the following child elements: +* Disk_size - number of sectors in the disk, + desired size of the disk. +* Cylinders - number of the disk cylinders. +* Heads - number of the disk heads. +* Sectors - number of the disk sectors per cylinder + (sector size is 512 bytes) + Limitation: Product of the Heads, Sectors and Cylinders + values MUST be equal to the value of the Disk_size parameter. +* Padding - must be 0. Parallels Cloud Server and Parallels Desktop may + use padding set to 1, however this case is not covered + by this spec, QEMU and other software should not open + such disks and should not create them. + +== StorageData element == +This element of the file describes the root image and all snapshot images. + +The StorageData element consists of the Storage child element, as shown below: + + +... + + + +A Storage element has following child elements: +* Start - start sector of the storage
[Qemu-devel] [PATCH 5/5] block/parallels: add backing support to readv/writev
From: Edgar Kaziakhmedov
Since parallels format supports backing files, refine
readv/writev (allocate_clusters) to redirect read/write requests
to a backing file (if cluster is not available in the current bs).
Signed-off-by: Edgar Kaziakhmedov
Signed-off-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Denis V. Lunev
CC: Stefan Hajnoczi
---
block/parallels.c | 50 --
1 file changed, 44 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 7a8e8b05a9..d3802085e3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -142,6 +142,7 @@ static int64_t block_status(BDRVParallelsState *s, int64_t
sector_num,
static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, int *pnum)
{
+int ret;
BDRVParallelsState *s = bs->opaque;
int64_t pos, space, idx, to_allocate, i, len;
@@ -170,7 +171,6 @@ static int64_t allocate_clusters(BlockDriverState *bs,
int64_t sector_num,
return len;
}
if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) {
-int ret;
space += s->prealloc_size;
if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
ret = bdrv_pwrite_zeroes(bs->file,
@@ -186,6 +186,37 @@ static int64_t allocate_clusters(BlockDriverState *bs,
int64_t sector_num,
}
}
+/* Try to read from backing to fill empty clusters
+ * FIXME: 1. previous write_zeroes may be redundant
+ *2. most of data we read from backing will be rewritten by
+ * parallels_co_writev. On aligned-to-cluster write we do not
need
+ * this read at all.
+ *3. it would be good to combine write of data from backing and new
+ * data into one write call */
+if (bs->backing) {
+int64_t nb_cow_sectors = to_allocate * s->tracks;
+int64_t nb_cow_bytes = nb_cow_sectors << BDRV_SECTOR_BITS;
+QEMUIOVector qiov;
+struct iovec iov = {
+.iov_len = nb_cow_bytes,
+.iov_base = qemu_blockalign(bs, nb_cow_bytes)
+};
+qemu_iovec_init_external(&qiov, &iov, 1);
+
+ret = bdrv_co_readv(bs->backing, idx * s->tracks, nb_cow_sectors,
+&qiov);
+if (ret < 0) {
+qemu_vfree(iov.iov_base);
+return ret;
+}
+
+ret = bdrv_co_writev(bs->file, s->data_end, nb_cow_sectors, &qiov);
+qemu_vfree(iov.iov_base);
+if (ret < 0) {
+return ret;
+}
+}
+
for (i = 0; i < to_allocate; i++) {
s->bat_bitmap[idx + i] = cpu_to_le32(s->data_end / s->off_multiplier);
s->data_end += s->tracks;
@@ -309,12 +340,19 @@ static coroutine_fn int
parallels_co_readv(BlockDriverState *bs,
nbytes = n << BDRV_SECTOR_BITS;
+qemu_iovec_reset(&hd_qiov);
+qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
+
if (position < 0) {
-qemu_iovec_memset(qiov, bytes_done, 0, nbytes);
+if (bs->backing) {
+ret = bdrv_co_readv(bs->backing, sector_num, n, &hd_qiov);
+if (ret < 0) {
+break;
+}
+} else {
+qemu_iovec_memset(&hd_qiov, 0, 0, nbytes);
+}
} else {
-qemu_iovec_reset(&hd_qiov);
-qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
-
ret = bdrv_co_readv(bs->file, position, n, &hd_qiov);
if (ret < 0) {
break;
@@ -748,7 +786,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_flush_to_os = parallels_co_flush_to_os,
.bdrv_co_readv = parallels_co_readv,
.bdrv_co_writev = parallels_co_writev,
-
+.supports_backing = true,
.bdrv_create= parallels_create,
.bdrv_check = parallels_check,
.create_opts= ¶llels_create_opts,
--
2.14.3
[Qemu-devel] [PATCH 2/5] configure: add dependency
This dependency is required for adequate Parallels images support.
Typically the disk consists of several images which are glued by
XML disk descriptor. Also XML hides inside several important parameters
which are not available in the image header.
The patch also adds clause to checkpatch.pl to understand libxml2 types.
Signed-off-by: Denis V. Lunev
Signed-off-by: Klim Kireev
Signed-off-by: Edgar Kaziakhmedov
CC: Stefan Hajnoczi
---
block/Makefile.objs | 2 ++
configure | 27 +++
scripts/checkpatch.pl | 1 +
3 files changed, 30 insertions(+)
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 6eaf78a046..a73387f1bf 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -47,3 +47,5 @@ block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
dmg-bz2.o-libs := $(BZIP2_LIBS)
qcow.o-libs:= -lz
linux-aio.o-libs := -laio
+parallels.o-cflags := $(LIBXML2_CFLAGS)
+parallels.o-libs := $(LIBXML2_LIBS)
diff --git a/configure b/configure
index 6a040821c6..790fa635d1 100755
--- a/configure
+++ b/configure
@@ -435,6 +435,7 @@ tcmalloc="no"
jemalloc="no"
replication="yes"
vxhs=""
+libxml2=""
supported_cpu="no"
supported_os="no"
@@ -1298,6 +1299,10 @@ for opt do
;;
--enable-numa) numa="yes"
;;
+ --disable-libxml2) libxml2="no"
+ ;;
+ --enable-libxml2) libxml2="yes"
+ ;;
--disable-tcmalloc) tcmalloc="no"
;;
--enable-tcmalloc) tcmalloc="yes"
@@ -1573,6 +1578,7 @@ disabled with --disable-FEATURE, default is enabled if
available:
tpm TPM support
libssh2 ssh block device support
numalibnuma support
+ libxml2 for Parallels image format
tcmalloctcmalloc support
jemallocjemalloc support
replication replication support
@@ -3747,6 +3753,20 @@ EOF
fi
fi
+##
+# libxml2 probe
+if test "$libxml2" != "no" ; then
+if $pkg_config --exists libxml-2.0; then
+libxml2="yes"
+libxml2_cflags=$($pkg_config --cflags libxml-2.0)
+libxml2_libs=$($pkg_config --libs libxml-2.0)
+else
+if test "$libxml2" = "yes"; then
+feature_not_found "libxml2" "Install libxml2 devel"
+fi
+libxml2="no"
+fi
+fi
##
# glusterfs probe
@@ -5618,6 +5638,7 @@ echo "lzo support $lzo"
echo "snappy support$snappy"
echo "bzip2 support $bzip2"
echo "NUMA host support $numa"
+echo "libxml2 $libxml2"
echo "tcmalloc support $tcmalloc"
echo "jemalloc support $jemalloc"
echo "avx2 optimization $avx2_opt"
@@ -6281,6 +6302,12 @@ if test "$have_rtnetlink" = "yes" ; then
echo "CONFIG_RTNETLINK=y" >> $config_host_mak
fi
+if test "$libxml2" = "yes" ; then
+ echo "CONFIG_LIBXML2=y" >> $config_host_mak
+ echo "LIBXML2_CFLAGS=$libxml2_cflags" >> $config_host_mak
+ echo "LIBXML2_LIBS=$libxml2_libs" >> $config_host_mak
+fi
+
if test "$replication" = "yes" ; then
echo "CONFIG_REPLICATION=y" >> $config_host_mak
fi
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3dc27d9656..9bfe2b58e1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -265,6 +265,7 @@ our @typeList = (
qr{${Ident}_handler_fn},
qr{target_(?:u)?long},
qr{hwaddr},
+ qr{xml${Ident}},
);
# This can be modified by sub possible. Since it can be empty, be careful
--
2.14.3
[Qemu-devel] [PATCH 4/5] block/parallels: replace some magic numbers
Signed-off-by: Klim Kireev Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Denis V. Lunev CC: Stefan Hajnoczi --- block/parallels.c | 5 +++-- block/parallels.h | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/block/parallels.c b/block/parallels.c index f9a3b999ea..7a8e8b05a9 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -476,8 +476,9 @@ static int parallels_create(const char *filename, QemuOpts *opts, Error **errp) memcpy(header.magic, HEADER_MAGIC2, sizeof(header.magic)); header.version = cpu_to_le32(HEADER_VERSION); /* don't care much about geometry, it is not used on image level */ -header.heads = cpu_to_le32(16); -header.cylinders = cpu_to_le32(total_size / BDRV_SECTOR_SIZE / 16 / 32); +header.heads = cpu_to_le32(HEADS_NUMBER); +header.cylinders = cpu_to_le32(total_size / BDRV_SECTOR_SIZE + / HEADS_NUMBER / SEC_IN_CYL); header.tracks = cpu_to_le32(cl_size >> BDRV_SECTOR_BITS); header.bat_entries = cpu_to_le32(bat_entries); header.nb_sectors = cpu_to_le64(DIV_ROUND_UP(total_size, BDRV_SECTOR_SIZE)); diff --git a/block/parallels.h b/block/parallels.h index 71183c0c8e..4b044079ef 100644 --- a/block/parallels.h +++ b/block/parallels.h @@ -34,6 +34,8 @@ #include "qemu/coroutine.h" #include "qemu/typedefs.h" +#define HEADS_NUMBER 16 +#define SEC_IN_CYL 32 #define DEFAULT_CLUSTER_SIZE 1048576/* 1 MiB */ /* always little-endian */ -- 2.14.3
[Qemu-devel] [PATCH 3/5] block/parallels: move some structures into header
To implement xml format, some defines and structures
from parallels.c are required.
Signed-off-by: Klim Kireev
Reviewed-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Denis V. Lunev
CC: Stefan Hajnoczi
---
block/parallels.c | 53 +-
block/parallels.h | 86 +++
2 files changed, 87 insertions(+), 52 deletions(-)
create mode 100644 block/parallels.h
diff --git a/block/parallels.c b/block/parallels.c
index 9545761f49..f9a3b999ea 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -36,6 +36,7 @@
#include "qemu/bswap.h"
#include "qemu/bitmap.h"
#include "migration/blocker.h"
+#include "parallels.h"
/**/
@@ -45,30 +46,6 @@
#define HEADER_INUSE_MAGIC (0x746F6E59)
#define MAX_PARALLELS_IMAGE_FACTOR (1ull << 32)
-#define DEFAULT_CLUSTER_SIZE 1048576/* 1 MiB */
-
-
-// always little-endian
-typedef struct ParallelsHeader {
-char magic[16]; // "WithoutFreeSpace"
-uint32_t version;
-uint32_t heads;
-uint32_t cylinders;
-uint32_t tracks;
-uint32_t bat_entries;
-uint64_t nb_sectors;
-uint32_t inuse;
-uint32_t data_off;
-char padding[12];
-} QEMU_PACKED ParallelsHeader;
-
-
-typedef enum ParallelsPreallocMode {
-PRL_PREALLOC_MODE_FALLOCATE = 0,
-PRL_PREALLOC_MODE_TRUNCATE = 1,
-PRL_PREALLOC_MODE__MAX = 2,
-} ParallelsPreallocMode;
-
static QEnumLookup prealloc_mode_lookup = {
.array = (const char *const[]) {
"falloc",
@@ -77,34 +54,6 @@ static QEnumLookup prealloc_mode_lookup = {
.size = PRL_PREALLOC_MODE__MAX
};
-typedef struct BDRVParallelsState {
-/** Locking is conservative, the lock protects
- * - image file extending (truncate, fallocate)
- * - any access to block allocation table
- */
-CoMutex lock;
-
-ParallelsHeader *header;
-uint32_t header_size;
-bool header_unclean;
-
-unsigned long *bat_dirty_bmap;
-unsigned int bat_dirty_block;
-
-uint32_t *bat_bitmap;
-unsigned int bat_size;
-
-int64_t data_end;
-uint64_t prealloc_size;
-ParallelsPreallocMode prealloc_mode;
-
-unsigned int tracks;
-
-unsigned int off_multiplier;
-Error *migration_blocker;
-} BDRVParallelsState;
-
-
#define PARALLELS_OPT_PREALLOC_MODE "prealloc-mode"
#define PARALLELS_OPT_PREALLOC_SIZE "prealloc-size"
diff --git a/block/parallels.h b/block/parallels.h
new file mode 100644
index 00..71183c0c8e
--- /dev/null
+++ b/block/parallels.h
@@ -0,0 +1,86 @@
+/*
+* Block driver for Parallels disk image format
+*
+* Copyright (c) 2015-2017 Virtuozzo, Inc.
+* Authors:
+* 2016-2017 Klim S. Kireev
+* 2015 Denis V. Lunev
+*
+* This code was originally based on comparing different disk images created
+* by Parallels. Currently it is based on opened OpenVZ sources
+* available at
+* https://github.com/OpenVZ/ploop
+*
+* Permission is hereby granted, free of charge, to any person obtaining a copy
+* of this software and associated documentation files (the "Software"), to deal
+* in the Software without restriction, including without limitation the rights
+* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+* copies of the Software, and to permit persons to whom the Software is
+* furnished to do so, subject to the following conditions:
+*
+* The above copyright notice and this permission notice shall be included in
+* all copies or substantial portions of the Software.
+*
+* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+* THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+* THE SOFTWARE.
+*/
+#ifndef BLOCK_PARALLELS_H
+#define BLOCK_PARALLELS_H
+#include "qemu/coroutine.h"
+#include "qemu/typedefs.h"
+
+#define DEFAULT_CLUSTER_SIZE 1048576/* 1 MiB */
+
+/* always little-endian */
+typedef struct ParallelsHeader {
+char magic[16]; /* "WithoutFreeSpace" */
+uint32_t version;
+uint32_t heads;
+uint32_t cylinders;
+uint32_t tracks;
+uint32_t bat_entries;
+uint64_t nb_sectors;
+uint32_t inuse;
+uint32_t data_off;
+char padding[12];
+} QEMU_PACKED ParallelsHeader;
+
+typedef enum ParallelsPreallocMode {
+PRL_PREALLOC_MODE_FALLOCATE = 0,
+PRL_PREALLOC_MODE_TRUNCATE = 1,
+PRL_PREALLOC_MODE__MAX = 2,
+} ParallelsPreallocMode;
+
+typedef struct BDRVParallelsState {
+/** Locking is conservative, the lock pro
[Qemu-devel] [PATCH 1/5] docs/interop/prl-xml: description of Parallels Disk format
This patch adds main information about Parallels Disk format, which consists of DiskDescriptor.xml and other files. Signed-off-by: Edgar Kaziakhmedov Signed-off-by: Klim Kireev Signed-off-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Denis V. Lunev CC: Stefan Hajnoczi --- docs/interop/prl-xml.txt | 158 +++ 1 file changed, 158 insertions(+) create mode 100644 docs/interop/prl-xml.txt diff --git a/docs/interop/prl-xml.txt b/docs/interop/prl-xml.txt new file mode 100644 index 00..7031f8752c --- /dev/null +++ b/docs/interop/prl-xml.txt @@ -0,0 +1,158 @@ += License = + +Copyright (c) 2015-2017, Virtuozzo, Inc. +Authors: +2015 Denis Lunev +2015 Vladimir Sementsov-Ogievskiy +2016-2017 Klim Kireev +2016-2017 Edgar Kaziakhmedov + +This work is licensed under the terms of the GNU GPL, version 2 or later. +See the COPYING file in the top-level directory. + +This specification contains minimal information about Parallels Disk Format, +which is enough to proper work with QEMU. Nevertheless, Parallels Cloud Server +and Parallels Desktop are able to add some unspecified nodes to xml and use +them, but they are for internal work and don't affect functionality. Also it +uses auxiliary xml "Snapshot.xml", which allows to store optional snapshot +information, but it doesn't influence open/read/write functionality. QEMU and +other software should not use fields not covered in this document and +Snapshot.xml file and must leave them as is. + += Parallels Disk Format = + +Parallels disk consists of two parts: the set of snapshots and the disk +descriptor file, which stores information about all files and snapshots. + +== Definitions == +Snapshot a record of the contents captured at a particular time, + capable of storing current state. A snapshot has UUID and + parent UUID. + + Snapshot imagean overlay representing the difference between this + snapshot and some earlier snapshot. + +Overlayan image storing the different sectors between two captured + states. + + Root image snapshot image with no parent, the root of snapshot tree. + +Storagethe backing storage for a subset of the virtual disk. When + there is more than one storage in a Parallels disk then that + is referred to as a split image. In this case every storage + covers specific address space area of the disk and has its + particular root image. Split images are not considered here + and are not supported. Each storage consists of disk + parameters and a list of images. The list of images always + contains a root image and may also contain overlays. The + root image can be an expandable Parallels image file or + plain. Overlays must be expandable. + + Description DiskDescriptor.xml stores information about disk parameters, + file snapshots, storages. + + Top The overlay between actual state and some previous snapshot. + SnapshotIt is not a snapshot in the classical sense because it + serves as the active image that the guest writes to. + +Sector a 512-byte data chunk. + +== Description file == +All information is placed in a single XML element Parallels_disk_image. +The element has only one attribute "Version", that must be 1.0. +Schema of DiskDescriptor.xml: + + + +... + + +... + + +... + + + +== Disk_Parameters element == +The Disk_Parameters element describes the physical layout of the virtual disk +and some general settings. + +The Disk_Parameters element MUST contain the following child elements: +* Disk_size - number of sectors in the disk, + desired size of the disk. +* Cylinders - number of the disk cylinders. +* Heads - number of the disk heads. +* Sectors - number of the disk sectors per cylinder + (sector size is 512 bytes) + Limitation: Product of the Heads, Sectors and Cylinders + values MUST be equal to the value of the Disk_size parameter. +* Padding - must be 0. Parallels Cloud Server and Parallels Desktop may + use padding set to 1, however this case is not covered + by this spec, QEMU and other software should not open + such disks and should not create them. + +== StorageData element == +This element of the file describes the root image and all snapshot images. + +The StorageData element consists of the Storage child element, as shown below: + + +... + + + +A Storage element has following child elements: +* Start - start sector of the storage
[Qemu-devel] [PATCH 0/5 v3] preparation for Parallels Disk xml driver
Parallels Desktop and Parallels Cloud Server uses images glued with the bundle description in XML format. This series contains very basic description of this XML files and makes preparations for actual implementation to be followed. Signed-off-by: Edgar Kaziakhmedov Signed-off-by: Klim Kireev Signed-off-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Denis V. Lunev Changelog: v2: PATCH 1/5: Fix some places noticed by Stefan Hajnoczi PATCH 2/5: Rebase to upstream PATCH 3/5: Fix includes v3: PATCH 1/5: Fix the place about GUID, add emails of the authors
[Qemu-devel] [PATCH 5/5] block/parallels: add backing support to readv/writev
From: Edgar Kaziakhmedov
Since parallels format supports backing files, refine
readv/writev (allocate_clusters) to redirect read/write requests
to a backing file (if cluster is not available in the current bs).
Signed-off-by: Edgar Kaziakhmedov
Signed-off-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Denis V. Lunev
CC: Stefan Hajnoczi
---
block/parallels.c | 50 --
1 file changed, 44 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 7a8e8b05a9..d3802085e3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -142,6 +142,7 @@ static int64_t block_status(BDRVParallelsState *s, int64_t
sector_num,
static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, int *pnum)
{
+int ret;
BDRVParallelsState *s = bs->opaque;
int64_t pos, space, idx, to_allocate, i, len;
@@ -170,7 +171,6 @@ static int64_t allocate_clusters(BlockDriverState *bs,
int64_t sector_num,
return len;
}
if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) {
-int ret;
space += s->prealloc_size;
if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
ret = bdrv_pwrite_zeroes(bs->file,
@@ -186,6 +186,37 @@ static int64_t allocate_clusters(BlockDriverState *bs,
int64_t sector_num,
}
}
+/* Try to read from backing to fill empty clusters
+ * FIXME: 1. previous write_zeroes may be redundant
+ *2. most of data we read from backing will be rewritten by
+ * parallels_co_writev. On aligned-to-cluster write we do not
need
+ * this read at all.
+ *3. it would be good to combine write of data from backing and new
+ * data into one write call */
+if (bs->backing) {
+int64_t nb_cow_sectors = to_allocate * s->tracks;
+int64_t nb_cow_bytes = nb_cow_sectors << BDRV_SECTOR_BITS;
+QEMUIOVector qiov;
+struct iovec iov = {
+.iov_len = nb_cow_bytes,
+.iov_base = qemu_blockalign(bs, nb_cow_bytes)
+};
+qemu_iovec_init_external(&qiov, &iov, 1);
+
+ret = bdrv_co_readv(bs->backing, idx * s->tracks, nb_cow_sectors,
+&qiov);
+if (ret < 0) {
+qemu_vfree(iov.iov_base);
+return ret;
+}
+
+ret = bdrv_co_writev(bs->file, s->data_end, nb_cow_sectors, &qiov);
+qemu_vfree(iov.iov_base);
+if (ret < 0) {
+return ret;
+}
+}
+
for (i = 0; i < to_allocate; i++) {
s->bat_bitmap[idx + i] = cpu_to_le32(s->data_end / s->off_multiplier);
s->data_end += s->tracks;
@@ -309,12 +340,19 @@ static coroutine_fn int
parallels_co_readv(BlockDriverState *bs,
nbytes = n << BDRV_SECTOR_BITS;
+qemu_iovec_reset(&hd_qiov);
+qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
+
if (position < 0) {
-qemu_iovec_memset(qiov, bytes_done, 0, nbytes);
+if (bs->backing) {
+ret = bdrv_co_readv(bs->backing, sector_num, n, &hd_qiov);
+if (ret < 0) {
+break;
+}
+} else {
+qemu_iovec_memset(&hd_qiov, 0, 0, nbytes);
+}
} else {
-qemu_iovec_reset(&hd_qiov);
-qemu_iovec_concat(&hd_qiov, qiov, bytes_done, nbytes);
-
ret = bdrv_co_readv(bs->file, position, n, &hd_qiov);
if (ret < 0) {
break;
@@ -748,7 +786,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_flush_to_os = parallels_co_flush_to_os,
.bdrv_co_readv = parallels_co_readv,
.bdrv_co_writev = parallels_co_writev,
-
+.supports_backing = true,
.bdrv_create= parallels_create,
.bdrv_check = parallels_check,
.create_opts= ¶llels_create_opts,
--
2.14.3
[Qemu-devel] [PATCH 2/5] configure: add dependency
This dependency is required for adequate Parallels images support.
Typically the disk consists of several images which are glued by
XML disk descriptor. Also XML hides inside several important parameters
which are not available in the image header.
The patch also adds clause to checkpatch.pl to understand libxml2 types.
Signed-off-by: Denis V. Lunev
Signed-off-by: Klim Kireev
Signed-off-by: Edgar Kaziakhmedov
CC: Stefan Hajnoczi
---
block/Makefile.objs | 2 ++
configure | 27 +++
scripts/checkpatch.pl | 1 +
3 files changed, 30 insertions(+)
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 6eaf78a046..a73387f1bf 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -47,3 +47,5 @@ block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
dmg-bz2.o-libs := $(BZIP2_LIBS)
qcow.o-libs:= -lz
linux-aio.o-libs := -laio
+parallels.o-cflags := $(LIBXML2_CFLAGS)
+parallels.o-libs := $(LIBXML2_LIBS)
diff --git a/configure b/configure
index 6a040821c6..790fa635d1 100755
--- a/configure
+++ b/configure
@@ -435,6 +435,7 @@ tcmalloc="no"
jemalloc="no"
replication="yes"
vxhs=""
+libxml2=""
supported_cpu="no"
supported_os="no"
@@ -1298,6 +1299,10 @@ for opt do
;;
--enable-numa) numa="yes"
;;
+ --disable-libxml2) libxml2="no"
+ ;;
+ --enable-libxml2) libxml2="yes"
+ ;;
--disable-tcmalloc) tcmalloc="no"
;;
--enable-tcmalloc) tcmalloc="yes"
@@ -1573,6 +1578,7 @@ disabled with --disable-FEATURE, default is enabled if
available:
tpm TPM support
libssh2 ssh block device support
numalibnuma support
+ libxml2 for Parallels image format
tcmalloctcmalloc support
jemallocjemalloc support
replication replication support
@@ -3747,6 +3753,20 @@ EOF
fi
fi
+##
+# libxml2 probe
+if test "$libxml2" != "no" ; then
+if $pkg_config --exists libxml-2.0; then
+libxml2="yes"
+libxml2_cflags=$($pkg_config --cflags libxml-2.0)
+libxml2_libs=$($pkg_config --libs libxml-2.0)
+else
+if test "$libxml2" = "yes"; then
+feature_not_found "libxml2" "Install libxml2 devel"
+fi
+libxml2="no"
+fi
+fi
##
# glusterfs probe
@@ -5618,6 +5638,7 @@ echo "lzo support $lzo"
echo "snappy support$snappy"
echo "bzip2 support $bzip2"
echo "NUMA host support $numa"
+echo "libxml2 $libxml2"
echo "tcmalloc support $tcmalloc"
echo "jemalloc support $jemalloc"
echo "avx2 optimization $avx2_opt"
@@ -6281,6 +6302,12 @@ if test "$have_rtnetlink" = "yes" ; then
echo "CONFIG_RTNETLINK=y" >> $config_host_mak
fi
+if test "$libxml2" = "yes" ; then
+ echo "CONFIG_LIBXML2=y" >> $config_host_mak
+ echo "LIBXML2_CFLAGS=$libxml2_cflags" >> $config_host_mak
+ echo "LIBXML2_LIBS=$libxml2_libs" >> $config_host_mak
+fi
+
if test "$replication" = "yes" ; then
echo "CONFIG_REPLICATION=y" >> $config_host_mak
fi
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3dc27d9656..9bfe2b58e1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -265,6 +265,7 @@ our @typeList = (
qr{${Ident}_handler_fn},
qr{target_(?:u)?long},
qr{hwaddr},
+ qr{xml${Ident}},
);
# This can be modified by sub possible. Since it can be empty, be careful
--
2.14.3
[Qemu-devel] [PATCH 4/5] block/parallels: replace some magic numbers
Signed-off-by: Klim Kireev Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Denis V. Lunev CC: Stefan Hajnoczi --- block/parallels.c | 5 +++-- block/parallels.h | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/block/parallels.c b/block/parallels.c index f9a3b999ea..7a8e8b05a9 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -476,8 +476,9 @@ static int parallels_create(const char *filename, QemuOpts *opts, Error **errp) memcpy(header.magic, HEADER_MAGIC2, sizeof(header.magic)); header.version = cpu_to_le32(HEADER_VERSION); /* don't care much about geometry, it is not used on image level */ -header.heads = cpu_to_le32(16); -header.cylinders = cpu_to_le32(total_size / BDRV_SECTOR_SIZE / 16 / 32); +header.heads = cpu_to_le32(HEADS_NUMBER); +header.cylinders = cpu_to_le32(total_size / BDRV_SECTOR_SIZE + / HEADS_NUMBER / SEC_IN_CYL); header.tracks = cpu_to_le32(cl_size >> BDRV_SECTOR_BITS); header.bat_entries = cpu_to_le32(bat_entries); header.nb_sectors = cpu_to_le64(DIV_ROUND_UP(total_size, BDRV_SECTOR_SIZE)); diff --git a/block/parallels.h b/block/parallels.h index 71183c0c8e..4b044079ef 100644 --- a/block/parallels.h +++ b/block/parallels.h @@ -34,6 +34,8 @@ #include "qemu/coroutine.h" #include "qemu/typedefs.h" +#define HEADS_NUMBER 16 +#define SEC_IN_CYL 32 #define DEFAULT_CLUSTER_SIZE 1048576/* 1 MiB */ /* always little-endian */ -- 2.14.3
Re: [Qemu-devel] [PATCH v3] vnc: fix segfault in closed connection handling
ping
On 02/07/2018 12:48 PM, Klim Kireev wrote:
On one of our client's node, due to trying to read from closed ioc,
a segmentation fault occured. Corresponding backtrace:
0 object_get_class (obj=obj@entry=0x0)
1 qio_channel_readv_full (ioc=0x0, iov=0x7ffe55277180 ...
2 qio_channel_read (ioc= ...
3 vnc_client_read_buf (vs=vs@entry=0x55625f3c6000, ...
4 vnc_client_read_plain (vs=0x55625f3c6000)
5 vnc_client_read (vs=0x55625f3c6000)
6 vnc_client_io (ioc=, condition=G_IO_IN, ...
7 g_main_dispatch (context=0x556251568a50)
8 g_main_context_dispatch (context=context@entry=0x556251568a50)
9 glib_pollfds_poll ()
10 os_host_main_loop_wait (timeout=)
11 main_loop_wait (nonblocking=nonblocking@entry=0)
12 main_loop () at vl.c:1909
13 main (argc=, argv=, ...
Having analyzed the coredump, I understood that the reason is that
ioc_tag is reset on vnc_disconnect_start and ioc is cleaned
in vnc_disconnect_finish. Between these two events due to some
reasons the ioc_tag was set again and after vnc_disconnect_finish
the handler is running with freed ioc,
which led to the segmentation fault.
The patch checks vs->disconnecting in places where we call
qio_channel_add_watch and resets handler if disconnecting == TRUE
to prevent such an occurrence.
Signed-off-by: Klim Kireev
---
Changelog:
v2: Attach the backtrace
v3: Change checks
ui/vnc-jobs.c | 6 --
ui/vnc.c | 15 ++-
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index e326679dd0..868dddef4b 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -148,8 +148,10 @@ void vnc_jobs_consume_buffer(VncState *vs)
if (vs->ioc_tag) {
g_source_remove(vs->ioc_tag);
}
-vs->ioc_tag = qio_channel_add_watch(
-vs->ioc, G_IO_IN | G_IO_OUT, vnc_client_io, vs, NULL);
+if (vs->disconnecting == FALSE) {
+vs->ioc_tag = qio_channel_add_watch(
+vs->ioc, G_IO_IN | G_IO_OUT, vnc_client_io, vs, NULL);
+}
}
buffer_move(&vs->output, &vs->jobs_buffer);
diff --git a/ui/vnc.c b/ui/vnc.c
index 93731accb6..67ccc8160f 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1536,12 +1536,19 @@ gboolean vnc_client_io(QIOChannel *ioc G_GNUC_UNUSED,
VncState *vs = opaque;
if (condition & G_IO_IN) {
if (vnc_client_read(vs) < 0) {
-return TRUE;
+goto end;
}
}
if (condition & G_IO_OUT) {
vnc_client_write(vs);
}
+end:
+if (vs->disconnecting) {
+if (vs->ioc_tag != 0) {
+g_source_remove(vs->ioc_tag);
+}
+vs->ioc_tag = 0;
+}
return TRUE;
}
@@ -1630,6 +1637,12 @@ void vnc_flush(VncState *vs)
if (vs->ioc != NULL && vs->output.offset) {
vnc_client_write_locked(vs);
}
+if (vs->disconnecting) {
+if (vs->ioc_tag != 0) {
+g_source_remove(vs->ioc_tag);
+}
+vs->ioc_tag = 0;
+}
vnc_unlock_output(vs);
}
[Qemu-devel] [PATCH] vnc: fix segfault in closed connection handling
On one of our client's node, due to trying to read from closed ioc,
a segmentation fault occured. Corresponding backtrace:
0 object_get_class (obj=obj@entry=0x0)
1 qio_channel_readv_full (ioc=0x0, iov=0x7ffe55277180 ...
2 qio_channel_read (ioc= ...
3 vnc_client_read_buf (vs=vs@entry=0x55625f3c6000, ...
4 vnc_client_read_plain (vs=0x55625f3c6000)
5 vnc_client_read (vs=0x55625f3c6000)
6 vnc_client_io (ioc=, condition=G_IO_IN, ...
7 g_main_dispatch (context=0x556251568a50)
8 g_main_context_dispatch (context=context@entry=0x556251568a50)
9 glib_pollfds_poll ()
10 os_host_main_loop_wait (timeout=)
11 main_loop_wait (nonblocking=nonblocking@entry=0)
12 main_loop () at vl.c:1909
13 main (argc=, argv=, ...
Having analyzed the coredump, I understood that the reason is that
ioc_tag is reset on vnc_disconnect_start and ioc is cleaned
in vnc_disconnect_finish. Between these two events due to some
reasons the ioc_tag was set again and after vnc_disconnect_finish
the handler is running with freed ioc,
which led to the segmentation fault.
The patch checks vs->disconnecting in places where we call
qio_channel_add_watch and resets handler if disconnecting == TRUE
to prevent such an occurrence.
Signed-off-by: Klim Kireev
---
Changelog:
v2: Attach the backtrace
v3: Change checks
ui/vnc-jobs.c | 6 --
ui/vnc.c | 15 ++-
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index e326679dd0..868dddef4b 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -148,8 +148,10 @@ void vnc_jobs_consume_buffer(VncState *vs)
if (vs->ioc_tag) {
g_source_remove(vs->ioc_tag);
}
-vs->ioc_tag = qio_channel_add_watch(
-vs->ioc, G_IO_IN | G_IO_OUT, vnc_client_io, vs, NULL);
+if (vs->disconnecting == FALSE) {
+vs->ioc_tag = qio_channel_add_watch(
+vs->ioc, G_IO_IN | G_IO_OUT, vnc_client_io, vs, NULL);
+}
}
buffer_move(&vs->output, &vs->jobs_buffer);
diff --git a/ui/vnc.c b/ui/vnc.c
index 93731accb6..67ccc8160f 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1536,12 +1536,19 @@ gboolean vnc_client_io(QIOChannel *ioc G_GNUC_UNUSED,
VncState *vs = opaque;
if (condition & G_IO_IN) {
if (vnc_client_read(vs) < 0) {
-return TRUE;
+goto end;
}
}
if (condition & G_IO_OUT) {
vnc_client_write(vs);
}
+end:
+if (vs->disconnecting) {
+if (vs->ioc_tag != 0) {
+g_source_remove(vs->ioc_tag);
+}
+vs->ioc_tag = 0;
+}
return TRUE;
}
@@ -1630,6 +1637,12 @@ void vnc_flush(VncState *vs)
if (vs->ioc != NULL && vs->output.offset) {
vnc_client_write_locked(vs);
}
+if (vs->disconnecting) {
+if (vs->ioc_tag != 0) {
+g_source_remove(vs->ioc_tag);
+}
+vs->ioc_tag = 0;
+}
vnc_unlock_output(vs);
}
--
2.14.3
Re: [Qemu-devel] "make check -j4" hangs
On 02/08/2018 10:13 PM, Thomas Huth wrote:
> On 30.01.2018 20:49, Paolo Bonzini wrote:
>> On 25/01/2018 08:51, Klim Kireev wrote:
>>> The following behavior was observed for QEMU configured by libvirt
>>> to use guest agent as usual for the guests without virtio-serial
>>> driver (Windows or the guest remaining in BIOS stage).
>>>
>>> In QEMU on first connect to listen character device socket
>>> the listen socket is removed from poll just after the accept().
>>> virtio_serial_guest_ready() returns 0 and the descriptor
>>> of the connected Unix socket is removed from poll and it will
>>> not be present in poll() until the guest will initialize the driver
>>> and change the state of the serial to "guest connected".
>>>
>>> In libvirt connect() to guest agent is performed on restart and
>>> is run under VM state lock. Connect() is blocking and can
>>> wait forever.
>>> In this case libvirt can not perform ANY operation on that VM.
>>>
>>> The bug can be easily reproduced this way:
>>>
>>> Terminal 1:
>>> qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev
>>> socket,id=serial1,path=/tmp/console.sock,server,nowait
>>> (virtio-serial and isa-serial also fit)
>>>
>>> Terminal 2:
>>> minicom -D unix\#/tmp/console.sock
>>> (type something and press enter)
>>> C-a x (to exit)
>>>
>>> Do 3 times:
>>> minicom -D unix\#/tmp/console.sock
>>> C-a x
>>>
>>> It needs 4 connections, because the first one is accepted by QEMU, then two
>>> are queued by
>>> the kernel, and the 4th blocks.
>>>
>>> The problem is that QEMU doesn't add a read watcher after succesful read
>>> until the guest device wants to acquire recieved data, so
>>> I propose to install a separate pullhup watcher regardless of
>>> whether the device waits for data or not.
>>>
>>> Signed-off-by: Klim Kireev
>>> ---
>>> Changelog:
>>> v2: Remove timer as a redundant feature
>>>
>>> v3: Remove read call and return G_SOURCE_REMOVE
>>>
>>> v4: Move to GSource API
>>>
>>> v5: Fix git typos
>>>
>>> chardev/char-socket.c | 22 ++
>>> 1 file changed, 22 insertions(+)
>>>
>>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>>> index 77cdf487eb..a340af6cd3 100644
>>> --- a/chardev/char-socket.c
>>> +++ b/chardev/char-socket.c
>>> @@ -42,6 +42,7 @@ typedef struct {
>>> QIOChannel *ioc; /* Client I/O channel */
>>> QIOChannelSocket *sioc; /* Client master channel */
>>> QIONetListener *listener;
>>> +GSource *hup_source;
>>> QCryptoTLSCreds *tls_creds;
>>> int connected;
>>> int max_size;
>>> @@ -352,6 +353,12 @@ static void tcp_chr_free_connection(Chardev *chr)
>>> s->read_msgfds_num = 0;
>>> }
>>>
>>> +if (s->hup_source != NULL) {
>>> +g_source_destroy(s->hup_source);
>>> +g_source_unref(s->hup_source);
>>> +s->hup_source = NULL;
>>> +}
>>> +
>>> tcp_set_msgfds(chr, NULL, 0);
>>> remove_fd_in_watch(chr);
>>> object_unref(OBJECT(s->sioc));
>>> @@ -455,6 +462,15 @@ static gboolean tcp_chr_read(QIOChannel *chan,
>>> GIOCondition cond, void *opaque)
>>> return TRUE;
>>> }
>>>
>>> +static gboolean tcp_chr_hup(QIOChannel *channel,
>>> + GIOCondition cond,
>>> + void *opaque)
>>> +{
>>> +Chardev *chr = CHARDEV(opaque);
>>> +tcp_chr_disconnect(chr);
>>> +return G_SOURCE_REMOVE;
>>> +}
>>> +
>>> static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
>>> {
>>> SocketChardev *s = SOCKET_CHARDEV(chr);
>>> @@ -528,6 +544,12 @@ static void tcp_chr_connect(void *opaque)
>>> tcp_chr_read,
>>> chr, chr->gcontext);
>>> }
>>> +
>>> +s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
>>> +g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
>>> + chr, NULL);
>>> +g_source_attach(s->hup_source, chr->gcontext);
>>> +
>>> qemu_chr_be_event(chr, CHR_EVENT_OPENED);
>>> }
>>>
>>>
>> Queued, thanks.
> Hi!
>
> I'm currently facing some issues with "make check -j4" (i.e. running the
> tests in parallel). Git bisect blames this commit - though I'm not sure
> whether this is really the right one ... Starting with this commit, I
> saw hangs in test-filter-redirector,
Commit 8f6d701044bc87197aac8aa2a627d70ce8471726 fixes it
Have you tried to apply it?
> but git master rather seems to hang
> with vhost-user-test instead.
> Stefan also had issues with "make check -j4" today, so it's apparently
> not only me ... can somebody else reproduce the problem with the git
> master branch?
>
> Thomas
