[PATCH] hw/i386/pc: fix regression in parsing vga cmdline parameter
When the 'vga=' parameter is succeeded by another parameter, QEMU 4.2.0
would refuse to start with a rather cryptic message:
$ qemu-system-x86_64 -kernel /boot/vmlinuz-linux -append 'vga=792 quiet'
qemu: can't parse 'vga' parameter: Invalid argument
It was not clear whether this applied to the '-vga std' parameter or the
'-append' one. Fix the parsing regression and clarify the error.
Fixes: 133ef074bd ("hw/i386/pc: replace use of strtol with qemu_strtoui in
x86_load_linux()")
Cc: Sergio Lopez
Signed-off-by: Peter Wu
---
Hi,
This fixes a regression in QEMU 4.2.0 where my existing scripts would
fail to boot while it worked fine with QEMU 4.1.1.
I do wonder whether QEMU has any business in strictly enforcing the
contents of the kernel command line. Perhaps it should only warn about
the issue, and not exit? Previously it would silently ignore bad values.
Kind regards,
Peter
---
hw/i386/x86.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 394edc2f72..121650ae51 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -508,6 +508,7 @@ void x86_load_linux(X86MachineState *x86ms,
vmode = strstr(kernel_cmdline, "vga=");
if (vmode) {
unsigned int video_mode;
+const char *end;
int ret;
/* skip "vga=" */
vmode += 4;
@@ -518,10 +519,9 @@ void x86_load_linux(X86MachineState *x86ms,
} else if (!strncmp(vmode, "ask", 3)) {
video_mode = 0xfffd;
} else {
-ret = qemu_strtoui(vmode, NULL, 0, &video_mode);
-if (ret != 0) {
-fprintf(stderr, "qemu: can't parse 'vga' parameter: %s\n",
-strerror(-ret));
+ret = qemu_strtoui(vmode, &end, 0, &video_mode);
+if (ret != 0 || (*end && *end != ' ')) {
+fprintf(stderr, "qemu: invalid 'vga=' kernel parameter.\n");
exit(1);
}
}
--
2.24.1
[Qemu-devel] [PATCH] qxl: support mono cursors with inverted colors
Monochrome cursors are still used by Windows guests with the
QXL-WDDM-DOD driver. Such cursor types have one odd feature, inversion
of colors. GDK does not seem to support it, so implement an alternative
solution: fill the inverted pixels and add an outline to make the cursor
more visible. Tested with the text cursor in Notepad and Windows 10.
cursor_set_mono is also used by the vmware GPU, so add a special check
to avoid breaking its 32bpp format (tested with Kubuntu 14.04.4). I was
unable to find a guest which supports the 1bpp format with a vmware GPU.
The old implementation was buggy and removed in v2.10.0-108-g79c5a10cdd
("qxl: drop mono cursor support"), this version improves upon that by
adding bounds validation, clarifying the semantics of the two masks and
adds a workaround for inverted colors support.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1611984
Signed-off-by: Peter Wu
---
hw/display/qxl-render.c | 16
ui/cursor.c | 42 +++--
2 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
index c62b9a5e75..b82501ea9a 100644
--- a/hw/display/qxl-render.c
+++ b/hw/display/qxl-render.c
@@ -234,12 +234,28 @@ static QEMUCursor *qxl_cursor(PCIQXLDevice *qxl,
QXLCursor *cursor,
uint32_t group_id)
{
QEMUCursor *c;
+uint8_t *and_mask, *xor_mask;
size_t size;
c = cursor_alloc(cursor->header.width, cursor->header.height);
c->hot_x = cursor->header.hot_spot_x;
c->hot_y = cursor->header.hot_spot_y;
switch (cursor->header.type) {
+case SPICE_CURSOR_TYPE_MONO:
+/* Assume that the full cursor is available in a single chunk. */
+size = 2 * cursor_get_mono_bpl(c) * c->height;
+if (size != cursor->data_size) {
+fprintf(stderr, "%s: bad monochrome cursor %ux%u with size %u\n",
+__func__, c->width, c->height, cursor->data_size);
+goto fail;
+}
+and_mask = cursor->chunk.data;
+xor_mask = and_mask + cursor_get_mono_bpl(c) * c->height;
+cursor_set_mono(c, 0xff, 0x00, xor_mask, 1, and_mask);
+if (qxl->debug > 2) {
+cursor_print_ascii_art(c, "qxl/mono");
+}
+break;
case SPICE_CURSOR_TYPE_ALPHA:
size = sizeof(uint32_t) * cursor->header.width * cursor->header.height;
qxl_unpack_chunks(c->data, size, qxl, &cursor->chunk, group_id);
diff --git a/ui/cursor.c b/ui/cursor.c
index f3da0cee79..836bfb847f 100644
--- a/ui/cursor.c
+++ b/ui/cursor.c
@@ -128,13 +128,25 @@ void cursor_set_mono(QEMUCursor *c,
uint32_t *data = c->data;
uint8_t bit;
int x,y,bpl;
-
+bool expand_bitmap_only = image == mask;
+bool has_inverted_colors = false;
+const uint32_t inverted = 0x8000;
+
+/*
+ * Converts a monochrome bitmap with XOR mask 'image' and AND mask 'mask':
+ *
https://docs.microsoft.com/en-us/windows-hardware/drivers/display/drawing-monochrome-pointers
+ */
bpl = cursor_get_mono_bpl(c);
for (y = 0; y < c->height; y++) {
bit = 0x80;
for (x = 0; x < c->width; x++, data++) {
if (transparent && mask[x/8] & bit) {
-*data = 0x;
+if (!expand_bitmap_only && image[x/8] & bit) {
+*data = inverted;
+has_inverted_colors = true;
+} else {
+*data = 0x;
+}
} else if (!transparent && !(mask[x/8] & bit)) {
*data = 0x;
} else if (image[x/8] & bit) {
@@ -150,6 +162,32 @@ void cursor_set_mono(QEMUCursor *c,
mask += bpl;
image += bpl;
}
+
+/*
+ * If there are any pixels with inverted colors, create an outline (fill
+ * transparent neighbors with the background color) and use the foreground
+ * color as "inverted" color.
+ */
+if (has_inverted_colors) {
+data = c->data;
+for (y = 0; y < c->height; y++) {
+for (x = 0; x < c->width; x++, data++) {
+if (*data == 0 /* transparent */ &&
+((x > 0 && data[-1] == inverted) ||
+ (x + 1 < c->width && data[1] == inverted) ||
+ (y > 0 && data[-c->width] == inverted) ||
+ (y + 1 < c->height && data[c->width] == inverted))) {
+*data = 0xff00 | background;
+}
+}
+}
+data = c->data;
+for (x = 0; x < c->width * c->height; x++, data++) {
+if (*data == inverted) {
+*data = 0xff00 | foreground;
+}
+}
+}
}
void cursor_get_mono_image(QEMUCursor *c, int foreground, uint8_t *image)
--
2.18.0
[Qemu-devel] [PATCH] vnc: fix memleak of the "vnc-worker-output" name
Fixes repeated memory leaks of 18 bytes when using VNC:
Direct leak of 831024 byte(s) in 46168 object(s) allocated from:
...
#4 0x7f6d2f919bdd in g_strdup_vprintf glib/gstrfuncs.c:514
#5 0x56085cdcf660 in buffer_init util/buffer.c:59
#6 0x56085ca6a7ec in vnc_async_encoding_start ui/vnc-jobs.c:177
#7 0x56085ca6b815 in vnc_worker_thread_loop ui/vnc-jobs.c:240
Fixes: 543b95801f98 ("vnc: attach names to buffers")
Cc: Gerd Hoffmann
CC: qemu-sta...@nongnu.org
Signed-off-by: Peter Wu
---
ui/vnc-jobs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index b0b15d42a8..929391f85d 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -193,6 +193,7 @@ static void vnc_async_encoding_start(VncState *orig,
VncState *local)
static void vnc_async_encoding_end(VncState *orig, VncState *local)
{
+buffer_free(&local->output);
orig->tight = local->tight;
orig->zlib = local->zlib;
orig->hextile = local->hextile;
@@ -278,7 +279,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
/* Copy persistent encoding data */
vnc_async_encoding_end(job->vs, &vs);
- qemu_bh_schedule(job->vs->bh);
+qemu_bh_schedule(job->vs->bh);
} else {
buffer_reset(&vs.output);
/* Copy persistent encoding data */
--
2.18.0
Re: [Qemu-devel] [PATCH 2/2] gtk: disable the F10 menubar key
On Tue, May 15, 2018 at 10:46:54AM +0200, Gerd Hoffmann wrote: > On Tue, May 15, 2018 at 09:39:12AM +0100, Daniel P. Berrangé wrote: > > On Tue, May 15, 2018 at 10:30:09AM +0200, Gerd Hoffmann wrote: > > > On Fri, May 11, 2018 at 01:07:39AM +0200, Peter Wu wrote: > > > > The F10 key is used in various applications, disable it unconditionally > > > > (do not limit it to grab mode). Note that this property is deprecated > > > > and might be removed in the future (GTK+ commit b082fb598d). > > > > > > Any replacement provided by gtk? > > > > It doesn't look like it Would it still be possible to change the commit message? I asked Timm (the author of that commit) about it, and it appears that the feature was restored at some point again because gnome-terminal relied on it. The new message (if possible): The F10 key is used in various applications, disable it unconditionally (do not limit it to grab mode). This property will still work with GTK3, but as it is deprecated it might be removed in GTK4. -- Kind regards, Peter Wu https://lekensteyn.nl
[Qemu-devel] [PATCH 2/2] gtk: disable the F10 menubar key
The F10 key is used in various applications, disable it unconditionally
(do not limit it to grab mode). Note that this property is deprecated
and might be removed in the future (GTK+ commit b082fb598d).
Fixes: https://bugs.launchpad.net/qemu/+bug/1726910
Signed-off-by: Peter Wu
---
ui/gtk.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/ui/gtk.c b/ui/gtk.c
index 9e5390f2c2..6ad6cacfaa 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2321,6 +2321,8 @@ static GtkWidget *gd_create_menu_view(GtkDisplayState *s)
static void gd_create_menus(GtkDisplayState *s)
{
+GtkSettings *settings;
+
s->accel_group = gtk_accel_group_new();
s->machine_menu = gd_create_menu_machine(s);
s->view_menu = gd_create_menu_view(s);
@@ -2336,6 +2338,10 @@ static void gd_create_menus(GtkDisplayState *s)
g_object_set_data(G_OBJECT(s->window), "accel_group", s->accel_group);
gtk_window_add_accel_group(GTK_WINDOW(s->window), s->accel_group);
+
+/* Disable the default "F10" menu shortcut. */
+settings = gtk_widget_get_settings(s->window);
+g_object_set(G_OBJECT(settings), "gtk-menu-bar-accel", "", NULL);
}
--
2.17.0
[Qemu-devel] [PATCH 1/2] gtk: make it possible to hide the menu bar
Saves some space and disables the F10 button as side-effect.
Fixes: https://bugs.launchpad.net/qemu/+bug/1726910
Signed-off-by: Peter Wu
---
ui/gtk.c | 46 +-
1 file changed, 45 insertions(+), 1 deletion(-)
diff --git a/ui/gtk.c b/ui/gtk.c
index bb3214cffb..9e5390f2c2 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -145,6 +145,7 @@
#define GDK_KEY_2 GDK_2
#define GDK_KEY_f GDK_f
#define GDK_KEY_g GDK_g
+#define GDK_KEY_m GDK_m
#define GDK_KEY_q GDK_q
#define GDK_KEY_plus GDK_plus
#define GDK_KEY_equal GDK_equal
@@ -208,6 +209,7 @@ struct GtkDisplayState {
GtkWidget *show_tabs_item;
GtkWidget *untabify_item;
+GtkWidget *show_menubar_item;
GtkWidget *vbox;
GtkWidget *notebook;
@@ -1387,6 +1389,30 @@ static void gd_menu_untabify(GtkMenuItem *item, void
*opaque)
}
}
+static void gd_menu_show_menubar(GtkMenuItem *item, void *opaque)
+{
+GtkDisplayState *s = opaque;
+VirtualConsole *vc = gd_vc_find_current(s);
+
+if (s->full_screen) {
+return;
+}
+
+if (gtk_check_menu_item_get_active(
+GTK_CHECK_MENU_ITEM(s->show_menubar_item))) {
+gtk_widget_show(s->menu_bar);
+} else {
+gtk_widget_hide(s->menu_bar);
+}
+gd_update_windowsize(vc);
+}
+
+static void gd_accel_show_menubar(void *opaque)
+{
+GtkDisplayState *s = opaque;
+gtk_menu_item_activate(GTK_MENU_ITEM(s->show_menubar_item));
+}
+
static void gd_menu_full_screen(GtkMenuItem *item, void *opaque)
{
GtkDisplayState *s = opaque;
@@ -1403,7 +1429,10 @@ static void gd_menu_full_screen(GtkMenuItem *item, void
*opaque)
} else {
gtk_window_unfullscreen(GTK_WINDOW(s->window));
gd_menu_show_tabs(GTK_MENU_ITEM(s->show_tabs_item), s);
-gtk_widget_show(s->menu_bar);
+if (gtk_check_menu_item_get_active(
+GTK_CHECK_MENU_ITEM(s->show_menubar_item))) {
+gtk_widget_show(s->menu_bar);
+}
s->full_screen = FALSE;
if (vc->type == GD_VC_GFX) {
vc->gfx.scale_x = 1.0;
@@ -2036,6 +2065,8 @@ static void gd_connect_signals(GtkDisplayState *s)
G_CALLBACK(gd_menu_show_tabs), s);
g_signal_connect(s->untabify_item, "activate",
G_CALLBACK(gd_menu_untabify), s);
+g_signal_connect(s->show_menubar_item, "activate",
+ G_CALLBACK(gd_menu_show_menubar), s);
g_signal_connect(s->window, "delete-event",
G_CALLBACK(gd_window_close), s);
@@ -2272,6 +2303,19 @@ static GtkWidget *gd_create_menu_view(GtkDisplayState *s)
s->untabify_item = gtk_menu_item_new_with_mnemonic(_("Detach Tab"));
gtk_menu_shell_append(GTK_MENU_SHELL(view_menu), s->untabify_item);
+s->show_menubar_item = gtk_check_menu_item_new_with_mnemonic(
+_("Show Menubar"));
+gtk_check_menu_item_set_active(GTK_CHECK_MENU_ITEM(s->show_menubar_item),
+ TRUE);
+gtk_accel_group_connect(s->accel_group, GDK_KEY_m, HOTKEY_MODIFIERS, 0,
+g_cclosure_new_swap(G_CALLBACK(gd_accel_show_menubar), s, NULL));
+#if GTK_CHECK_VERSION(3, 8, 0)
+gtk_accel_label_set_accel(
+GTK_ACCEL_LABEL(gtk_bin_get_child(GTK_BIN(s->show_menubar_item))),
+GDK_KEY_m, HOTKEY_MODIFIERS);
+#endif
+gtk_menu_shell_append(GTK_MENU_SHELL(view_menu), s->show_menubar_item);
+
return view_menu;
}
--
2.17.0
[Qemu-devel] [Bug 1726910] Re: UI request: add a function key toolbar (f1-f12)
Current QEMU code (at least, v2.12.0-329-ge5cd695266) does not grab the
F10 key ("menubar activate"), accelerators (Ctrl-Alt-1) or menu bar
"mnemonic"s (such as Alt-V for _V_iew).
This comment nicely summarizes what kind of grabs is needed:
https://bugzilla.redhat.com/show_bug.cgi?id=499362#c1
This is the corresponding implementation:
https://github.com/SPICE/virt-viewer/blob/4048d28de853854a57835e91fb8758e78bc9ecd5/src/virt-viewer-window.c#L696
Changing the "gtk-enable-accels" setting on the window should still work
in order to disable accelerators such as Ctrl-Alt-G and Ctrl-Alt-1.
Being able to switch to the monitor (e.g. to issue "sendkey ..." is
important, so something else should probably catch this. Or else just
accept that the grab key has to be pressed first.
Alt-M (Menu) and Alt-V (View) are not that commonly used shortcuts,
these could be removed/disabled with a grab if desired, but otherwise I
am not annoyed by it.
The F10 key however is used for various purposes in applications and must be
passed to the guest.
Unfortunately, Timm Bäder removed the "gtk-menu-bar-accel" property in GTK+
3.22.1-295-gb082fb598d (Oct 2016) which means that the F10 shortcut can no
longer be disabled using said property. So now another hack is needed to
prevent "gtk_window_activate_key" (gtk/gtkwindow.c) from triggering this menu
bar.
** Bug watch added: Red Hat Bugzilla #499362
https://bugzilla.redhat.com/show_bug.cgi?id=499362
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1726910
Title:
UI request: add a function key toolbar (f1-f12)
Status in QEMU:
New
Bug description:
I run old DOS programs under FreeDOS using QEMU.
It's common when running/testing DOS applications to use the function
keys. Some of these (such as F10) are intercepted by the window
system. For example, some DOS program installers use F10 to install
the software, but F10 is intecepted by the window system.
The standard solution is to jump to the QEMU console and use sendkey
to send a specific function key to the QEMU guest (often needed for
'sendkey f10'). But this does not seem to be very user friendly for
new users. Nor is it very fast if you need to this often.
I propose QEMU add a toolbar with the function keys.
I've attached a mockup of one possible design, with a simple toolbar
for F1-F12. A possible modification would be to add "modifier" buttons
for Ctrl and Shift and Alt to make it easier for users to enter
combinations like Ctrl-F12 or Alt-F10 or Shift-F1.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1726910/+subscriptions
Re: [Qemu-devel] DMG chunk size independence
On Mon, Apr 24, 2017 at 05:19:48PM -0400, John Snow wrote: > > > On 04/23/2017 05:03 AM, Ashijeet Acharya wrote: > > Hi, > > > > Great news! > > I have almost completed this task and the results are looking > > promising. I have not yet attended to the DMG files having bz2 > > compressed chunks but that should be easy and pretty similar to my > > approach for zlib compressed files. So, no worries there. > > > > For testing I am first converting the images to raw format and then > > comparing the resulting image with the one converted using v2.9.0 DMG > > driver and after battling for 2 days with my code, it finally prints > > "Images are identical." According to John, that should be pretty > > conclusive and I completely agree. > > > > Yes, comparing a sample.dmg against a raw file generated from the 2.9.0 > qemu-img tool should be reasonably good evidence that you have not > altered the behavior of the tool. > > > Now, the real thing I wanted to ask was, if someone is aware of a DMG > > file which has a chunk size above 64 MiB so that I can test those too. > > If yes, please share the download link with me. > > Currently I am testing the ones posted by Peter Wu while submitting > > his DMG work in 2014. > > Here -> > > https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg03606.html > > > > Are any of those over 64MB? I assume you're implying that they aren't. > > Maybe Peter knows?... I don't know DMG with bzip2-compressed chunks over 64M. Looking through more recent files, there is this log for "Install macOS Sierra 10.12(16A323)-B.dmg" which contains only zlib-compressed or raw data where the uncompressed size (in the MISH block) is always at most 1MiB: https://github.com/Lekensteyn/dmg2img/issues/1#issuecomment-273662984 In an Xcode_7.2.dmg file, the situation was similar, only zlib or raw and also with a max uncompressed size of 1MiB (actually, an exact size of 1MiB in both cases based on "sectorCount"). Perhaps bzip2-compressed chunks are not so common for larger disk images since zlib is faster. -- Kind regards, Peter Wu https://lekensteyn.nl
[Qemu-devel] [PATCH v2] gdbstub: avoid busy loop while waiting for gdb
While waiting for a gdb response, or while sending an acknowledgement
there is not much to do, so do not mark the socket as non-blocking to
avoid a busy loop while paused at gdb. This only affects the user-mode
emulation (qemu-arm -g 1234 ./a.out).
Note that this issue was reported before at
https://lists.nongnu.org/archive/html/qemu-devel/2013-02/msg02277.html.
While at it, close the gdb client fd on EOF or error while reading.
Signed-off-by: Peter Wu
Reviewed-by: Peter Maydell
---
v2: resent with fixed commit message + R-b from
https://patchwork.kernel.org/patch/9008751/
---
gdbstub.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 0e431fd..b126bf5 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -332,7 +332,7 @@ static int get_char(GDBState *s)
if (ret < 0) {
if (errno == ECONNRESET)
s->fd = -1;
-if (errno != EINTR && errno != EAGAIN)
+if (errno != EINTR)
return -1;
} else if (ret == 0) {
close(s->fd);
@@ -393,7 +393,7 @@ static void put_buffer(GDBState *s, const uint8_t *buf, int
len)
while (len > 0) {
ret = send(s->fd, buf, len, 0);
if (ret < 0) {
-if (errno != EINTR && errno != EAGAIN)
+if (errno != EINTR)
return;
} else {
buf += ret;
@@ -1542,9 +1542,13 @@ gdb_handlesig(CPUState *cpu, int sig)
for (i = 0; i < n; i++) {
gdb_read_byte(s, buf[i]);
}
-} else if (n == 0 || errno != EAGAIN) {
+} else {
/* XXX: Connection closed. Should probably wait for another
connection before continuing. */
+if (n == 0) {
+close(s->fd);
+}
+s->fd = -1;
return sig;
}
}
@@ -1599,8 +1603,6 @@ static void gdb_accept(void)
gdb_has_xml = false;
gdbserver_state = s;
-
-fcntl(fd, F_SETFL, O_NONBLOCK);
}
static int gdbserver_open(int port)
--
2.8.3
Re: [Qemu-devel] [PATCH] gdbstub: avoid busy loop while waiting for gdb
On Thu, May 05, 2016 at 04:37:40PM +0100, Peter Maydell wrote: > On 3 May 2016 at 23:58, Peter Wu wrote: > > While waiting for a gdb response, or while sending an acknowledgement > > there is not much to do, so just mark the socket as non-blocking to > > avoid a busy loop while paused at gdb. This only affects the user-mode > > emulation (qemu-arm -g 1234 ./a.out). > > > > Note that this issue was reported before at > > https://lists.nongnu.org/archive/html/qemu-devel/2013-02/msg02277.html. > > > > While at it, close the gdb client fd on EOF or error while reading. > > The commit message says "mark the socket as non-blocking"... > > > @@ -1599,8 +1603,6 @@ static void gdb_accept(void) > > gdb_has_xml = false; > > > > gdbserver_state = s; > > - > > -fcntl(fd, F_SETFL, O_NONBLOCK); > > } > > ...but the code change is *removing* a call to mark the > socket as non-blocking. Which is correct? > > thanks > -- PMM The commit message is misleading, it should have been "so do not mark the socket as non-blocking". If you were to apply this, please fix it up :-) -- Kind regards, Peter Wu https://lekensteyn.nl
[Qemu-devel] [PATCH] gdbstub: set listen backlog to 1
Avoid possible connection drops on Linux (when tcp_syncookies is
disabled) or fallbacks to SYN cookies with the following kernel warning:
TCP: request_sock_TCP: Possible SYN flooding on port 1234. Sending cookies.
Check SNMP counters.
Since Linux 4.4 (ef547f2ac16b "tcp: remove max_qlen_log"), a backlog of
zero is really treated as the "queue length for completely established
sockets waiting to be accepted" (listen(2)). This is apparently a valid
interpretation of an "implementation-defined minimum value" for a
backlog value of 0 (listen(3p)). Previous kernels would use 8 as
minimum value, but that is no longer the case.
Signed-off-by: Peter Wu
---
Hi,
This was also reported at Linux, but the author of that patch argued that it is
intended behavior[1].
Kind regards,
Peter
[1]:
https://lkml.kernel.org/r/1462321544.5535.337.ca...@edumazet-glaptop3.roam.corp.google.com
---
gdbstub.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gdbstub.c b/gdbstub.c
index b126bf5..f181dc3 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1630,7 +1630,7 @@ static int gdbserver_open(int port)
close(fd);
return -1;
}
-ret = listen(fd, 0);
+ret = listen(fd, 1);
if (ret < 0) {
perror("listen");
close(fd);
--
2.8.0
[Qemu-devel] [PATCH] gdbstub: avoid busy loop while waiting for gdb
While waiting for a gdb response, or while sending an acknowledgement
there is not much to do, so just mark the socket as non-blocking to
avoid a busy loop while paused at gdb. This only affects the user-mode
emulation (qemu-arm -g 1234 ./a.out).
Note that this issue was reported before at
https://lists.nongnu.org/archive/html/qemu-devel/2013-02/msg02277.html.
While at it, close the gdb client fd on EOF or error while reading.
Signed-off-by: Peter Wu
---
gdbstub.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 0e431fd..b126bf5 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -332,7 +332,7 @@ static int get_char(GDBState *s)
if (ret < 0) {
if (errno == ECONNRESET)
s->fd = -1;
-if (errno != EINTR && errno != EAGAIN)
+if (errno != EINTR)
return -1;
} else if (ret == 0) {
close(s->fd);
@@ -393,7 +393,7 @@ static void put_buffer(GDBState *s, const uint8_t *buf, int
len)
while (len > 0) {
ret = send(s->fd, buf, len, 0);
if (ret < 0) {
-if (errno != EINTR && errno != EAGAIN)
+if (errno != EINTR)
return;
} else {
buf += ret;
@@ -1542,9 +1542,13 @@ gdb_handlesig(CPUState *cpu, int sig)
for (i = 0; i < n; i++) {
gdb_read_byte(s, buf[i]);
}
-} else if (n == 0 || errno != EAGAIN) {
+} else {
/* XXX: Connection closed. Should probably wait for another
connection before continuing. */
+if (n == 0) {
+close(s->fd);
+}
+s->fd = -1;
return sig;
}
}
@@ -1599,8 +1603,6 @@ static void gdb_accept(void)
gdb_has_xml = false;
gdbserver_state = s;
-
-fcntl(fd, F_SETFL, O_NONBLOCK);
}
static int gdbserver_open(int port)
--
2.8.0
Re: [Qemu-devel] [PATCH] block/dmg: make it modular if using additional library
On Tue, Mar 10, 2015 at 10:06:04AM +0300, Michael Tokarev wrote: > block/dmg can use additional library (libbz2) to read > bzip2-compressed files. Make the block driver to be > a module if libbz2 support is requested, to avoid extra > library dependency by default. > > Signed-off-by: Michael Tokarev Tested-by: Peter Wu Reviewed-by: Peter Wu > -- > > This might be questionable, to make the thing to be either > module or built-in depending on build environment, so a > better idea may be to make it modular unconditionally. > This block device format isn't used often. I do not mind making it a module unconditionally, that would make it easier to disable should a security bug come around the corner. Kind regards, Peter > block/Makefile.objs | 3 ++- > configure | 3 +++ > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/block/Makefile.objs b/block/Makefile.objs > index db2933e..440c51f 100644 > --- a/block/Makefile.objs > +++ b/block/Makefile.objs > @@ -1,7 +1,8 @@ > -block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o > vvfat.o > +block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o > block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o > qcow2-cache.o > block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o > block-obj-y += qed-check.o > +block-obj-$(CONFIG_DMG) += dmg.o > block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o > block-obj-$(CONFIG_QUORUM) += quorum.o > block-obj-y += parallels.o blkdebug.o blkverify.o > diff --git a/configure b/configure > index 7ba4bcb..1dd5721 100755 > --- a/configure > +++ b/configure > @@ -4772,6 +4772,9 @@ fi > if test "$bzip2" = "yes" ; then >echo "CONFIG_BZIP2=y" >> $config_host_mak >echo "BZIP2_LIBS=-lbz2" >> $config_host_mak > + echo "CONFIG_DMG=m" >> $config_host_mak > +else > + echo "CONFIG_DMG=y" >> $config_host_mak > fi > > if test "$libiscsi" = "yes" ; then > -- > 2.1.4 >
Re: [Qemu-devel] [PULL 00/16] Block patches
On Friday 16 January 2015 16:46:39 Peter Maydell wrote:
> CentOS5:
>
> ../block/dmg.o: In function `dmg_read_plist_xml':
> /home/petmay01/linaro/qemu-for-merges/block/dmg.c:414: undefined
> reference to `g_base64_decode_inplace'
Should have paid more attention to the API docs. Can you try the
following patch? It still passes 4 dmg tests for me
(https://lekensteyn.nl/files/dmg-tests/).
--
Kind regards,
Peter
https://lekensteyn.nl
--
>From 462454e820d2fa5f8eefe7b039d6ea32e4a88d41 Mon Sep 17 00:00:00 2001
From: Peter Wu
Date: Sat, 17 Jan 2015 11:34:32 +0100
Subject: [PATCH] block/dmg: fix compatibility with glib 2.12
For compatibility with glib 2.12, use g_base64_decode (which
additionally requires an extra buffer allocation) instead of
g_base64_decode_inplace (which is only available since glib 2.20).
Signed-off-by: Peter Wu
---
block/dmg.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 4e24076..0430f55 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -403,6 +403,7 @@ static int dmg_read_plist_xml(BlockDriverState *bs,
DmgHeaderState *ds,
* and line feeds. */
data_end = (char *)buffer;
while ((data_begin = strstr(data_end, "")) != NULL) {
+guchar *mish;
gsize out_len = 0;
data_begin += 6;
@@ -413,9 +414,9 @@ static int dmg_read_plist_xml(BlockDriverState *bs,
DmgHeaderState *ds,
goto fail;
}
*data_end++ = '\0';
-g_base64_decode_inplace(data_begin, &out_len);
-ret = dmg_read_mish_block(s, ds, (uint8_t *)data_begin,
- (uint32_t)out_len);
+mish = g_base64_decode(data_begin, &out_len);
+ret = dmg_read_mish_block(s, ds, mish, (uint32_t)out_len);
+g_free(mish);
if (ret < 0) {
goto fail;
}
--
2.2.2
Re: [Qemu-devel] [PATCH 09/10] block/dmg: support bzip2 block entry types
On Wednesday 07 January 2015 11:29:20 Paolo Bonzini wrote:
>
> On 05/01/2015 20:32, John Snow wrote:
> >>
> >> fi
> >>
> >> ##
> >> +# bzip2 check
> >> +
> >> +if test "$bzip2" != "no" ; then
> >> +cat > $TMPC << EOF
> >> +#include
> >> +int main(void) { BZ2_bzlibVersion(); return 0; }
> >> +EOF
> >> +if compile_prog "" "-lbz2" ; then
> >> +libs_softmmu="$libs_softmmu -lbz2"
> >> +bzip2="yes"
> >> +else
> >> +if test "$bzip2" = "yes"; then
> >> +feature_not_found "libbzip2" "Install libbzip2 devel"
> >> +fi
> >> +bzip2="no"
> >> +fi
> >> +fi
> >> +
> >> +##
> >> # libseccomp check
> >>
> >> if test "$seccomp" != "no" ; then
> >> @@ -4340,6 +4366,7 @@ echo "vhdx $vhdx"
> >> echo "Quorum$quorum"
> >> echo "lzo support $lzo"
> >> echo "snappy support$snappy"
> >> +echo "bzip2 support $bzip2"
> >> echo "NUMA host support $numa"
> >>
> >> if test "$sdl_too_old" = "yes"; then
> >> @@ -4695,6 +4722,10 @@ if test "$snappy" = "yes" ; then
> >> echo "CONFIG_SNAPPY=y" >> $config_host_mak
> >> fi
> >>
> >> +if test "$bzip2" = "yes" ; then
> >> + echo "CONFIG_BZIP2=y" >> $config_host_mak
> >> +fi
> >> +
> >> if test "$libiscsi" = "yes" ; then
> >> echo "CONFIG_LIBISCSI=m" >> $config_host_mak
> >> echo "LIBISCSI_CFLAGS=$libiscsi_cflags" >> $config_host_mak
> >>
> >
> > Looks good otherwise. CCing Paolo so he can take a quick peek at the
> > configure script. It looks sane to me, though.
>
> It is sane, but instead of libs_softmmu="$libs_softmmu -lbz2" it's
> better to follow what we do for libiscsi. In configure:
>
> CONFIG_BZIP2=y
> LIBBZ2_CFLAGS=-lbz2
>
> In Makefile.objs:
>
> dmg.o-libs := $(LIBBZ2_CFLAGS)
>
> Paolo
Already taken care of in v2 which was posted to the list yesterday :-)
I named it "BZIP2_LIBS" though (matching the package name).
--
Kind regards,
Peter
https://lekensteyn.nl
Re: [Qemu-devel] [PATCH v2 01/12] block/dmg: properly detect the UDIF trailer
On Wednesday 07 January 2015 13:19:34 Stefan Hajnoczi wrote:
> On Tue, Jan 06, 2015 at 06:48:04PM +0100, Peter Wu wrote:
> > DMG files have a variable length with a UDIF trailer at the end of a
> > file. This UDIF trailer is essential as it describes the contents of
> > the image. At the moment however, the start of this trailer is almost
> > always incorrect as bdrv_getlength() returns a multiple of the block
> > size (rounded up). This results in a failure to recognize DMG files,
> > resulting in Invalid argument (EINVAL) errors.
> >
> > As there is no API to retrieve the real file size, look for the magic
> > header in the last two sectors to find the start of this 512-byte UDIF
> > trailer (the "koly" block).
> >
> > The resource fork offset ("info_begin") has its offset adjusted as the
> > initial value of offset does not mean "end of file" anymore, but "begin
> > of UDIF trailer".
> >
> > Signed-off-by: Peter Wu
> > Reviewed-by: John Snow
> > ---
> > v2: added R-b, set errp as suggested by Stefan Hajnoczi
> > ---
> > block/dmg.c | 49 +
> > 1 file changed, 45 insertions(+), 4 deletions(-)
> >
> > diff --git a/block/dmg.c b/block/dmg.c
> > index e455886..9183459 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -131,6 +131,48 @@ static void update_max_chunk_size(BDRVDMGState *s,
> > uint32_t chunk,
> > }
> > }
> >
> > +static int64_t dmg_find_koly_offset(BlockDriverState *file_bs, Error
> > **errp)
> > +{
> > +int64_t length;
> > +int64_t offset = 0;
> > +uint8_t buffer[515];
> > +int i, ret;
> > +
> > +/* bdrv_getlength returns a multiple of block size (512), rounded up.
> > Since
> > + * dmg images can have odd sizes, try to look for the "koly" magic
> > which
> > + * marks the begin of the UDIF trailer (512 bytes). This magic can be
> > found
> > + * in the last 511 bytes of the second-last sector or the first 4
> > bytes of
> > + * the last sector (search space: 515 bytes) */
> > +length = bdrv_getlength(file_bs);
> > +if (length < 0) {
> > +error_setg_errno(errp, -length,
> > +"Failed to get file size while reading UDIF trailer");
> > +return length;
> > +} else if (length < 512) {
> > +error_set(errp, ERROR_CLASS_GENERIC_ERROR,
> > +"dmg file must be at least 512 bytes long");
> > +return -EINVAL;
>
> Not worth respinning but error_setg() is a shortcut for error_set(errp,
> ERROR_CLASS_GENERIC_ERROR, fmt, ...).
>
> Reviewed-by: Stefan Hajnoczi
Good to know, I got a compile error when ERROR_CLASS_GENERIC_ERROR was
omitted, didn't think of using error_setg instead.
When merging, please replace the above ERROR_CLASS_GENERIC_ERROR by:
error_setg(errp, "dmg file must be at least 512 bytes long");
(and likewise for the last error_set in the function)
--
Kind regards,
Peter
https://lekensteyn.nl
[Qemu-devel] [PATCH v2 11/12] block/dmg: support bzip2 block entry types
This patch adds support for bzip2-compressed block entries as introduced
with OS X 10.4 (source: https://en.wikipedia.org/wiki/Apple_Disk_Image).
It was tested against a 5.2G "OS X Yosemite" installation image which
stores the BLXX block in the XML property list (instead of resource
forks) and has over 5k chunks.
New configure entries are added (--enable-bzip2 / --disable-bzip2) to
control inclusion of bzip2 functionality (which requires linking against
libbz2). The help message suggests that this option is needed for DMG
files, but the tests are generic enough that other parts of QEMU can use
bzip2 if needed.
The identifiers are based on http://newosxbook.com/DMG.html.
The decompression routines are based on the zlib case, but as there is
no way to reset the decompression state (unlike zlib), memory is
allocated and deallocated for every decompression. This should not be
problematic as the decompression takes most of the time and as blocks
are typically about/over 1 MiB in size, only one allocation is done
every 2000 sectors.
Signed-off-by: Peter Wu
---
v2: split block type check into a different patch ("[PATCH v2 10/12]
block/dmg: factor out block type check").
Add BZIP2_LIBS instead of polluting libs_softmmu with -lbz2 (which
would also end up in fsdev/virtfs-proxy-helper).
Fix unused variable warning with --disable-bzip2.
---
block/Makefile.objs | 1 +
block/dmg.c | 43 ++-
configure | 31 +++
3 files changed, 74 insertions(+), 1 deletion(-)
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 04b0e43..e7ea07c 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -36,5 +36,6 @@ gluster.o-libs := $(GLUSTERFS_LIBS)
ssh.o-cflags := $(LIBSSH2_CFLAGS)
ssh.o-libs := $(LIBSSH2_LIBS)
archipelago.o-libs := $(ARCHIPELAGO_LIBS)
+dmg.o-libs := $(BZIP2_LIBS)
qcow.o-libs:= -lz
linux-aio.o-libs := -laio
diff --git a/block/dmg.c b/block/dmg.c
index b1d0930..8239221 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -26,6 +26,9 @@
#include "qemu/bswap.h"
#include "qemu/module.h"
#include
+#ifdef CONFIG_BZIP2
+#include
+#endif
#include
enum {
@@ -56,6 +59,9 @@ typedef struct BDRVDMGState {
uint8_t *compressed_chunk;
uint8_t *uncompressed_chunk;
z_stream zstream;
+#ifdef CONFIG_BZIP2
+bz_stream bzstream;
+#endif
} BDRVDMGState;
static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
@@ -123,6 +129,7 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t
chunk,
switch (s->types[chunk]) {
case 0x8005: /* zlib compressed */
+case 0x8006: /* bzip2 compressed */
compressed_size = s->lengths[chunk];
uncompressed_sectors = s->sectorcounts[chunk];
break;
@@ -200,6 +207,9 @@ static bool dmg_is_known_block_type(uint32_t entry_type)
case 0x0001:/* uncompressed */
case 0x0002:/* zeroes */
case 0x8005:/* zlib */
+#ifdef CONFIG_BZIP2
+case 0x8006:/* bzip2 */
+#endif
return true;
default:
return false;
@@ -565,13 +575,16 @@ static inline int dmg_read_chunk(BlockDriverState *bs,
uint64_t sector_num)
if (!is_sector_in_chunk(s, s->current_chunk, sector_num)) {
int ret;
uint32_t chunk = search_chunk(s, sector_num);
+#ifdef CONFIG_BZIP2
+uint64_t total_out;
+#endif
if (chunk >= s->n_chunks) {
return -1;
}
s->current_chunk = s->n_chunks;
-switch (s->types[chunk]) {
+switch (s->types[chunk]) { /* block entry type */
case 0x8005: { /* zlib compressed */
/* we need to buffer, because only the chunk as whole can be
* inflated. */
@@ -595,6 +608,34 @@ static inline int dmg_read_chunk(BlockDriverState *bs,
uint64_t sector_num)
return -1;
}
break; }
+#ifdef CONFIG_BZIP2
+case 0x8006: /* bzip2 compressed */
+/* we need to buffer, because only the chunk as whole can be
+ * inflated. */
+ret = bdrv_pread(bs->file, s->offsets[chunk],
+ s->compressed_chunk, s->lengths[chunk]);
+if (ret != s->lengths[chunk]) {
+return -1;
+}
+
+ret = BZ2_bzDecompressInit(&s->bzstream, 0, 0);
+if (ret != BZ_OK) {
+return -1;
+}
+s->bzstream.next_in = (char *)s->compressed_chunk;
+s->bzstream.avail_in = (unsigned int) s->lengths[chunk];
+s->bzstream.next_out = (char *)s->uncompressed_chunk;
+s->bzstream.avail_out = (unsigned int) 512 *
s->sectorcounts[chunk];
+ret = BZ2_bzDecompress(&s->bzstream);
[Qemu-devel] [PATCH v2 02/12] block/dmg: extract mish block decoding functionality
Extract the mish block decoder such that this can be used for other
formats in the future. A new DmgHeaderState struct is introduced to
share state while decoding.
The code is kept unchanged as much as possible, a "fail" label is added
for example where a simple return would probably do. In dmg_open, the
variable "tmp" is renamed to "rsrc_data_offset" for clarity and comments
have been added explaining various data.
Note that this patch has one subtle difference with the previous
version which should not affect functionality. In the previous code,
the end of a resource was inferred from the mish block (the offsets
would be increased by the fields). In this patch, the resource length
is used instead to avoid the need to rely on the previous offsets.
Signed-off-by: Peter Wu
Reviewed-by: John Snow
Reviewed-by: Stefan Hajnoczi
---
v2: added R-b, added comments, renamed "tmp" var to "rsrc_data_offset".
--
block/dmg.c | 228 +++-
1 file changed, 133 insertions(+), 95 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 9183459..e01559f 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -173,19 +173,138 @@ static int64_t dmg_find_koly_offset(BlockDriverState
*file_bs, Error **errp)
return -EINVAL;
}
+/* used when building the sector table */
+typedef struct DmgHeaderState {
+/* used internally by dmg_read_mish_block to remember offsets of blocks
+ * across calls */
+uint64_t last_in_offset;
+uint64_t last_out_offset;
+/* exported for dmg_open */
+uint32_t max_compressed_size;
+uint32_t max_sectors_per_chunk;
+} DmgHeaderState;
+
+static int dmg_read_mish_block(BlockDriverState *bs, DmgHeaderState *ds,
+ int64_t offset, uint32_t count)
+{
+BDRVDMGState *s = bs->opaque;
+uint32_t type, i;
+int ret;
+size_t new_size;
+uint32_t chunk_count;
+
+ret = read_uint32(bs, offset, &type);
+if (ret < 0) {
+goto fail;
+}
+
+/* skip data that is not a valid MISH block (invalid magic or too small) */
+if (type != 0x6d697368 || count < 244) {
+/* assume success for now */
+return 0;
+}
+
+offset += 4;
+offset += 200;
+
+chunk_count = (count - 204) / 40;
+new_size = sizeof(uint64_t) * (s->n_chunks + chunk_count);
+s->types = g_realloc(s->types, new_size / 2);
+s->offsets = g_realloc(s->offsets, new_size);
+s->lengths = g_realloc(s->lengths, new_size);
+s->sectors = g_realloc(s->sectors, new_size);
+s->sectorcounts = g_realloc(s->sectorcounts, new_size);
+
+for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) {
+ret = read_uint32(bs, offset, &s->types[i]);
+if (ret < 0) {
+goto fail;
+}
+offset += 4;
+if (s->types[i] != 0x8005 && s->types[i] != 1 &&
+s->types[i] != 2) {
+if (s->types[i] == 0x && i > 0) {
+ds->last_in_offset = s->offsets[i - 1] + s->lengths[i - 1];
+ds->last_out_offset = s->sectors[i - 1] +
+ s->sectorcounts[i - 1];
+}
+chunk_count--;
+i--;
+offset += 36;
+continue;
+}
+offset += 4;
+
+ret = read_uint64(bs, offset, &s->sectors[i]);
+if (ret < 0) {
+goto fail;
+}
+s->sectors[i] += ds->last_out_offset;
+offset += 8;
+
+ret = read_uint64(bs, offset, &s->sectorcounts[i]);
+if (ret < 0) {
+goto fail;
+}
+offset += 8;
+
+if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
+error_report("sector count %" PRIu64 " for chunk %" PRIu32
+ " is larger than max (%u)",
+ s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
+ret = -EINVAL;
+goto fail;
+}
+
+ret = read_uint64(bs, offset, &s->offsets[i]);
+if (ret < 0) {
+goto fail;
+}
+s->offsets[i] += ds->last_in_offset;
+offset += 8;
+
+ret = read_uint64(bs, offset, &s->lengths[i]);
+if (ret < 0) {
+goto fail;
+}
+offset += 8;
+
+if (s->lengths[i] > DMG_LENGTHS_MAX) {
+error_report("length %" PRIu64 " for chunk %" PRIu32
+ " is larger than max (%u)",
+ s->lengths[i], i, DMG_LENGTHS_MAX);
+ret = -EINVAL;
+goto fail;
+}
+
+update_max_chunk_size(s, i, &ds->max_compressed_size,
+ &ds->max_
[Qemu-devel] [PATCH v2 12/12] block/dmg: improve zeroes handling
Disk images may contain large all-zeroes gaps (1.66k sectors or 812 MiB
is seen in the real world). These blocks (type 2) do not need to be
extracted into a temporary buffer, there is no need to allocate memory
for these blocks nor to check its length.
(For the test image, the maximum uncompressed size is 1054371 bytes,
probably for a bzip2-compressed block.)
Signed-off-by: Peter Wu
---
v2: no changes (did not receive comments last time)
---
block/dmg.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 8239221..4e24076 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -137,7 +137,9 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t
chunk,
uncompressed_sectors = (s->lengths[chunk] + 511) / 512;
break;
case 2: /* zero */
-uncompressed_sectors = s->sectorcounts[chunk];
+/* as the all-zeroes block may be large, it is treated specially: the
+ * sector is not copied from a large buffer, a simple memset is used
+ * instead. Therefore uncompressed_sectors does not need to be set. */
break;
}
@@ -269,7 +271,9 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
/* sector count */
s->sectorcounts[i] = buff_read_uint64(buffer, offset + 0x10);
-if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
+/* all-zeroes sector (type 2) does not need to be "uncompressed" and
can
+ * therefore be unbounded. */
+if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
error_report("sector count %" PRIu64 " for chunk %" PRIu32
" is larger than max (%u)",
s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
@@ -644,7 +648,8 @@ static inline int dmg_read_chunk(BlockDriverState *bs,
uint64_t sector_num)
}
break;
case 2: /* zero */
-memset(s->uncompressed_chunk, 0, 512 * s->sectorcounts[chunk]);
+/* see dmg_read, it is treated specially. No buffer needs to be
+ * pre-filled, the zeroes can be set directly. */
break;
}
s->current_chunk = chunk;
@@ -663,6 +668,13 @@ static int dmg_read(BlockDriverState *bs, int64_t
sector_num,
if (dmg_read_chunk(bs, sector_num + i) != 0) {
return -1;
}
+/* Special case: current chunk is all zeroes. Do not perform a memcpy
as
+ * s->uncompressed_chunk may be too small to cover the large all-zeroes
+ * section. dmg_read_chunk is called to find s->current_chunk */
+if (s->types[s->current_chunk] == 2) { /* all zeroes block entry */
+memset(buf + i * 512, 0, 512);
+continue;
+}
sector_offset_in_chunk = sector_num + i - s->sectors[s->current_chunk];
memcpy(buf + i * 512,
s->uncompressed_chunk + sector_offset_in_chunk * 512, 512);
--
2.2.1
[Qemu-devel] [PATCH v2 10/12] block/dmg: factor out block type check
In preparation for adding bzip2 support, split the type check into a
separate function. Make all offsets relative to the begin of a chunk
such that it is easier to recognize the position without having to
add up all offsets. Some comments are added to describe the fields.
There is no functional change.
Signed-off-by: Peter Wu
---
v2: new patch, split off bzip2 patch. Besides the
dmg_is_known_block_type function, the offsets are now also made
relative.
---
block/dmg.c | 36 +++-
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 57922c5..b1d0930 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -194,6 +194,18 @@ typedef struct DmgHeaderState {
uint32_t max_sectors_per_chunk;
} DmgHeaderState;
+static bool dmg_is_known_block_type(uint32_t entry_type)
+{
+switch (entry_type) {
+case 0x0001:/* uncompressed */
+case 0x0002:/* zeroes */
+case 0x8005:/* zlib */
+return true;
+default:
+return false;
+}
+}
+
static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
uint8_t *buffer, uint32_t count)
{
@@ -233,22 +245,19 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) {
s->types[i] = buff_read_uint32(buffer, offset);
-offset += 4;
-if (s->types[i] != 0x8005 && s->types[i] != 1 &&
-s->types[i] != 2) {
+if (!dmg_is_known_block_type(s->types[i])) {
chunk_count--;
i--;
-offset += 36;
+offset += 40;
continue;
}
-offset += 4;
-s->sectors[i] = buff_read_uint64(buffer, offset);
+/* sector number */
+s->sectors[i] = buff_read_uint64(buffer, offset + 8);
s->sectors[i] += out_offset;
-offset += 8;
-s->sectorcounts[i] = buff_read_uint64(buffer, offset);
-offset += 8;
+/* sector count */
+s->sectorcounts[i] = buff_read_uint64(buffer, offset + 0x10);
if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
error_report("sector count %" PRIu64 " for chunk %" PRIu32
@@ -258,12 +267,12 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
goto fail;
}
-s->offsets[i] = buff_read_uint64(buffer, offset);
+/* offset in (compressed) data fork */
+s->offsets[i] = buff_read_uint64(buffer, offset + 0x18);
s->offsets[i] += in_offset;
-offset += 8;
-s->lengths[i] = buff_read_uint64(buffer, offset);
-offset += 8;
+/* length in (compressed) data fork */
+s->lengths[i] = buff_read_uint64(buffer, offset + 0x20);
if (s->lengths[i] > DMG_LENGTHS_MAX) {
error_report("length %" PRIu64 " for chunk %" PRIu32
@@ -275,6 +284,7 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
update_max_chunk_size(s, i, &ds->max_compressed_size,
&ds->max_sectors_per_chunk);
+offset += 40;
}
s->n_chunks += chunk_count;
return 0;
--
2.2.1
[Qemu-devel] [PATCH v2 03/12] block/dmg: extract processing of resource forks
Besides the offset, also read the resource length. This length is now
used in the extracted function to verify the end of the resource fork
against "count" from the resource fork.
Instead of relying on the value of offset to conclude whether the
resource fork is available or not (info_begin==0), check the
rsrc_fork_length instead. This would allow a dmg file to begin with a
resource fork. This seemingly unnecessary restriction was found while
trying to craft a DMG file by hand.
Other changes:
- Do not require resource data offset to be 0x100 (but check that it
is within bounds though).
- Further improve boundary checking (resource data must be within
the resource fork).
- Use correct value for resource data length (spotted by John Snow)
- Consider the resource data offset when determining info_end.
This fixes an EINVAL on the tuxpaint dmg example.
The resource fork format is documented at
https://developer.apple.com/legacy/library/documentation/mac/pdf/MoreMacintoshToolbox.pdf#page=151
Signed-off-by: Peter Wu
---
v2: expanded commit message (incl. documentation link). Do not require
a fixed resource data offset of 0x100, improve boundary checking,
fix resource data len (8 vs 4), append offset when checking the end
of the resource data.
--
block/dmg.c | 104 ++--
1 file changed, 66 insertions(+), 38 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index e01559f..ed99cf5 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -287,60 +287,38 @@ fail:
return ret;
}
-static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
-Error **errp)
+static int dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
+ uint64_t info_begin, uint64_t info_length)
{
-BDRVDMGState *s = bs->opaque;
-DmgHeaderState ds;
-uint64_t info_begin, info_end;
-uint32_t count, rsrc_data_offset;
-int64_t offset;
int ret;
+uint32_t count, rsrc_data_offset;
+uint64_t info_end;
+uint64_t offset;
-bs->read_only = 1;
-s->n_chunks = 0;
-s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
-/* used by dmg_read_mish_block to keep track of the current I/O position */
-ds.last_in_offset = 0;
-ds.last_out_offset = 0;
-ds.max_compressed_size = 1;
-ds.max_sectors_per_chunk = 1;
-
-/* locate the UDIF trailer */
-offset = dmg_find_koly_offset(bs->file, errp);
-if (offset < 0) {
-ret = offset;
-goto fail;
-}
-
-ret = read_uint64(bs, offset + 0x28, &info_begin);
-if (ret < 0) {
-goto fail;
-} else if (info_begin == 0) {
-ret = -EINVAL;
-goto fail;
-}
-
+/* read offset from begin of resource fork (info_begin) to resource data */
ret = read_uint32(bs, info_begin, &rsrc_data_offset);
if (ret < 0) {
goto fail;
-} else if (rsrc_data_offset != 0x100) {
+} else if (rsrc_data_offset > info_length) {
ret = -EINVAL;
goto fail;
}
-ret = read_uint32(bs, info_begin + 4, &count);
+/* read length of resource data */
+ret = read_uint32(bs, info_begin + 8, &count);
if (ret < 0) {
goto fail;
-} else if (count == 0) {
+} else if (count == 0 || rsrc_data_offset + count > info_length) {
ret = -EINVAL;
goto fail;
}
-/* end of resource data, ignoring the following resource map */
-info_end = info_begin + count;
/* begin of resource data (consisting of one or more resources) */
-offset = info_begin + 0x100;
+offset = info_begin + rsrc_data_offset;
+
+/* end of resource data (there is possibly a following resource map
+ * which will be ignored). */
+info_end = offset + count;
/* read offsets (mish blocks) from one or more resources in resource data
*/
while (offset < info_end) {
@@ -354,13 +332,63 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
}
offset += 4;
-ret = dmg_read_mish_block(bs, &ds, offset, count);
+ret = dmg_read_mish_block(bs, ds, offset, count);
if (ret < 0) {
goto fail;
}
/* advance offset by size of resource */
offset += count;
}
+return 0;
+
+fail:
+return ret;
+}
+
+static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
+Error **errp)
+{
+BDRVDMGState *s = bs->opaque;
+DmgHeaderState ds;
+uint64_t rsrc_fork_offset, rsrc_fork_length;
+int64_t offset;
+int ret;
+
+bs->read_only = 1;
+s->n_chunks = 0;
+s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
+/* used by dmg_read_mish_block to keep track of the current I/O position */
+ds.last_in_offset = 0;
+ds.last_out_offset = 0;
+
[Qemu-devel] [PATCH v2 06/12] block/dmg: process XML plists
The format is simple enough to avoid using a full-blown XML parser. It
assumes that all BLKX items begin with the "mish" magic word, therefore
it is not a problem if other values get matched which are not a BLKX
block.
The offsets are based on the description at
http://newosxbook.com/DMG.html
Signed-off-by: Peter Wu
---
v2: added offset check, allow resource fork to be located at beginning
of file (removed `rsrc_fork_offset != 0` condition)
It got a `Reviewed-by: John Snow ` for v1. I have not
copied the R-b as I wanted to be sure that John agrees with the removal
of the offset != 0 condition.
---
block/dmg.c | 74 +
1 file changed, 74 insertions(+)
diff --git a/block/dmg.c b/block/dmg.c
index 5f6976b..1a0fa0e 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -26,6 +26,7 @@
#include "qemu/bswap.h"
#include "qemu/module.h"
#include
+#include
enum {
/* Limit chunk sizes to prevent unreasonable amounts of memory being used
@@ -345,12 +346,66 @@ fail:
return ret;
}
+static int dmg_read_plist_xml(BlockDriverState *bs, DmgHeaderState *ds,
+ uint64_t info_begin, uint64_t info_length)
+{
+BDRVDMGState *s = bs->opaque;
+int ret;
+uint8_t *buffer = NULL;
+char *data_begin, *data_end;
+
+/* Have at least some length to avoid NULL for g_malloc. Attempt to set a
+ * safe upper cap on the data length. A test sample had a XML length of
+ * about 1 MiB. */
+if (info_length == 0 || info_length > 16 * 1024 * 1024) {
+ret = -EINVAL;
+goto fail;
+}
+
+buffer = g_malloc(info_length + 1);
+buffer[info_length] = '\0';
+ret = bdrv_pread(bs->file, info_begin, buffer, info_length);
+if (ret != info_length) {
+ret = -EINVAL;
+goto fail;
+}
+
+/* look for The data is 284 (0x11c) bytes after base64
+ * decode. The actual data element has 431 (0x1af) bytes which includes
tabs
+ * and line feeds. */
+data_end = (char *)buffer;
+while ((data_begin = strstr(data_end, "")) != NULL) {
+gsize out_len = 0;
+
+data_begin += 6;
+data_end = strstr(data_begin, "");
+/* malformed XML? */
+if (data_end == NULL) {
+ret = -EINVAL;
+goto fail;
+}
+*data_end++ = '\0';
+g_base64_decode_inplace(data_begin, &out_len);
+ret = dmg_read_mish_block(s, ds, (uint8_t *)data_begin,
+ (uint32_t)out_len);
+if (ret < 0) {
+goto fail;
+}
+}
+ret = 0;
+
+fail:
+g_free(buffer);
+return ret;
+}
+
static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
BDRVDMGState *s = bs->opaque;
DmgHeaderState ds;
uint64_t rsrc_fork_offset, rsrc_fork_length;
+uint64_t plist_xml_offset, plist_xml_length;
int64_t offset;
int ret;
@@ -384,12 +439,31 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
ret = -EINVAL;
goto fail;
}
+/* offset of property list (XMLOffset) */
+ret = read_uint64(bs, offset + 0xd8, &plist_xml_offset);
+if (ret < 0) {
+goto fail;
+}
+ret = read_uint64(bs, offset + 0xe0, &plist_xml_length);
+if (ret < 0) {
+goto fail;
+}
+if (plist_xml_offset >= offset ||
+plist_xml_length > offset - plist_xml_offset) {
+ret = -EINVAL;
+goto fail;
+}
if (rsrc_fork_length != 0) {
ret = dmg_read_resource_fork(bs, &ds,
rsrc_fork_offset, rsrc_fork_length);
if (ret < 0) {
goto fail;
}
+} else if (plist_xml_length != 0) {
+ret = dmg_read_plist_xml(bs, &ds, plist_xml_offset, plist_xml_length);
+if (ret < 0) {
+goto fail;
+}
} else {
ret = -EINVAL;
goto fail;
--
2.2.1
[Qemu-devel] [PATCH v2 07/12] block/dmg: set virtual size to a non-zero value
Right now the virtual size is always reported as zero which makes it
impossible to convert between formats.
After this patch, the number of sectors will be read from the trailer
("koly" block).
To verify the behavior, the output of `dmg2img foo.dmg foo.img` was
compared against `qemu-img convert -f dmg -O raw foo.dmg foo.raw`. The
tests showed that the file contents are exactly the same, except that
QEMU creates a slightly larger file (it matches the total sectors
count).
Signed-off-by: Peter Wu
---
v2: fixed typo in commit message (s/mish/koly/)
---
block/dmg.c | 8
1 file changed, 8 insertions(+)
diff --git a/block/dmg.c b/block/dmg.c
index 1a0fa0e..57feb1b 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -453,6 +453,14 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
ret = -EINVAL;
goto fail;
}
+ret = read_uint64(bs, offset + 0x1ec, (uint64_t *)&bs->total_sectors);
+if (ret < 0) {
+goto fail;
+}
+if (bs->total_sectors < 0) {
+ret = -EINVAL;
+goto fail;
+}
if (rsrc_fork_length != 0) {
ret = dmg_read_resource_fork(bs, &ds,
rsrc_fork_offset, rsrc_fork_length);
--
2.2.1
[Qemu-devel] [PATCH v2 08/12] block/dmg: fix sector data offset calculation
This patch addresses two issues:
- The data fork offset was not taken into account, resulting in failure
to read an InstallESD.dmg file (5164763151 bytes) which had a
non-zero DataForkOffset field.
- The offset of the previous block ("partition") was unconditionally
added to the current block because older files would start the input
offset of a new block at zero. Newer files (including vlc-2.1.5.dmg,
tuxpaint-0.9.15-macosx.dmg and OS X Yosemite [MAS].dmg) failed in
reads because these files have chunk offsets, relative to the begin
of a data fork.
Now the data offset of the mish is taken into account. While we could
check that the data_offset is within the data fork, let's not do that
here as it would only result in parse failures on invalid files (rather
than gracefully handling such bad files). dmg_read will error out if
the offset is incorrect.
Signed-off-by: Peter Wu
---
v2: use sector and data offset as provided by the BLKX header. This
allows us to drop last_in_offset. The previous heuristics to detect
relative offsets is not needed anymore. Squashed the data fork
offset length check into this patch.
---
block/dmg.c | 26 --
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 57feb1b..130efac 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -188,7 +188,7 @@ static int64_t dmg_find_koly_offset(BlockDriverState
*file_bs, Error **errp)
typedef struct DmgHeaderState {
/* used internally by dmg_read_mish_block to remember offsets of blocks
* across calls */
-uint64_t last_in_offset;
+uint64_t data_fork_offset;
uint64_t last_out_offset;
/* exported for dmg_open */
uint32_t max_compressed_size;
@@ -203,6 +203,8 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
size_t new_size;
uint32_t chunk_count;
int64_t offset = 0;
+uint64_t data_offset;
+uint64_t in_offset = ds->data_fork_offset;
type = buff_read_uint32(buffer, offset);
/* skip data that is not a valid MISH block (invalid magic or too small) */
@@ -211,8 +213,12 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
return 0;
}
-offset += 4;
-offset += 200;
+/* location in data fork for (compressed) blob (in bytes) */
+data_offset = buff_read_uint64(buffer, offset + 0x18);
+in_offset += data_offset;
+
+/* move to begin of chunk entries */
+offset += 204;
chunk_count = (count - 204) / 40;
new_size = sizeof(uint64_t) * (s->n_chunks + chunk_count);
@@ -228,7 +234,6 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
if (s->types[i] != 0x8005 && s->types[i] != 1 &&
s->types[i] != 2) {
if (s->types[i] == 0x && i > 0) {
-ds->last_in_offset = s->offsets[i - 1] + s->lengths[i - 1];
ds->last_out_offset = s->sectors[i - 1] +
s->sectorcounts[i - 1];
}
@@ -255,7 +260,7 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
}
s->offsets[i] = buff_read_uint64(buffer, offset);
-s->offsets[i] += ds->last_in_offset;
+s->offsets[i] += in_offset;
offset += 8;
s->lengths[i] = buff_read_uint64(buffer, offset);
@@ -413,7 +418,7 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
s->n_chunks = 0;
s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
/* used by dmg_read_mish_block to keep track of the current I/O position */
-ds.last_in_offset = 0;
+ds.data_fork_offset = 0;
ds.last_out_offset = 0;
ds.max_compressed_size = 1;
ds.max_sectors_per_chunk = 1;
@@ -425,6 +430,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
goto fail;
}
+/* offset of data fork (DataForkOffset) */
+ret = read_uint64(bs, offset + 0x18, &ds.data_fork_offset);
+if (ret < 0) {
+goto fail;
+} else if (ds.data_fork_offset > offset) {
+ret = -EINVAL;
+goto fail;
+}
+
/* offset of resource fork (RsrcForkOffset) */
ret = read_uint64(bs, offset + 0x28, &rsrc_fork_offset);
if (ret < 0) {
--
2.2.1
[Qemu-devel] [PATCH v2 09/12] block/dmg: use SectorNumber from BLKX header
Previously the sector table parsing relied on the previous offset of
the DMG file. Now it uses the sector number from the BLKX header
(see http://newosxbook.com/DMG.html).
The implementation of dmg2img (from vu1tur) does not base the output
sector on the location of the terminator (0x) either so it
should be safe to drop this dependency on the previous state.
(It makes somehow makes sense, a terminator should halt further
processing of a block and is perhaps used to preallocate some space.)
Signed-off-by: Peter Wu
---
v2: initial patch after suggestions from John Snow to read and use
these fields.
---
block/dmg.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 130efac..57922c5 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -189,7 +189,6 @@ typedef struct DmgHeaderState {
/* used internally by dmg_read_mish_block to remember offsets of blocks
* across calls */
uint64_t data_fork_offset;
-uint64_t last_out_offset;
/* exported for dmg_open */
uint32_t max_compressed_size;
uint32_t max_sectors_per_chunk;
@@ -205,6 +204,7 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
int64_t offset = 0;
uint64_t data_offset;
uint64_t in_offset = ds->data_fork_offset;
+uint64_t out_offset;
type = buff_read_uint32(buffer, offset);
/* skip data that is not a valid MISH block (invalid magic or too small) */
@@ -213,6 +213,9 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
return 0;
}
+/* chunk offsets are relative to this sector number */
+out_offset = buff_read_uint64(buffer, offset + 8);
+
/* location in data fork for (compressed) blob (in bytes) */
data_offset = buff_read_uint64(buffer, offset + 0x18);
in_offset += data_offset;
@@ -233,10 +236,6 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
offset += 4;
if (s->types[i] != 0x8005 && s->types[i] != 1 &&
s->types[i] != 2) {
-if (s->types[i] == 0x && i > 0) {
-ds->last_out_offset = s->sectors[i - 1] +
- s->sectorcounts[i - 1];
-}
chunk_count--;
i--;
offset += 36;
@@ -245,7 +244,7 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
offset += 4;
s->sectors[i] = buff_read_uint64(buffer, offset);
-s->sectors[i] += ds->last_out_offset;
+s->sectors[i] += out_offset;
offset += 8;
s->sectorcounts[i] = buff_read_uint64(buffer, offset);
@@ -419,7 +418,6 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
/* used by dmg_read_mish_block to keep track of the current I/O position */
ds.data_fork_offset = 0;
-ds.last_out_offset = 0;
ds.max_compressed_size = 1;
ds.max_sectors_per_chunk = 1;
--
2.2.1
[Qemu-devel] [PATCH v2 01/12] block/dmg: properly detect the UDIF trailer
DMG files have a variable length with a UDIF trailer at the end of a
file. This UDIF trailer is essential as it describes the contents of
the image. At the moment however, the start of this trailer is almost
always incorrect as bdrv_getlength() returns a multiple of the block
size (rounded up). This results in a failure to recognize DMG files,
resulting in Invalid argument (EINVAL) errors.
As there is no API to retrieve the real file size, look for the magic
header in the last two sectors to find the start of this 512-byte UDIF
trailer (the "koly" block).
The resource fork offset ("info_begin") has its offset adjusted as the
initial value of offset does not mean "end of file" anymore, but "begin
of UDIF trailer".
Signed-off-by: Peter Wu
Reviewed-by: John Snow
---
v2: added R-b, set errp as suggested by Stefan Hajnoczi
---
block/dmg.c | 49 +
1 file changed, 45 insertions(+), 4 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index e455886..9183459 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -131,6 +131,48 @@ static void update_max_chunk_size(BDRVDMGState *s,
uint32_t chunk,
}
}
+static int64_t dmg_find_koly_offset(BlockDriverState *file_bs, Error **errp)
+{
+int64_t length;
+int64_t offset = 0;
+uint8_t buffer[515];
+int i, ret;
+
+/* bdrv_getlength returns a multiple of block size (512), rounded up. Since
+ * dmg images can have odd sizes, try to look for the "koly" magic which
+ * marks the begin of the UDIF trailer (512 bytes). This magic can be found
+ * in the last 511 bytes of the second-last sector or the first 4 bytes of
+ * the last sector (search space: 515 bytes) */
+length = bdrv_getlength(file_bs);
+if (length < 0) {
+error_setg_errno(errp, -length,
+"Failed to get file size while reading UDIF trailer");
+return length;
+} else if (length < 512) {
+error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+"dmg file must be at least 512 bytes long");
+return -EINVAL;
+}
+if (length > 511 + 512) {
+offset = length - 511 - 512;
+}
+length = length < 515 ? length : 515;
+ret = bdrv_pread(file_bs, offset, buffer, length);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed while reading UDIF trailer");
+return ret;
+}
+for (i = 0; i < length - 3; i++) {
+if (buffer[i] == 'k' && buffer[i+1] == 'o' &&
+buffer[i+2] == 'l' && buffer[i+3] == 'y') {
+return offset + i;
+}
+}
+error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+"Could not locate UDIF trailer in dmg file");
+return -EINVAL;
+}
+
static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -145,15 +187,14 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
s->n_chunks = 0;
s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
-/* read offset of info blocks */
-offset = bdrv_getlength(bs->file);
+/* locate the UDIF trailer */
+offset = dmg_find_koly_offset(bs->file, errp);
if (offset < 0) {
ret = offset;
goto fail;
}
-offset -= 0x1d8;
-ret = read_uint64(bs, offset, &info_begin);
+ret = read_uint64(bs, offset + 0x28, &info_begin);
if (ret < 0) {
goto fail;
} else if (info_begin == 0) {
--
2.2.1
[Qemu-devel] [PATCH v2 00/12] block/dmg: (compatibility) fixes and bzip2 support
Hi, This is the second revision of improvements to DMG image file support. See [1] for an overview of the previous patchset. Thanks to John Snow for his efforts in reviewing patches and providing suggestions. The errp suggestion from Stefan Hajnoczi is also incorporated. An overview of changes since v1 (also mentioned in each patch): block/dmg: properly detect the UDIF trailer [+R-b, set errp) block/dmg: extract mish block decoding functionality [+R-b, added comments, expanded commit message, renamed var] block/dmg: extract processing of resource forks [see patch] block/dmg: process a buffer instead of reading ints [+R-b, no changes] block/dmg: validate chunk size to avoid overflow [added offset check] block/dmg: process XML plists [added offset check] block/dmg: set virtual size to a non-zero value [fix commit message] block/dmg: fix sector data offset calculation [use data provided by file] block/dmg: use SectorNumber from BLKX header [new patch] block/dmg: factor out block type check [extracted from bzip patch] block/dmg: support bzip2 block entry types [set/use BZIP2_LIBS] block/dmg: improve zeroes handling [no changes] (the other length checking patch was squashed into the others) Note: in the previous patches I mentioned that dmg2img would result in a shorter file than qemu-img convert. That turns out to be a bug in dmg2img for which a patch is available[2]. A quick test runner is available at https://lekensteyn.nl/files/dmg-tests/. This script depends on the fixed dmg2img program and can then be run as follows: QEMU_IMG=/tmp/qout/qemu-img ./run-dmg-tests.sh dmg-images/*.dmg You will first need some DMG files, I have collected four different public examples with different properties[1]. These can be found in urls.txt at https://lekensteyn.nl/files/dmg-tests/dmg-images/. Kind regards, Peter [1]: https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg03606.html [2]: https://github.com/Lekensteyn/dmg2img/commit/a1d4861b4b0f2ac5090938233a1156bb130cb3ef Peter Wu (12): block/dmg: properly detect the UDIF trailer block/dmg: extract mish block decoding functionality block/dmg: extract processing of resource forks block/dmg: process a buffer instead of reading ints block/dmg: validate chunk size to avoid overflow block/dmg: process XML plists block/dmg: set virtual size to a non-zero value block/dmg: fix sector data offset calculation block/dmg: use SectorNumber from BLKX header block/dmg: factor out block type check block/dmg: support bzip2 block entry types block/dmg: improve zeroes handling block/Makefile.objs | 1 + block/dmg.c | 503 configure | 31 3 files changed, 418 insertions(+), 117 deletions(-) -- 2.2.1
[Qemu-devel] [PATCH v2 05/12] block/dmg: validate chunk size to avoid overflow
Previously the chunk size was not checked, allowing for a large memory
allocation. This patch checks whether the chunks size is within the
resource fork length, and whether the resource fork is below the
trailer of the dmg file.
Signed-off-by: Peter Wu
---
v2: added resource fork offset check
---
block/dmg.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/block/dmg.c b/block/dmg.c
index 4913249..5f6976b 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -319,7 +319,7 @@ static int dmg_read_resource_fork(BlockDriverState *bs,
DmgHeaderState *ds,
ret = read_uint32(bs, offset, &count);
if (ret < 0) {
goto fail;
-} else if (count == 0) {
+} else if (count == 0 || count > info_end - offset) {
ret = -EINVAL;
goto fail;
}
@@ -379,6 +379,11 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
if (ret < 0) {
goto fail;
}
+if (rsrc_fork_offset >= offset ||
+rsrc_fork_length > offset - rsrc_fork_offset) {
+ret = -EINVAL;
+goto fail;
+}
if (rsrc_fork_length != 0) {
ret = dmg_read_resource_fork(bs, &ds,
rsrc_fork_offset, rsrc_fork_length);
--
2.2.1
[Qemu-devel] [PATCH v2 04/12] block/dmg: process a buffer instead of reading ints
As the decoded plist XML is not a pointer in the file,
dmg_read_mish_block must be able to process a buffer instead of a file
pointer. Since the full buffer must be processed, let's change the
return value again to just a success flag.
Signed-off-by: Peter Wu
Reviewed-by: John Snow
---
v2: added R-b
---
block/dmg.c | 60 ++--
1 file changed, 30 insertions(+), 30 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index ed99cf5..4913249 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -100,6 +100,16 @@ static int read_uint32(BlockDriverState *bs, int64_t
offset, uint32_t *result)
return 0;
}
+static inline uint64_t buff_read_uint64(const uint8_t *buffer, int64_t offset)
+{
+return be64_to_cpu(*(uint64_t *)&buffer[offset]);
+}
+
+static inline uint32_t buff_read_uint32(const uint8_t *buffer, int64_t offset)
+{
+return be32_to_cpu(*(uint32_t *)&buffer[offset]);
+}
+
/* Increase max chunk sizes, if necessary. This function is used to calculate
* the buffer sizes needed for compressed/uncompressed chunk I/O.
*/
@@ -184,20 +194,16 @@ typedef struct DmgHeaderState {
uint32_t max_sectors_per_chunk;
} DmgHeaderState;
-static int dmg_read_mish_block(BlockDriverState *bs, DmgHeaderState *ds,
- int64_t offset, uint32_t count)
+static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
+ uint8_t *buffer, uint32_t count)
{
-BDRVDMGState *s = bs->opaque;
uint32_t type, i;
int ret;
size_t new_size;
uint32_t chunk_count;
+int64_t offset = 0;
-ret = read_uint32(bs, offset, &type);
-if (ret < 0) {
-goto fail;
-}
-
+type = buff_read_uint32(buffer, offset);
/* skip data that is not a valid MISH block (invalid magic or too small) */
if (type != 0x6d697368 || count < 244) {
/* assume success for now */
@@ -216,10 +222,7 @@ static int dmg_read_mish_block(BlockDriverState *bs,
DmgHeaderState *ds,
s->sectorcounts = g_realloc(s->sectorcounts, new_size);
for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) {
-ret = read_uint32(bs, offset, &s->types[i]);
-if (ret < 0) {
-goto fail;
-}
+s->types[i] = buff_read_uint32(buffer, offset);
offset += 4;
if (s->types[i] != 0x8005 && s->types[i] != 1 &&
s->types[i] != 2) {
@@ -235,17 +238,11 @@ static int dmg_read_mish_block(BlockDriverState *bs,
DmgHeaderState *ds,
}
offset += 4;
-ret = read_uint64(bs, offset, &s->sectors[i]);
-if (ret < 0) {
-goto fail;
-}
+s->sectors[i] = buff_read_uint64(buffer, offset);
s->sectors[i] += ds->last_out_offset;
offset += 8;
-ret = read_uint64(bs, offset, &s->sectorcounts[i]);
-if (ret < 0) {
-goto fail;
-}
+s->sectorcounts[i] = buff_read_uint64(buffer, offset);
offset += 8;
if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
@@ -256,17 +253,11 @@ static int dmg_read_mish_block(BlockDriverState *bs,
DmgHeaderState *ds,
goto fail;
}
-ret = read_uint64(bs, offset, &s->offsets[i]);
-if (ret < 0) {
-goto fail;
-}
+s->offsets[i] = buff_read_uint64(buffer, offset);
s->offsets[i] += ds->last_in_offset;
offset += 8;
-ret = read_uint64(bs, offset, &s->lengths[i]);
-if (ret < 0) {
-goto fail;
-}
+s->lengths[i] = buff_read_uint64(buffer, offset);
offset += 8;
if (s->lengths[i] > DMG_LENGTHS_MAX) {
@@ -290,8 +281,10 @@ fail:
static int dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
uint64_t info_begin, uint64_t info_length)
{
+BDRVDMGState *s = bs->opaque;
int ret;
uint32_t count, rsrc_data_offset;
+uint8_t *buffer = NULL;
uint64_t info_end;
uint64_t offset;
@@ -332,16 +325,23 @@ static int dmg_read_resource_fork(BlockDriverState *bs,
DmgHeaderState *ds,
}
offset += 4;
-ret = dmg_read_mish_block(bs, ds, offset, count);
+buffer = g_realloc(buffer, count);
+ret = bdrv_pread(bs->file, offset, buffer, count);
+if (ret < 0) {
+goto fail;
+}
+
+ret = dmg_read_mish_block(s, ds, buffer, count);
if (ret < 0) {
goto fail;
}
/* advance offset by size of resource */
offset += count;
}
-return 0;
+ret = 0;
fail:
+g_free(buffer);
return ret;
}
--
2.2.1
Re: [Qemu-devel] [PATCH 10/10] block/dmg: improve zeroes handling
On Monday 05 January 2015 14:48:03 John Snow wrote:
> On 12/27/2014 10:01 AM, Peter Wu wrote:
> > Disk images may contain large all-zeroes gaps (1.66k sectors or 812 MiB
> > is seen in the real world). These blocks (type 2) do not need to be
> > extracted into a temporary buffer, there is no need to allocate memory
> > for these blocks nor to check its length.
> >
> > (For the test image, the maximum uncompressed size is 1054371 bytes,
> > probably for a bzip2-compressed block.)
> >
> > Signed-off-by: Peter Wu
> > ---
> > block/dmg.c | 18 +-
> > 1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/block/dmg.c b/block/dmg.c
> > index 67d4e2b..b84ba7e 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -137,7 +137,9 @@ static void update_max_chunk_size(BDRVDMGState *s,
> > uint32_t chunk,
> > uncompressed_sectors = (s->lengths[chunk] + 511) / 512;
> > break;
> > case 2: /* zero */
> > -uncompressed_sectors = s->sectorcounts[chunk];
> > +/* as the all-zeroes block may be large, it is treated specially:
> > the
> > + * sector is not copied from a large buffer, a simple memset is
> > used
> > + * instead. Therefore uncompressed_sectors does not need to be
> > set. */
> > break;
> > }
> >
> > @@ -260,7 +262,9 @@ static int dmg_read_mish_block(BDRVDMGState *s,
> > DmgHeaderState *ds,
> > s->sectorcounts[i] = buff_read_uint64(buffer, offset);
> > offset += 8;
> >
> > -if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
> > +/* all-zeroes sector (type 2) does not need to be "uncompressed"
> > and can
> > + * therefore be unbounded. */
> > +if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX)
> > {
> > error_report("sector count %" PRIu64 " for chunk %" PRIu32
> >" is larger than max (%u)",
> >s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
> > @@ -621,9 +625,6 @@ static inline int dmg_read_chunk(BlockDriverState *bs,
> > uint64_t sector_num)
> > return -1;
> > }
> > break;
> > -case 2: /* zero */
> > -memset(s->uncompressed_chunk, 0, 512 * s->sectorcounts[chunk]);
> > -break;
>
> This is just a personal preference, but I might actually prefer a little
> comment here that guards against anyone helpfully re-adding this case
> statement again in the future, if you really want to split it out.
Ok, makes sense. I'll add an appropriate comment here.
> > }
> > s->current_chunk = chunk;
> > }
> > @@ -641,6 +642,13 @@ static int dmg_read(BlockDriverState *bs, int64_t
> > sector_num,
> > if (dmg_read_chunk(bs, sector_num + i) != 0) {
> > return -1;
> > }
> > +/* Special case: current chunk is all zeroes. Do not perform a
> > memcpy as
> > + * s->uncompressed_chunk may be too small to cover the large
> > all-zeroes
> > + * section. */
> > +if (s->types[s->current_chunk] == 2) { /* all zeroes block entry */
> > +memset(buf + i * 512, 0, 512);
> > +continue;
> > +}
>
> Why even call dmg_read_chunk first if we just ignore it? You can
> probably move this special case forward and only do the dmg_read_chunk
> if it fails.
dmg_read_chunk is needed to find the current sector (which requires a
binary search if the sector is not within the current chunk). I have
updated the comments to clarify this.
> Or maybe we can keep this from becoming too fragmented by giving the
> read_chunk function access to the destination buffer somehow (parameter,
> structure member) and keeping all the logic for re-inflating the chunks
> together at the same layer.
dmg_read_chunk could be modified to return a pointer inside
s->uncompressed_chunk (adjusted to the sector number), but this just
moves the special case inside dmg_read_chunk. Alternatively, the whole
memcpy/memset stuff could be moved into dmg_read_chunk, but then the
function could be renamed to dmg_read_into_sector.
I treat dmg_read_chunk as a function which reads the current chunk (if
not already read), and returns the cached chunk otherwise. Then the
caller can decide on the part of the chunk it is interested in. As the
zero chunk does not really update the "current chunk contents", it'll
need special treatment.
The "continue" keyword is used while it actually means "either memset or
memcpy", this approach was chosen to avoid re-indenting the below memcpy
code.
Kind regards,
Peter
> > sector_offset_in_chunk = sector_num + i -
> > s->sectors[s->current_chunk];
> > memcpy(buf + i * 512,
> > s->uncompressed_chunk + sector_offset_in_chunk * 512, 512);
> >
>
> Looks good, otherwise. Thank you :)
>
> --j
Re: [Qemu-devel] [PATCH 08/10] block/dmg: fix sector data offset calculation
On Friday 02 January 2015 19:05:29 John Snow wrote:
> On 12/27/2014 10:01 AM, Peter Wu wrote:
> > This patch addresses two issues:
> >
> > - The data fork offset was not taken into account, resulting in failure
> > to read an InstallESD.dmg file (5164763151 bytes) which had a
> > non-zero DataForkOffset field.
> > - The offset of the previous block ("partition") was unconditionally
> > added to the current block because older files would start the input
> > offset of a new block at zero. Newer files (including vlc-2.1.5.dmg,
> > tuxpaint-0.9.15-macosx.dmg and OS X Yosemite [MAS].dmg) failed in
> > reads because these files have a continuous input offset.
> >
>
> What does "continuous input offset" mean? This change is not as clear to
> me, see below.
By "continuous" I mean that the new files have absolute offsets while
the offsets in older files were relative to the previous block.
> > Signed-off-by: Peter Wu
> > ---
> > block/dmg.c | 16 +++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/dmg.c b/block/dmg.c
> > index 984997f..93b597f 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -179,6 +179,7 @@ static int64_t dmg_find_koly_offset(BlockDriverState
> > *file_bs)
> > typedef struct DmgHeaderState {
> > /* used internally by dmg_read_mish_block to remember offsets of
> > blocks
> >* across calls */
> > +uint64_t data_fork_offset;
> > uint64_t last_in_offset;
> > uint64_t last_out_offset;
> > /* exported for dmg_open */
> > @@ -194,6 +195,7 @@ static int dmg_read_mish_block(BDRVDMGState *s,
> > DmgHeaderState *ds,
> > size_t new_size;
> > uint32_t chunk_count;
> > int64_t offset = 0;
> > +uint64_t in_offset = ds->data_fork_offset;
> >
> > type = buff_read_uint32(buffer, offset);
> > /* skip data that is not a valid MISH block (invalid magic or too
> > small) */
> > @@ -246,7 +248,12 @@ static int dmg_read_mish_block(BDRVDMGState *s,
> > DmgHeaderState *ds,
> > }
> >
> > s->offsets[i] = buff_read_uint64(buffer, offset);
> > -s->offsets[i] += ds->last_in_offset;
> > +/* If this offset is below the previous chunk end, then assume
> > that all
> > + * following offsets are after the previous chunks. */
> > +if (s->offsets[i] + in_offset < ds->last_in_offset) {
> > +in_offset = ds->last_in_offset;
> > +}
> > +s->offsets[i] += in_offset;
>
> I take it that all of the offsets referenced in the mish structures are
> relative to the start of the data fork block, which is why we are taking
> a value from the koly block and applying it to mish block values.
>
> correct?
Correct, the mish block describes the contents of the data fork.
http://newosxbook.com/DMG.html says:
typedef struct {
// ...
uint64_t CompressedOffset; // Start of chunk in data fork
uint64_t CompressedLength; // Count of bytes of chunk, in data fork
} __attribute__((__packed__)) BLKXChunkEntry;
> > offset += 8;
> >
> > s->lengths[i] = buff_read_uint64(buffer, offset);
> > @@ -400,6 +407,7 @@ static int dmg_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > bs->read_only = 1;
> > s->n_chunks = 0;
> > s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> > +ds.data_fork_offset = 0;
> > ds.last_in_offset = 0;
> > ds.last_out_offset = 0;
> > ds.max_compressed_size = 1;
> > @@ -412,6 +420,12 @@ static int dmg_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > goto fail;
> > }
> >
> > +/* offset of data fork (DataForkOffset) */
> > +ret = read_uint64(bs, offset + 0x18, &ds.data_fork_offset);
> > +if (ret < 0) {
> > +goto fail;
> > +}
> > +
> > /* offset of resource fork (RsrcForkOffset) */
> > ret = read_uint64(bs, offset + 0x28, &rsrc_fork_offset);
> > if (ret < 0) {
> >
>
> A general question here:
>
> Are we ever reading the preamble of the mish block? I see we are reading
> the 'n' items of 40-byte chunk data, but is there a reason we skip the
> first 200 bytes of mish data, or have I misread the documents on DMG
> that exist?
We only use the Signature field to verify that we indeed have a BLKX
en
Re: [Qemu-devel] [PATCH 06/10] block/dmg: process XML plists
On Friday 02 January 2015 19:04:32 John Snow wrote:
> On 12/27/2014 10:01 AM, Peter Wu wrote:
> > The format is simple enough to avoid using a full-blown XML parser.
> > The offsets are based on the description at
> > http://newosxbook.com/DMG.html
> >
> > Signed-off-by: Peter Wu
> > ---
> > block/dmg.c | 69
> > +
> > 1 file changed, 69 insertions(+)
> >
> > diff --git a/block/dmg.c b/block/dmg.c
> > index 19e4fe2..c03ea01 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -26,6 +26,7 @@
> > #include "qemu/bswap.h"
> > #include "qemu/module.h"
> > #include
> > +#include
> >
> > enum {
> > /* Limit chunk sizes to prevent unreasonable amounts of memory being
> > used
> > @@ -333,12 +334,66 @@ fail:
> > return ret;
> > }
> >
> > +static int dmg_read_plist_xml(BlockDriverState *bs, DmgHeaderState *ds,
> > + uint64_t info_begin, uint64_t info_length)
> > +{
> > +BDRVDMGState *s = bs->opaque;
> > +int ret;
> > +uint8_t *buffer = NULL;
> > +char *data_begin, *data_end;
> > +
> > +/* Have at least some length to avoid NULL for g_malloc. Attempt to
> > set a
> > + * safe upper cap on the data length. A test sample had a XML length of
> > + * about 1 MiB. */
> > +if (info_length == 0 || info_length > 16 * 1024 * 1024) {
> > +ret = -EINVAL;
> > +goto fail;
> > +}
> > +
> > +buffer = g_malloc(info_length + 1);
> > +buffer[info_length] = '\0';
> > +ret = bdrv_pread(bs->file, info_begin, buffer, info_length);
> > +if (ret != info_length) {
> > +ret = -EINVAL;
> > +goto fail;
> > +}
> > +
> > +/* look for The data is 284 (0x11c) bytes after
> > base64
> > + * decode. The actual data element has 431 (0x1af) bytes which
> > includes tabs
> > + * and line feeds. */
> > +data_end = (char *)buffer;
> > +while ((data_begin = strstr(data_end, "")) != NULL) {
> > +gsize out_len = 0;
> > +
> > +data_begin += 6;
> > +data_end = strstr(data_begin, "");
> > +/* malformed XML? */
> > +if (data_end == NULL) {
> > +ret = -EINVAL;
> > +goto fail;
> > +}
> > +*data_end++ = '\0';
> > +g_base64_decode_inplace(data_begin, &out_len);
> > +ret = dmg_read_mish_block(s, ds, (uint8_t *)data_begin,
> > + (uint32_t)out_len);
> > +if (ret < 0) {
> > +goto fail;
> > +}
> > +}
> > +ret = 0;
> > +
> > +fail:
> > +g_free(buffer);
> > +return ret;
> > +}
> > +
>
> This starts to make me a little nervous, because we're ignoring so much
> of the XML document structure here and just effectively performing a
> regular search for "(.*)".
>
> Can we guarantee that the ONLY time the data element is used in this
> document is when it is being used in the exact context we are expecting
> here, where it contains the b64 mish data we expect it to?
>
> i.e. it is always in a path like this as detailed by
> http://newosxbook.com/DMG.html :
>
> plist/dict/key[text()='resource-fork']/following-sibling::dict/key[text()='blkx']/following-sibling::array/dict/key[text()='data']/following-sibling::data
>
> I notice that this document says other sections MAY be present, do any
> of them ever need to be parsed? Has anyone written about them before?
>
> Do we know if any use data sections?
>
> I suppose at the very least, sections of interest are always going to
> include the "mish" magic, so that should probably keep us from doing
> anything too stupid ...
I did not find DMG files with elements at other locations. If it
would occur, at worst we would fail to parse a DMG file. I think that
introducing a XML parser here would introduce a risk for a minor benefit
(being prepared for future cases).
Since this is a property list, in theory people could include all kinds
of data for different keys (which would then be matched by the current
implementation). But how likely is this for a disk image?
FWIW, I looked into the dmg2img program and that also looks for the
strings "" and "". Nobody has raised a bug for that program
so far.
Do you think th
Re: [Qemu-devel] [PATCH 03/10] block/dmg: extract processing of resource forks
On Friday 02 January 2015 19:01:06 John Snow wrote:
> On 12/27/2014 10:01 AM, Peter Wu wrote:
> > Besides the offset, also read the resource length. This length is now
> > used in the extracted function to verify the end of the resource fork
> > against "count" from the resource fork.
> >
> > Signed-off-by: Peter Wu
> > ---
> > block/dmg.c | 90
> > -
> > 1 file changed, 59 insertions(+), 31 deletions(-)
> >
> > diff --git a/block/dmg.c b/block/dmg.c
> > index 6dc6dbb..7f49388 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -278,38 +278,13 @@ fail:
> > return ret;
> > }
> >
> > -static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
> > -Error **errp)
> > +static int dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
> > + uint64_t info_begin, uint64_t
> > info_length)
> > {
> > -BDRVDMGState *s = bs->opaque;
> > -DmgHeaderState ds;
> > -uint64_t info_begin, info_end;
> > -uint32_t count, tmp;
> > -int64_t offset;
> > int ret;
> > -
> > -bs->read_only = 1;
> > -s->n_chunks = 0;
> > -s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> > -ds.last_in_offset = 0;
> > -ds.last_out_offset = 0;
> > -ds.max_compressed_size = 1;
> > -ds.max_sectors_per_chunk = 1;
> > -
> > -/* locate the UDIF trailer */
> > -offset = dmg_find_koly_offset(bs->file);
> > -if (offset < 0) {
> > -ret = offset;
> > -goto fail;
> > -}
> > -
> > -ret = read_uint64(bs, offset + 0x28, &info_begin);
> > -if (ret < 0) {
> > -goto fail;
> > -} else if (info_begin == 0) {
> > -ret = -EINVAL;
> > -goto fail;
> > -}
> > +uint32_t count, tmp;
> > +uint64_t info_end;
> > +uint64_t offset;
> >
> > ret = read_uint32(bs, info_begin, &tmp);
> > if (ret < 0) {
> > @@ -326,6 +301,10 @@ static int dmg_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > ret = -EINVAL;
> > goto fail;
> > }
> > +if (count > info_length) {
> > +ret = -EINVAL;
> > +goto fail;
> > +}
> > info_end = info_begin + count;
> >
> > /* begin of mish block */
> > @@ -342,12 +321,61 @@ static int dmg_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > }
> > offset += 4;
> >
> > -ret = dmg_read_mish_block(bs, &ds, offset, count);
> > +ret = dmg_read_mish_block(bs, ds, offset, count);
> > if (ret < 0) {
> > goto fail;
> > }
> > offset += count;
> > }
> > +return 0;
> > +
> > +fail:
> > +return ret;
> > +}
> > +
> > +static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
> > +Error **errp)
> > +{
> > +BDRVDMGState *s = bs->opaque;
> > +DmgHeaderState ds;
> > +uint64_t rsrc_fork_offset, rsrc_fork_length;
> > +int64_t offset;
> > +int ret;
> > +
> > +bs->read_only = 1;
> > +s->n_chunks = 0;
> > +s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> > +ds.last_in_offset = 0;
> > +ds.last_out_offset = 0;
> > +ds.max_compressed_size = 1;
> > +ds.max_sectors_per_chunk = 1;
> > +
> > +/* locate the UDIF trailer */
> > +offset = dmg_find_koly_offset(bs->file);
> > +if (offset < 0) {
> > +ret = offset;
> > +goto fail;
> > +}
> > +
> > +/* offset of resource fork (RsrcForkOffset) */
> > +ret = read_uint64(bs, offset + 0x28, &rsrc_fork_offset);
> > +if (ret < 0) {
> > +goto fail;
> > +}
> > +ret = read_uint64(bs, offset + 0x30, &rsrc_fork_length);
> > +if (ret < 0) {
> > +goto fail;
> > +}
> > +if (rsrc_fork_offset != 0 && rsrc_fork_length != 0) {
> > +ret = dmg_read_resource_fork(bs, &ds,
> > + rsrc_fork_offset, rsrc_fork_length);
> > +if (ret < 0) {
> > +goto fail;
> > +}
> > +}
Re: [Qemu-devel] [PATCH 02/10] block/dmg: extract mish block decoding functionality
On Friday 02 January 2015 18:59:38 John Snow wrote:
>
> On 12/27/2014 10:01 AM, Peter Wu wrote:
> > Extract the mish block decoder such that this can be used for other
> > formats in the future. A new DmgHeaderState struct is introduced to
> > share state while decoding.
> >
> > The code is kept unchanged as much as possible, a "fail" label is added
> > for example where a simple return would probably do.
> >
> > Signed-off-by: Peter Wu
> > ---
> > block/dmg.c | 216
> > +++-
> > 1 file changed, 125 insertions(+), 91 deletions(-)
> >
> > diff --git a/block/dmg.c b/block/dmg.c
> > index df274f9..6dc6dbb 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -164,19 +164,137 @@ static int64_t dmg_find_koly_offset(BlockDriverState
> > *file_bs)
> > return -EINVAL;
> > }
> >
> > +/* used when building the sector table */
> > +typedef struct DmgHeaderState {
> > +/* used internally by dmg_read_mish_block to remember offsets of blocks
> > + * across calls */
> > +uint64_t last_in_offset;
> > +uint64_t last_out_offset;
> > +/* exported for dmg_open */
> > +uint32_t max_compressed_size;
> > +uint32_t max_sectors_per_chunk;
> > +} DmgHeaderState;
> > +
> > +static int dmg_read_mish_block(BlockDriverState *bs, DmgHeaderState *ds,
> > + int64_t offset, uint32_t count)
> > +{
> > +BDRVDMGState *s = bs->opaque;
> > +uint32_t type, i;
> > +int ret;
> > +size_t new_size;
> > +uint32_t chunk_count;
> > +
> > +ret = read_uint32(bs, offset, &type);
> > +if (ret < 0) {
> > +goto fail;
> > +}
> > +
> > +/* skip data that is not a valid MISH block (invalid magic or too
> > small) */
> > +if (type != 0x6d697368 || count < 244) {
> > +/* assume success for now */
> > +return 0;
> > +}
> > +
> > +offset += 4;
> > +offset += 200;
> > +
> > +chunk_count = (count - 204) / 40;
> > +new_size = sizeof(uint64_t) * (s->n_chunks + chunk_count);
> > +s->types = g_realloc(s->types, new_size / 2);
> > +s->offsets = g_realloc(s->offsets, new_size);
> > +s->lengths = g_realloc(s->lengths, new_size);
> > +s->sectors = g_realloc(s->sectors, new_size);
> > +s->sectorcounts = g_realloc(s->sectorcounts, new_size);
> > +
> > +for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) {
> > +ret = read_uint32(bs, offset, &s->types[i]);
> > +if (ret < 0) {
> > +goto fail;
> > +}
> > +offset += 4;
> > +if (s->types[i] != 0x8005 && s->types[i] != 1 &&
> > +s->types[i] != 2) {
> > +if (s->types[i] == 0x && i > 0) {
> > +ds->last_in_offset = s->offsets[i - 1] + s->lengths[i - 1];
> > +ds->last_out_offset = s->sectors[i - 1] +
> > + s->sectorcounts[i - 1];
> > +}
> > +chunk_count--;
> > +i--;
> > +offset += 36;
> > +continue;
> > +}
> > +offset += 4;
> > +
> > +ret = read_uint64(bs, offset, &s->sectors[i]);
> > +if (ret < 0) {
> > +goto fail;
> > +}
> > +s->sectors[i] += ds->last_out_offset;
> > +offset += 8;
> > +
> > +ret = read_uint64(bs, offset, &s->sectorcounts[i]);
> > +if (ret < 0) {
> > +goto fail;
> > +}
> > +offset += 8;
> > +
> > +if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
> > +error_report("sector count %" PRIu64 " for chunk %" PRIu32
> > + " is larger than max (%u)",
> > + s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
> > +ret = -EINVAL;
> > +goto fail;
> > +}
> > +
> > +ret = read_uint64(bs, offset, &s->offsets[i]);
> > +if (ret < 0) {
> > +goto fail;
> > +}
> > +s->offsets[i] += ds->last_in_offset;
> > +offset += 8;
> > +
> > +ret
Re: [Qemu-devel] [PATCH 01/10] block/dmg: properly detect the UDIF trailer
On Friday 02 January 2015 18:58:00 John Snow wrote:
>
> On 12/27/2014 10:01 AM, Peter Wu wrote:
> > DMG files have a variable length with a UDIF trailer at the end of a
> > file. This UDIF trailer is essential as it describes the contents of
> > the image. At the moment however, the start of this trailer is almost
> > always incorrect as bdrv_getlength() returns a multiple of the block
> > size (rounded up). This results in a failure to recognize DMG files,
> > resulting in Invalid argument (EINVAL) errors.
> >
> > As there is no API to retrieve the real file size, look for the magic
> > header in the last two sectors to find the start of this 512-byte UDIF
> > trailer (the "koly" block).
> >
> > The resource fork offset ("info_begin") has its offset adjusted as the
> > initial value of offset does not mean "end of file" anymore, but "begin
> > of UDIF trailer".
> >
> > Signed-off-by: Peter Wu
> > ---
> > block/dmg.c | 40
> > 1 file changed, 36 insertions(+), 4 deletions(-)
> >
> > diff --git a/block/dmg.c b/block/dmg.c
> > index e455886..df274f9 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -131,6 +131,39 @@ static void update_max_chunk_size(BDRVDMGState *s,
> > uint32_t chunk,
> > }
> > }
> >
> > +static int64_t dmg_find_koly_offset(BlockDriverState *file_bs)
> > +{
> > +int64_t length;
> > +int64_t offset = 0;
> > +uint8_t buffer[515];
> > +int i, ret;
> > +
> > +/* bdrv_getlength returns a multiple of block size (512), rounded up.
> > Since
> > + * dmg images can have odd sizes, try to look for the "koly" magic
> > which
> > + * marks the begin of the UDIF trailer (512 bytes). This magic can be
> > found
> > + * in the last 511 bytes of the second-last sector or the first 4
> > bytes of
> > + * the last sector (search space: 515 bytes) */
> > +length = bdrv_getlength(file_bs);
> > +if (length < 512) {
> > +return length < 0 ? length : -EINVAL;
> > +}
> > +if (length > 511 + 512) {
> > +offset = length - 511 - 512;
> > +}
> > +length = length < 515 ? length : 515;
> > +ret = bdrv_pread(file_bs, offset, buffer, length);
> > +if (ret < 4) {
> > +return ret < 0 ? ret : -EINVAL;
> > +}
> > +for (i = 0; i < length - 3; i++) {
> > +if (buffer[i] == 'k' && buffer[i+1] == 'o' &&
> > +buffer[i+2] == 'l' && buffer[i+3] == 'y') {
> > +return offset + i;
> > +}
> > +}
> > +return -EINVAL;
> > +}
> > +
> > static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
> > Error **errp)
> > {
> > @@ -145,15 +178,14 @@ static int dmg_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > s->n_chunks = 0;
> > s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> >
> > -/* read offset of info blocks */
> > -offset = bdrv_getlength(bs->file);
> > +/* locate the UDIF trailer */
> > +offset = dmg_find_koly_offset(bs->file);
> > if (offset < 0) {
> > ret = offset;
> > goto fail;
> > }
> > -offset -= 0x1d8;
> >
> > -ret = read_uint64(bs, offset, &info_begin);
> > +ret = read_uint64(bs, offset + 0x28, &info_begin);
> > if (ret < 0) {
> > goto fail;
> > } else if (info_begin == 0) {
> >
>
> If there really is no convenient way to retrieve the real length ...
> (Stefan: Would that be difficult to add?)
>
> Reviewed-by: John Snow
The real length can be stored, but it takes more effort to do so. See
the stalled work on this bdrv-getlength-conversion branch[1] and in
particular "block: do not directly set total_sectors"[2].
--
Kind regards,
Peter
https://lekensteyn.nl
[1]: https://git.lekensteyn.nl/peter/qemu/log/?h=bdrv-getlength-conversion
[2]:
https://git.lekensteyn.nl/peter/qemu/commit/?h=bdrv-getlength-conversion&id=e5164735e5b674a10134894589a060a0f5f32ccc
Re: [Qemu-devel] [PATCH 00/10] block/dmg: (compatibility) fixes and bzip2 support
On Friday 02 January 2015 13:58:33 John Snow wrote:
>
> On 01/02/2015 01:46 PM, Peter Wu wrote:
> > FYI, I plan to make some more changes:
> >
> > - do not require offset ≠0 for resource fork and XML offsets.
> > Technically it is allowed, do you agree on this change?
>
> If you have seen this in the wild, I definitely agree. If you haven't, I
> am not against the change, but there's likely no hurry to include it in
> this series if the changes are not simple.
It would involve only a removal of "rsrc_fork_offset != 0 && " in patch
3 and "plist_xml_offset != 0 && " in patch 5. I have not seen it in the
real world, only when trying to construct a dmg file by hand for testing
purposes. The change is simple and can be squashed in the patch.
It makes sense since previously only the offset was checked. Now the
length is checked instead. Before:
/* read offset */
ret = read_uint64(bs, offset, &info_begin);
if (ret < 0) {
goto fail;
} else if (info_begin == 0) {
/* assume invalid file when offset is zero */
ret = -EINVAL;
goto fail;
}
After (in current patch series):
/* offset of resource fork (RsrcForkOffset) */
ret = read_uint64(bs, offset + 0x28, &rsrc_fork_offset);
if (ret < 0) {
goto fail;
}
ret = read_uint64(bs, offset + 0x30, &rsrc_fork_length);
if (ret < 0) {
goto fail;
}
// ...
if (rsrc_fork_offset != 0 && rsrc_fork_length != 0) {
ret = dmg_read_resource_fork(bs, &ds,
In the current patch series both the offset and lengths are checked, but
it is sufficient to look at just the length.
Kind regards,
Peter
> > - improve offset checking
> > https://git.lekensteyn.nl/peter/qemu/commit/?h=block-dmg-2.3&id=41fd83773361923f668f54796ff563660b77e96c
> > (squash with the existing length checking patch)
> >
> > - (not part of this series, but for future consideration) read
> > errors currently return 1 (EPERM). EIO or EINVAL would probably a
> > better choice depending on the error type.
> >
> > Other than that, the patches should be ready for review. Thank you
> > in advance.
> >
> > Kind regards,
> > Peter
> > https://lekensteyn.nl
> > (pardon my brevity, top-posting and formatting, sent from my phone)
> >
> >
> > On January 2, 2015 5:31:33 PM CET, John Snow wrote:
> >>
> >>
> >> On 01/02/2015 09:14 AM, Stefan Hajnoczi wrote:
> >>> On Sat, Dec 27, 2014 at 04:01:34PM +0100, Peter Wu wrote:
> >>>> These series improve QEMU support for DMG image files:
> >>>
> >>> Hi,
> >>> Thanks for this patch series. Kevin and I consider patches for
> >> merging
> >>> after they have a Reviewed-by: from at least 1 other QEMU
> >> contributor.
> >>>
> >>> I have CCed John Snow.
> >>>
> >>> John: If you are busy, please CC someone else or let us know so this
> >>> series can get reviewed.
> >>>
> >>> Stefan
> >>>
> >>
> >> Just recomposing myself post-vacation, I will start looking this over
> >> today.
> >>
> >> --John
Re: [Qemu-devel] [PATCH 00/10] block/dmg: (compatibility) fixes and bzip2 support
FYI, I plan to make some more changes: - do not require offset ≠0 for resource fork and XML offsets. Technically it is allowed, do you agree on this change? - improve offset checking https://git.lekensteyn.nl/peter/qemu/commit/?h=block-dmg-2.3&id=41fd83773361923f668f54796ff563660b77e96c (squash with the existing length checking patch) - (not part of this series, but for future consideration) read errors currently return 1 (EPERM). EIO or EINVAL would probably a better choice depending on the error type. Other than that, the patches should be ready for review. Thank you in advance. Kind regards, Peter https://lekensteyn.nl (pardon my brevity, top-posting and formatting, sent from my phone) On January 2, 2015 5:31:33 PM CET, John Snow wrote: > > >On 01/02/2015 09:14 AM, Stefan Hajnoczi wrote: >> On Sat, Dec 27, 2014 at 04:01:34PM +0100, Peter Wu wrote: >>> These series improve QEMU support for DMG image files: >> >> Hi, >> Thanks for this patch series. Kevin and I consider patches for >merging >> after they have a Reviewed-by: from at least 1 other QEMU >contributor. >> >> I have CCed John Snow. >> >> John: If you are busy, please CC someone else or let us know so this >> series can get reviewed. >> >> Stefan >> > >Just recomposing myself post-vacation, I will start looking this over >today. > >--John
[Qemu-devel] [PATCH 10/10] block/dmg: improve zeroes handling
Disk images may contain large all-zeroes gaps (1.66k sectors or 812 MiB
is seen in the real world). These blocks (type 2) do not need to be
extracted into a temporary buffer, there is no need to allocate memory
for these blocks nor to check its length.
(For the test image, the maximum uncompressed size is 1054371 bytes,
probably for a bzip2-compressed block.)
Signed-off-by: Peter Wu
---
block/dmg.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 67d4e2b..b84ba7e 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -137,7 +137,9 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t
chunk,
uncompressed_sectors = (s->lengths[chunk] + 511) / 512;
break;
case 2: /* zero */
-uncompressed_sectors = s->sectorcounts[chunk];
+/* as the all-zeroes block may be large, it is treated specially: the
+ * sector is not copied from a large buffer, a simple memset is used
+ * instead. Therefore uncompressed_sectors does not need to be set. */
break;
}
@@ -260,7 +262,9 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
s->sectorcounts[i] = buff_read_uint64(buffer, offset);
offset += 8;
-if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
+/* all-zeroes sector (type 2) does not need to be "uncompressed" and
can
+ * therefore be unbounded. */
+if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
error_report("sector count %" PRIu64 " for chunk %" PRIu32
" is larger than max (%u)",
s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
@@ -621,9 +625,6 @@ static inline int dmg_read_chunk(BlockDriverState *bs,
uint64_t sector_num)
return -1;
}
break;
-case 2: /* zero */
-memset(s->uncompressed_chunk, 0, 512 * s->sectorcounts[chunk]);
-break;
}
s->current_chunk = chunk;
}
@@ -641,6 +642,13 @@ static int dmg_read(BlockDriverState *bs, int64_t
sector_num,
if (dmg_read_chunk(bs, sector_num + i) != 0) {
return -1;
}
+/* Special case: current chunk is all zeroes. Do not perform a memcpy
as
+ * s->uncompressed_chunk may be too small to cover the large all-zeroes
+ * section. */
+if (s->types[s->current_chunk] == 2) { /* all zeroes block entry */
+memset(buf + i * 512, 0, 512);
+continue;
+}
sector_offset_in_chunk = sector_num + i - s->sectors[s->current_chunk];
memcpy(buf + i * 512,
s->uncompressed_chunk + sector_offset_in_chunk * 512, 512);
--
2.2.1
[Qemu-devel] [PATCH 09/10] block/dmg: support bzip2 block entry types
This patch adds support for bzip2-compressed block entries as introduced
with OS X 10.4 (source: https://en.wikipedia.org/wiki/Apple_Disk_Image).
It was tested against a 5.2G "OS X Yosemite" installation image which
stores the BLXX block in the XML property list (instead of resource
forks) and has over 5k chunks.
New configure entries are added (--enable-bzip2 / --disable-bzip2) to
control inclusion of bzip2 functionality (which requires linking against
libbz2). The help message suggests that this option is needed for DMG
files, but the tests are generic enough that other parts of QEMU can use
bzip2 if needed.
The identifiers are based on http://newosxbook.com/DMG.html.
The decompression routines are based on the zlib case, but as there is
no way to reset the decompression state (unlike zlib), memory is
allocated and deallocated for every decompression. This should not be
problematic as the decompression takes most of the time and as blocks
are typically about/over 1 MiB in size, only one allocation is done
every 2000 sectors.
Signed-off-by: Peter Wu
---
block/dmg.c | 56 +---
configure | 31 +++
2 files changed, 84 insertions(+), 3 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 93b597f..67d4e2b 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -26,6 +26,9 @@
#include "qemu/bswap.h"
#include "qemu/module.h"
#include
+#ifdef CONFIG_BZIP2
+#include
+#endif
#include
enum {
@@ -56,6 +59,9 @@ typedef struct BDRVDMGState {
uint8_t *compressed_chunk;
uint8_t *uncompressed_chunk;
z_stream zstream;
+#ifdef CONFIG_BZIP2
+bz_stream bzstream;
+#endif
} BDRVDMGState;
static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
@@ -123,6 +129,7 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t
chunk,
switch (s->types[chunk]) {
case 0x8005: /* zlib compressed */
+case 0x8006: /* bzip2 compressed */
compressed_size = s->lengths[chunk];
uncompressed_sectors = s->sectorcounts[chunk];
break;
@@ -187,6 +194,21 @@ typedef struct DmgHeaderState {
uint32_t max_sectors_per_chunk;
} DmgHeaderState;
+static bool dmg_is_valid_block_type(uint32_t entry_type)
+{
+switch (entry_type) {
+case 0x0001:/* uncompressed */
+case 0x0002:/* zeroes */
+case 0x8005:/* zlib */
+#ifdef CONFIG_BZIP2
+case 0x8006:/* bzip2 */
+#endif
+return true;
+default:
+return false;
+}
+}
+
static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
uint8_t *buffer, uint32_t count)
{
@@ -218,8 +240,7 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) {
s->types[i] = buff_read_uint32(buffer, offset);
offset += 4;
-if (s->types[i] != 0x8005 && s->types[i] != 1 &&
-s->types[i] != 2) {
+if (!dmg_is_valid_block_type(s->types[i])) {
if (s->types[i] == 0x && i > 0) {
ds->last_in_offset = s->offsets[i - 1] + s->lengths[i - 1];
ds->last_out_offset = s->sectors[i - 1] +
@@ -534,13 +555,14 @@ static inline int dmg_read_chunk(BlockDriverState *bs,
uint64_t sector_num)
if (!is_sector_in_chunk(s, s->current_chunk, sector_num)) {
int ret;
uint32_t chunk = search_chunk(s, sector_num);
+uint64_t total_out;
if (chunk >= s->n_chunks) {
return -1;
}
s->current_chunk = s->n_chunks;
-switch (s->types[chunk]) {
+switch (s->types[chunk]) { /* block entry type */
case 0x8005: { /* zlib compressed */
/* we need to buffer, because only the chunk as whole can be
* inflated. */
@@ -564,6 +586,34 @@ static inline int dmg_read_chunk(BlockDriverState *bs,
uint64_t sector_num)
return -1;
}
break; }
+#ifdef CONFIG_BZIP2
+case 0x8006: /* bzip2 compressed */
+/* we need to buffer, because only the chunk as whole can be
+ * inflated. */
+ret = bdrv_pread(bs->file, s->offsets[chunk],
+ s->compressed_chunk, s->lengths[chunk]);
+if (ret != s->lengths[chunk]) {
+return -1;
+}
+
+ret = BZ2_bzDecompressInit(&s->bzstream, 0, 0);
+if (ret != BZ_OK) {
+return -1;
+}
+s->bzstream.next_in = (char *)s->compressed_chunk;
+s->bzstream.avail_in = (unsigned int) s->lengths[chunk];
+s->bzstream.next_out = (char *)s->uncompressed_chunk;
+
[Qemu-devel] [PATCH 06/10] block/dmg: process XML plists
The format is simple enough to avoid using a full-blown XML parser.
The offsets are based on the description at
http://newosxbook.com/DMG.html
Signed-off-by: Peter Wu
---
block/dmg.c | 69 +
1 file changed, 69 insertions(+)
diff --git a/block/dmg.c b/block/dmg.c
index 19e4fe2..c03ea01 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -26,6 +26,7 @@
#include "qemu/bswap.h"
#include "qemu/module.h"
#include
+#include
enum {
/* Limit chunk sizes to prevent unreasonable amounts of memory being used
@@ -333,12 +334,66 @@ fail:
return ret;
}
+static int dmg_read_plist_xml(BlockDriverState *bs, DmgHeaderState *ds,
+ uint64_t info_begin, uint64_t info_length)
+{
+BDRVDMGState *s = bs->opaque;
+int ret;
+uint8_t *buffer = NULL;
+char *data_begin, *data_end;
+
+/* Have at least some length to avoid NULL for g_malloc. Attempt to set a
+ * safe upper cap on the data length. A test sample had a XML length of
+ * about 1 MiB. */
+if (info_length == 0 || info_length > 16 * 1024 * 1024) {
+ret = -EINVAL;
+goto fail;
+}
+
+buffer = g_malloc(info_length + 1);
+buffer[info_length] = '\0';
+ret = bdrv_pread(bs->file, info_begin, buffer, info_length);
+if (ret != info_length) {
+ret = -EINVAL;
+goto fail;
+}
+
+/* look for The data is 284 (0x11c) bytes after base64
+ * decode. The actual data element has 431 (0x1af) bytes which includes
tabs
+ * and line feeds. */
+data_end = (char *)buffer;
+while ((data_begin = strstr(data_end, "")) != NULL) {
+gsize out_len = 0;
+
+data_begin += 6;
+data_end = strstr(data_begin, "");
+/* malformed XML? */
+if (data_end == NULL) {
+ret = -EINVAL;
+goto fail;
+}
+*data_end++ = '\0';
+g_base64_decode_inplace(data_begin, &out_len);
+ret = dmg_read_mish_block(s, ds, (uint8_t *)data_begin,
+ (uint32_t)out_len);
+if (ret < 0) {
+goto fail;
+}
+}
+ret = 0;
+
+fail:
+g_free(buffer);
+return ret;
+}
+
static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
BDRVDMGState *s = bs->opaque;
DmgHeaderState ds;
uint64_t rsrc_fork_offset, rsrc_fork_length;
+uint64_t plist_xml_offset, plist_xml_length;
int64_t offset;
int ret;
@@ -366,12 +421,26 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
if (ret < 0) {
goto fail;
}
+/* offset of property list (XMLOffset) */
+ret = read_uint64(bs, offset + 0xd8, &plist_xml_offset);
+if (ret < 0) {
+goto fail;
+}
+ret = read_uint64(bs, offset + 0xe0, &plist_xml_length);
+if (ret < 0) {
+goto fail;
+}
if (rsrc_fork_offset != 0 && rsrc_fork_length != 0) {
ret = dmg_read_resource_fork(bs, &ds,
rsrc_fork_offset, rsrc_fork_length);
if (ret < 0) {
goto fail;
}
+} else if (plist_xml_offset != 0 && plist_xml_length != 0) {
+ret = dmg_read_plist_xml(bs, &ds, plist_xml_offset, plist_xml_length);
+if (ret < 0) {
+goto fail;
+}
} else {
ret = -EINVAL;
goto fail;
--
2.2.1
[Qemu-devel] [PATCH 07/10] block/dmg: set virtual size to a non-zero value
Right now the virtual size is always reported as zero which makes it
impossible to convert between formats.
After this patch, the number of sectors will be read from the BLXX
("mish") header.
To verify the behavior, the output of `dmg2img foo.dmg foo.img` was
compared against `qemu-img convert -f dmg -O raw foo.dmg foo.raw`. The
tests showed that the file contents are exactly the same, except that
QEMU creates a slightly larger file (it matches the total sectors
count).
Signed-off-by: Peter Wu
---
block/dmg.c | 8
1 file changed, 8 insertions(+)
diff --git a/block/dmg.c b/block/dmg.c
index c03ea01..984997f 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -430,6 +430,14 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
if (ret < 0) {
goto fail;
}
+ret = read_uint64(bs, offset + 0x1ec, (uint64_t *)&bs->total_sectors);
+if (ret < 0) {
+goto fail;
+}
+if (bs->total_sectors < 0) {
+ret = -EINVAL;
+goto fail;
+}
if (rsrc_fork_offset != 0 && rsrc_fork_length != 0) {
ret = dmg_read_resource_fork(bs, &ds,
rsrc_fork_offset, rsrc_fork_length);
--
2.2.1
[Qemu-devel] [PATCH 02/10] block/dmg: extract mish block decoding functionality
Extract the mish block decoder such that this can be used for other
formats in the future. A new DmgHeaderState struct is introduced to
share state while decoding.
The code is kept unchanged as much as possible, a "fail" label is added
for example where a simple return would probably do.
Signed-off-by: Peter Wu
---
block/dmg.c | 216 +++-
1 file changed, 125 insertions(+), 91 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index df274f9..6dc6dbb 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -164,19 +164,137 @@ static int64_t dmg_find_koly_offset(BlockDriverState
*file_bs)
return -EINVAL;
}
+/* used when building the sector table */
+typedef struct DmgHeaderState {
+/* used internally by dmg_read_mish_block to remember offsets of blocks
+ * across calls */
+uint64_t last_in_offset;
+uint64_t last_out_offset;
+/* exported for dmg_open */
+uint32_t max_compressed_size;
+uint32_t max_sectors_per_chunk;
+} DmgHeaderState;
+
+static int dmg_read_mish_block(BlockDriverState *bs, DmgHeaderState *ds,
+ int64_t offset, uint32_t count)
+{
+BDRVDMGState *s = bs->opaque;
+uint32_t type, i;
+int ret;
+size_t new_size;
+uint32_t chunk_count;
+
+ret = read_uint32(bs, offset, &type);
+if (ret < 0) {
+goto fail;
+}
+
+/* skip data that is not a valid MISH block (invalid magic or too small) */
+if (type != 0x6d697368 || count < 244) {
+/* assume success for now */
+return 0;
+}
+
+offset += 4;
+offset += 200;
+
+chunk_count = (count - 204) / 40;
+new_size = sizeof(uint64_t) * (s->n_chunks + chunk_count);
+s->types = g_realloc(s->types, new_size / 2);
+s->offsets = g_realloc(s->offsets, new_size);
+s->lengths = g_realloc(s->lengths, new_size);
+s->sectors = g_realloc(s->sectors, new_size);
+s->sectorcounts = g_realloc(s->sectorcounts, new_size);
+
+for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) {
+ret = read_uint32(bs, offset, &s->types[i]);
+if (ret < 0) {
+goto fail;
+}
+offset += 4;
+if (s->types[i] != 0x8005 && s->types[i] != 1 &&
+s->types[i] != 2) {
+if (s->types[i] == 0x && i > 0) {
+ds->last_in_offset = s->offsets[i - 1] + s->lengths[i - 1];
+ds->last_out_offset = s->sectors[i - 1] +
+ s->sectorcounts[i - 1];
+}
+chunk_count--;
+i--;
+offset += 36;
+continue;
+}
+offset += 4;
+
+ret = read_uint64(bs, offset, &s->sectors[i]);
+if (ret < 0) {
+goto fail;
+}
+s->sectors[i] += ds->last_out_offset;
+offset += 8;
+
+ret = read_uint64(bs, offset, &s->sectorcounts[i]);
+if (ret < 0) {
+goto fail;
+}
+offset += 8;
+
+if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
+error_report("sector count %" PRIu64 " for chunk %" PRIu32
+ " is larger than max (%u)",
+ s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
+ret = -EINVAL;
+goto fail;
+}
+
+ret = read_uint64(bs, offset, &s->offsets[i]);
+if (ret < 0) {
+goto fail;
+}
+s->offsets[i] += ds->last_in_offset;
+offset += 8;
+
+ret = read_uint64(bs, offset, &s->lengths[i]);
+if (ret < 0) {
+goto fail;
+}
+offset += 8;
+
+if (s->lengths[i] > DMG_LENGTHS_MAX) {
+error_report("length %" PRIu64 " for chunk %" PRIu32
+ " is larger than max (%u)",
+ s->lengths[i], i, DMG_LENGTHS_MAX);
+ret = -EINVAL;
+goto fail;
+}
+
+update_max_chunk_size(s, i, &ds->max_compressed_size,
+ &ds->max_sectors_per_chunk);
+}
+s->n_chunks += chunk_count;
+return 0;
+
+fail:
+return ret;
+}
+
static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
BDRVDMGState *s = bs->opaque;
-uint64_t info_begin, info_end, last_in_offset, last_out_offset;
+DmgHeaderState ds;
+uint64_t info_begin, info_end;
uint32_t count, tmp;
-uint32_t max_compressed_size = 1, max_sectors_per_chunk = 1, i;
int64_t offset;
int ret;
bs->read_only = 1;
s->n_chunks = 0;
s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL
[Qemu-devel] [PATCH 03/10] block/dmg: extract processing of resource forks
Besides the offset, also read the resource length. This length is now
used in the extracted function to verify the end of the resource fork
against "count" from the resource fork.
Signed-off-by: Peter Wu
---
block/dmg.c | 90 -
1 file changed, 59 insertions(+), 31 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 6dc6dbb..7f49388 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -278,38 +278,13 @@ fail:
return ret;
}
-static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
-Error **errp)
+static int dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
+ uint64_t info_begin, uint64_t info_length)
{
-BDRVDMGState *s = bs->opaque;
-DmgHeaderState ds;
-uint64_t info_begin, info_end;
-uint32_t count, tmp;
-int64_t offset;
int ret;
-
-bs->read_only = 1;
-s->n_chunks = 0;
-s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
-ds.last_in_offset = 0;
-ds.last_out_offset = 0;
-ds.max_compressed_size = 1;
-ds.max_sectors_per_chunk = 1;
-
-/* locate the UDIF trailer */
-offset = dmg_find_koly_offset(bs->file);
-if (offset < 0) {
-ret = offset;
-goto fail;
-}
-
-ret = read_uint64(bs, offset + 0x28, &info_begin);
-if (ret < 0) {
-goto fail;
-} else if (info_begin == 0) {
-ret = -EINVAL;
-goto fail;
-}
+uint32_t count, tmp;
+uint64_t info_end;
+uint64_t offset;
ret = read_uint32(bs, info_begin, &tmp);
if (ret < 0) {
@@ -326,6 +301,10 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
ret = -EINVAL;
goto fail;
}
+if (count > info_length) {
+ret = -EINVAL;
+goto fail;
+}
info_end = info_begin + count;
/* begin of mish block */
@@ -342,12 +321,61 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
}
offset += 4;
-ret = dmg_read_mish_block(bs, &ds, offset, count);
+ret = dmg_read_mish_block(bs, ds, offset, count);
if (ret < 0) {
goto fail;
}
offset += count;
}
+return 0;
+
+fail:
+return ret;
+}
+
+static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
+Error **errp)
+{
+BDRVDMGState *s = bs->opaque;
+DmgHeaderState ds;
+uint64_t rsrc_fork_offset, rsrc_fork_length;
+int64_t offset;
+int ret;
+
+bs->read_only = 1;
+s->n_chunks = 0;
+s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
+ds.last_in_offset = 0;
+ds.last_out_offset = 0;
+ds.max_compressed_size = 1;
+ds.max_sectors_per_chunk = 1;
+
+/* locate the UDIF trailer */
+offset = dmg_find_koly_offset(bs->file);
+if (offset < 0) {
+ret = offset;
+goto fail;
+}
+
+/* offset of resource fork (RsrcForkOffset) */
+ret = read_uint64(bs, offset + 0x28, &rsrc_fork_offset);
+if (ret < 0) {
+goto fail;
+}
+ret = read_uint64(bs, offset + 0x30, &rsrc_fork_length);
+if (ret < 0) {
+goto fail;
+}
+if (rsrc_fork_offset != 0 && rsrc_fork_length != 0) {
+ret = dmg_read_resource_fork(bs, &ds,
+ rsrc_fork_offset, rsrc_fork_length);
+if (ret < 0) {
+goto fail;
+}
+} else {
+ret = -EINVAL;
+goto fail;
+}
/* initialize zlib engine */
s->compressed_chunk = qemu_try_blockalign(bs->file,
--
2.2.1
[Qemu-devel] [PATCH 01/10] block/dmg: properly detect the UDIF trailer
DMG files have a variable length with a UDIF trailer at the end of a
file. This UDIF trailer is essential as it describes the contents of
the image. At the moment however, the start of this trailer is almost
always incorrect as bdrv_getlength() returns a multiple of the block
size (rounded up). This results in a failure to recognize DMG files,
resulting in Invalid argument (EINVAL) errors.
As there is no API to retrieve the real file size, look for the magic
header in the last two sectors to find the start of this 512-byte UDIF
trailer (the "koly" block).
The resource fork offset ("info_begin") has its offset adjusted as the
initial value of offset does not mean "end of file" anymore, but "begin
of UDIF trailer".
Signed-off-by: Peter Wu
---
block/dmg.c | 40
1 file changed, 36 insertions(+), 4 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index e455886..df274f9 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -131,6 +131,39 @@ static void update_max_chunk_size(BDRVDMGState *s,
uint32_t chunk,
}
}
+static int64_t dmg_find_koly_offset(BlockDriverState *file_bs)
+{
+int64_t length;
+int64_t offset = 0;
+uint8_t buffer[515];
+int i, ret;
+
+/* bdrv_getlength returns a multiple of block size (512), rounded up. Since
+ * dmg images can have odd sizes, try to look for the "koly" magic which
+ * marks the begin of the UDIF trailer (512 bytes). This magic can be found
+ * in the last 511 bytes of the second-last sector or the first 4 bytes of
+ * the last sector (search space: 515 bytes) */
+length = bdrv_getlength(file_bs);
+if (length < 512) {
+return length < 0 ? length : -EINVAL;
+}
+if (length > 511 + 512) {
+offset = length - 511 - 512;
+}
+length = length < 515 ? length : 515;
+ret = bdrv_pread(file_bs, offset, buffer, length);
+if (ret < 4) {
+return ret < 0 ? ret : -EINVAL;
+}
+for (i = 0; i < length - 3; i++) {
+if (buffer[i] == 'k' && buffer[i+1] == 'o' &&
+buffer[i+2] == 'l' && buffer[i+3] == 'y') {
+return offset + i;
+}
+}
+return -EINVAL;
+}
+
static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -145,15 +178,14 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
s->n_chunks = 0;
s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
-/* read offset of info blocks */
-offset = bdrv_getlength(bs->file);
+/* locate the UDIF trailer */
+offset = dmg_find_koly_offset(bs->file);
if (offset < 0) {
ret = offset;
goto fail;
}
-offset -= 0x1d8;
-ret = read_uint64(bs, offset, &info_begin);
+ret = read_uint64(bs, offset + 0x28, &info_begin);
if (ret < 0) {
goto fail;
} else if (info_begin == 0) {
--
2.2.1
[Qemu-devel] [PATCH 08/10] block/dmg: fix sector data offset calculation
This patch addresses two issues:
- The data fork offset was not taken into account, resulting in failure
to read an InstallESD.dmg file (5164763151 bytes) which had a
non-zero DataForkOffset field.
- The offset of the previous block ("partition") was unconditionally
added to the current block because older files would start the input
offset of a new block at zero. Newer files (including vlc-2.1.5.dmg,
tuxpaint-0.9.15-macosx.dmg and OS X Yosemite [MAS].dmg) failed in
reads because these files have a continuous input offset.
Signed-off-by: Peter Wu
---
block/dmg.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/block/dmg.c b/block/dmg.c
index 984997f..93b597f 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -179,6 +179,7 @@ static int64_t dmg_find_koly_offset(BlockDriverState
*file_bs)
typedef struct DmgHeaderState {
/* used internally by dmg_read_mish_block to remember offsets of blocks
* across calls */
+uint64_t data_fork_offset;
uint64_t last_in_offset;
uint64_t last_out_offset;
/* exported for dmg_open */
@@ -194,6 +195,7 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
size_t new_size;
uint32_t chunk_count;
int64_t offset = 0;
+uint64_t in_offset = ds->data_fork_offset;
type = buff_read_uint32(buffer, offset);
/* skip data that is not a valid MISH block (invalid magic or too small) */
@@ -246,7 +248,12 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
}
s->offsets[i] = buff_read_uint64(buffer, offset);
-s->offsets[i] += ds->last_in_offset;
+/* If this offset is below the previous chunk end, then assume that all
+ * following offsets are after the previous chunks. */
+if (s->offsets[i] + in_offset < ds->last_in_offset) {
+in_offset = ds->last_in_offset;
+}
+s->offsets[i] += in_offset;
offset += 8;
s->lengths[i] = buff_read_uint64(buffer, offset);
@@ -400,6 +407,7 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
bs->read_only = 1;
s->n_chunks = 0;
s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
+ds.data_fork_offset = 0;
ds.last_in_offset = 0;
ds.last_out_offset = 0;
ds.max_compressed_size = 1;
@@ -412,6 +420,12 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
int flags,
goto fail;
}
+/* offset of data fork (DataForkOffset) */
+ret = read_uint64(bs, offset + 0x18, &ds.data_fork_offset);
+if (ret < 0) {
+goto fail;
+}
+
/* offset of resource fork (RsrcForkOffset) */
ret = read_uint64(bs, offset + 0x28, &rsrc_fork_offset);
if (ret < 0) {
--
2.2.1
[Qemu-devel] [PATCH 05/10] block/dmg: validate chunk size to avoid overflow
Previously the chunk size was not checked, allowing for a large memory
allocation. This patch checks whether the chunks size is within the
resource fork length.
Signed-off-by: Peter Wu
---
block/dmg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/dmg.c b/block/dmg.c
index 75e771a..19e4fe2 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -308,7 +308,7 @@ static int dmg_read_resource_fork(BlockDriverState *bs,
DmgHeaderState *ds,
ret = read_uint32(bs, offset, &count);
if (ret < 0) {
goto fail;
-} else if (count == 0) {
+} else if (count == 0 || count > info_end - offset) {
ret = -EINVAL;
goto fail;
}
--
2.2.1
[Qemu-devel] [PATCH 00/10] block/dmg: (compatibility) fixes and bzip2 support
Hi,
These series improve QEMU support for DMG image files:
- Images which are not multiples of a block size (512) are now properly
recognized ("properly detect the UDIF trailer").
- Block descriptors stored in the XML plist section rather than the Resource
Fork are now recognized ("process XML plists").
- The virtual (uncompressed) image size is now advertised ("set virtual size to
a non-zero value").
- Files which have a different data offsets are now properly handled ("fix
sector data offset calculation").
- bzip2-compressed chunks can now be read ("support bzip2 block entry types").
This adds a new --enable-bzip2 option.
- Files with a large all-zeroes data chunk do not trip an error anymore
("improve zeroes handling").
The XML plist patch depends on these refactorings (which add no new
functionality):
block/dmg: extract mish block decoding functionality
block/dmg: extract processing of resource forks
block/dmg: process a buffer instead of reading ints
Finally there is this patch which avoids a DoS (large memory allocation) due to
a missing check on untrusted data:
block/dmg: validate chunk size to avoid overflow
These patches were tested against disk images, but since DMG files are also used
for installers, I used those too. The following files pass these patches:
- filename + source; real size in bytes; publication date. (additional info:
whether BLXX is stored in XML plists or a resource fork; compression method)
- espgs-7.05.5-0.ppc.dmg[1]5593786 2002-09 (rsrc fork, zlib, sector
offsets start at 0)
- Wireshark 1.12.2 Intel 64.dmg[2]26375047 2014-11 (xml, zlib)
- tuxpaint-0.9.15-macosx.dmg[3]9022458 2005-11 (rsrc fork, zlib)
- vlc-2.1.5.dmg[4]33519849 2014-07 (xml, bzlib)
- OS X Yosemite.dmg 5189100314 2014-10 (xml, bzlib)
- InstallESD.dmg (in prev. image) 5164763151 2014-10 (xml, zlib; data offset
is non-zero)
For these above files, I executed `qemu-img info foo.dmg` to verify that there
is
no EINVAL error and `qemu-img convert -f dmg -O raw foo.dmg foo.raw` to check
whether reading works. The resulting `foo.raw` was then compared against
`foo.img` as generated by `dmg2img foo.dmg`.
This file was tested and 'qemu-img info' works properly, but conversion failed
because the Apple Data Compression (ADC) format is not supported[5].
- NetBoot9.dmg[6]534884900 2003-09 (rsrc fork, adc)
These patches (rebased against current master) can be found at:
https://git.lekensteyn.nl/peter/qemu/log/?h=block-dmg-2.3
(development and testing was done against v2.2.0, branch block-dmg-2.2)
Kind regards,
Peter
[1]: http://sourceforge.net/projects/espgs/files/espgs/7.05.5/
[2]: https://www.wireshark.org/download/osx/
[3]: http://sourceforge.net/projects/tuxpaint/files/tuxpaint/0.9.15/
[4]: http://download.videolan.org/pub/videolan/vlc/2.1.5/macosx/
[5]: https://bugzilla.redhat.com/show_bug.cgi?id=1058132
[6]:
http://download.info.apple.com/Mac_OS_X/693-4445.20030912.gnr39/NetBoot9.dmg
--
Peter Wu (10):
block/dmg: properly detect the UDIF trailer
block/dmg: extract mish block decoding functionality
block/dmg: extract processing of resource forks
block/dmg: process a buffer instead of reading ints
block/dmg: validate chunk size to avoid overflow
block/dmg: process XML plists
block/dmg: set virtual size to a non-zero value
block/dmg: fix sector data offset calculation
block/dmg: support bzip2 block entry types
block/dmg: improve zeroes handling
block/dmg.c | 475 +---
configure | 31
2 files changed, 390 insertions(+), 116 deletions(-)
--
2.2.1
[Qemu-devel] [PATCH 04/10] block/dmg: process a buffer instead of reading ints
As the decoded plist XML is not a pointer in the file,
dmg_read_mish_block must be able to process a buffer instead of a file
pointer. Since the full buffer must be processed, let's change the
return value again to just a success flag.
Signed-off-by: Peter Wu
---
block/dmg.c | 60 ++--
1 file changed, 30 insertions(+), 30 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 7f49388..75e771a 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -100,6 +100,16 @@ static int read_uint32(BlockDriverState *bs, int64_t
offset, uint32_t *result)
return 0;
}
+static inline uint64_t buff_read_uint64(const uint8_t *buffer, int64_t offset)
+{
+return be64_to_cpu(*(uint64_t *)&buffer[offset]);
+}
+
+static inline uint32_t buff_read_uint32(const uint8_t *buffer, int64_t offset)
+{
+return be32_to_cpu(*(uint32_t *)&buffer[offset]);
+}
+
/* Increase max chunk sizes, if necessary. This function is used to calculate
* the buffer sizes needed for compressed/uncompressed chunk I/O.
*/
@@ -175,20 +185,16 @@ typedef struct DmgHeaderState {
uint32_t max_sectors_per_chunk;
} DmgHeaderState;
-static int dmg_read_mish_block(BlockDriverState *bs, DmgHeaderState *ds,
- int64_t offset, uint32_t count)
+static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
+ uint8_t *buffer, uint32_t count)
{
-BDRVDMGState *s = bs->opaque;
uint32_t type, i;
int ret;
size_t new_size;
uint32_t chunk_count;
+int64_t offset = 0;
-ret = read_uint32(bs, offset, &type);
-if (ret < 0) {
-goto fail;
-}
-
+type = buff_read_uint32(buffer, offset);
/* skip data that is not a valid MISH block (invalid magic or too small) */
if (type != 0x6d697368 || count < 244) {
/* assume success for now */
@@ -207,10 +213,7 @@ static int dmg_read_mish_block(BlockDriverState *bs,
DmgHeaderState *ds,
s->sectorcounts = g_realloc(s->sectorcounts, new_size);
for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) {
-ret = read_uint32(bs, offset, &s->types[i]);
-if (ret < 0) {
-goto fail;
-}
+s->types[i] = buff_read_uint32(buffer, offset);
offset += 4;
if (s->types[i] != 0x8005 && s->types[i] != 1 &&
s->types[i] != 2) {
@@ -226,17 +229,11 @@ static int dmg_read_mish_block(BlockDriverState *bs,
DmgHeaderState *ds,
}
offset += 4;
-ret = read_uint64(bs, offset, &s->sectors[i]);
-if (ret < 0) {
-goto fail;
-}
+s->sectors[i] = buff_read_uint64(buffer, offset);
s->sectors[i] += ds->last_out_offset;
offset += 8;
-ret = read_uint64(bs, offset, &s->sectorcounts[i]);
-if (ret < 0) {
-goto fail;
-}
+s->sectorcounts[i] = buff_read_uint64(buffer, offset);
offset += 8;
if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
@@ -247,17 +244,11 @@ static int dmg_read_mish_block(BlockDriverState *bs,
DmgHeaderState *ds,
goto fail;
}
-ret = read_uint64(bs, offset, &s->offsets[i]);
-if (ret < 0) {
-goto fail;
-}
+s->offsets[i] = buff_read_uint64(buffer, offset);
s->offsets[i] += ds->last_in_offset;
offset += 8;
-ret = read_uint64(bs, offset, &s->lengths[i]);
-if (ret < 0) {
-goto fail;
-}
+s->lengths[i] = buff_read_uint64(buffer, offset);
offset += 8;
if (s->lengths[i] > DMG_LENGTHS_MAX) {
@@ -281,8 +272,10 @@ fail:
static int dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
uint64_t info_begin, uint64_t info_length)
{
+BDRVDMGState *s = bs->opaque;
int ret;
uint32_t count, tmp;
+uint8_t *buffer = NULL;
uint64_t info_end;
uint64_t offset;
@@ -321,15 +314,22 @@ static int dmg_read_resource_fork(BlockDriverState *bs,
DmgHeaderState *ds,
}
offset += 4;
-ret = dmg_read_mish_block(bs, ds, offset, count);
+buffer = g_realloc(buffer, count);
+ret = bdrv_pread(bs->file, offset, buffer, count);
+if (ret < 0) {
+goto fail;
+}
+
+ret = dmg_read_mish_block(s, ds, buffer, count);
if (ret < 0) {
goto fail;
}
offset += count;
}
-return 0;
+ret = 0;
fail:
+g_free(buffer);
return ret;
}
--
2.2.1
[Qemu-devel] [PATCH] block/iscsi: fix uninitialized variable
'ret' was never initialized in the success path.
Signed-off-by: Peter Wu
---
Hi,
Found this warning when compiling with --enable-debug. This issue was previously
fixed as part of Peter Lieven's patch series[1] but it never seemed to get
merged.
Kinds regards,
Peter
[1]: https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg04279.html
---
block/iscsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index ed375fc..12ddbfb 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1286,7 +1286,7 @@ static int iscsi_open(BlockDriverState *bs, QDict
*options, int flags,
QemuOpts *opts;
Error *local_err = NULL;
const char *filename;
-int i, ret;
+int i, ret = 0;
if ((BDRV_SECTOR_SIZE % 512) != 0) {
error_setg(errp, "iSCSI: Invalid BDRV_SECTOR_SIZE. "
--
2.2.1
Re: [Qemu-devel] Improved shared folders support
On Tuesday 04 November 2014 09:13:31 Michael Tokarev wrote: > >> The whole thing needs to be rewritten to use a shell script to create > >> smb.conf and to call smbd. It's something I wanted to do for very long > >> time, the only question is where to put this script so a user can > >> easily customize it. In debian we have a rule that anything in /etc > >> is a conffile, and it gets upgraded (when upgrading the corresponding > >> package) only if it hasn't been modified by the user. Everything else > >> (in /usr/lib, /usr/share etc) gets upgraded unconditionally. So maybe > >> storing it in /etc/qemu/run-smbd.sh or somehting like that makes sense. > > > > I do not think that everyone is (or wants to be) a Samba wizard. For > > that reason, I see little value in giving the user too much control over > > the creation of the samba config file. There exists helpers for setting up > > bridges, but that is because it may need more privileges, and starting > > samba is > > not such a case. > > The talk is not about being a wizard or relying on users creating that > script. The talk is about letting user a quick way to fix stuff with > new or unusual samba combination, without a need to recompile qemu > binary. Changing a shell script is trivial, including trial and error > (so you dont really need to be a wizard), while recompiling is much > more difficult. So this is more to workaround bugs in Samba should they occur? I previously mentioned a possible smbd=/path/to/smbd option, but it could also become a smbhelper=/path/to/helper.sh. Possible options: - Generate the smb.conf and pass the temp dir containing it to that helper. smbd is started by the helper. - Do not create smb.conf nor create a tempdir, let the helper control this. smbd is started by the helper. - Same as previous, but allow smb and smbhelper to co-exist and pass the smb option via an envvar to smbhelper. > > That single program could be a wrapper script that modifies the files as > > needed. > > > > I propose to keep a simple interface which do not need users to modify a > > shell script or learn Samba. Consider Virtual Box' and VMWare' shared > > folders functionality. It allows multiple folders to be specified with > > these properties: > > Again, my variant does not mean users will need to learn anything, it > is only needed in emergency cases. My proposal is to move all current > smb.conf file creation from qemu binary to an external shell script > which is *shipped by qemu*, and which creates the same setup as currently > done by qemu binary. But with it being a shell script, it will be easy > to modify it *if needed*. If Samba options have be to be modified in this way, then that is a bug in QEMU and must be fixed (consider the Samba config an implementation detail which does not need to be exposed to the user in one way or another). I would like to have the ability to let QEMU generate the config file *without* having to rely on an external script with a hard-coded path. Consider out-of-tree builds and testing such binaries. If a helper is wanted, see above for a possible smbhelper. > Modifying already created smb.conf is not a good idea, that requires > even more wizardy skills. For manual *testing* you can actually modify the smb.conf between invocations of smbd (that is, before the guest accesses port 139 or 445). I actually did this to add "log level". In the proposed extension of the smb option I mentioned rewriting smb.conf on config changes, but that would then be done by QEMU, not the user. Btw, what do you think about the proposed smb extension to allow more shares with custom names? -- Kind regards, Peter https://lekensteyn.nl
Re: [Qemu-devel] slirp-smb broken with Samba 4.1
On Monday 03 November 2014 13:22:19 Peter Wu wrote: > On Friday 24 October 2014 18:55:07 Jan Kiszka wrote: > > writing to you as you provided a fix for the last related issue: > > > > I just noticed that the samba-based share is broken again with smbd > > version 4.1.11. Tried to look briefly at it, realized that it is a > > permission thing (different error when qemu runs as root) but also some > > more nasty problem with configuring r/w guest access to a share. If you > > have experience in that area, maybe you have an idea. > > It appears that the issue is solved in Samba 4.2 (tested with > samba-4.2.0rc1-388-ga3b333a). Using that samba version fixes the > problem, I'll try to do a bisect to see where it got fixed in Samba. Unfortunately I was wrong. I have tested it by forwarding a smbd daemon which listens on a port, not one that runs in inetd mode. The referenced version was still broken. The earliest version I tested in the Samba 4 series is samba-4.0.0alpha17 and that is still broken. I have reported the issue here, it turns out to affect instances running as non-root user: https://bugzilla.samba.org/show_bug.cgi?id=10919 -- Kind regards, Peter https://lekensteyn.nl
[Qemu-devel] Improved shared folders support (was: Re: slirp-smb broken with Samba 4.1)
On Monday 03 November 2014 15:58:35 Michael Tokarev wrote: > 03.11.2014 15:22, Peter Wu wrote: > [] > > As an aside, would it be possible to override the samba binary at runtime, > > without compiling? Either an envvar (SMBD) or an option (-net > > user,smb=...,smbd=...). > > The whole thing needs to be rewritten to use a shell script to create > smb.conf and to call smbd. It's something I wanted to do for very long > time, the only question is where to put this script so a user can > easily customize it. In debian we have a rule that anything in /etc > is a conffile, and it gets upgraded (when upgrading the corresponding > package) only if it hasn't been modified by the user. Everything else > (in /usr/lib, /usr/share etc) gets upgraded unconditionally. So maybe > storing it in /etc/qemu/run-smbd.sh or somehting like that makes sense. I do not think that everyone is (or wants to be) a Samba wizard. For that reason, I see little value in giving the user too much control over the creation of the samba config file. There exists helpers for setting up bridges, but that is because it may need more privileges, and starting samba is not such a case. > The rest is a shell code to create smb.conf in a temp dir -- the same > as current code does in qemu, plus all your additions, If by "my additions" you mean specifying a custom samba path, that is just a single program, why not allow to override it at runtime? > plus whatever else might be needed (that script can even query which > samba version is in use and enable one or another option). That single program could be a wrapper script that modifies the files as needed. I propose to keep a simple interface which do not need users to modify a shell script or learn Samba. Consider Virtual Box' and VMWare' shared folders functionality. It allows multiple folders to be specified with these properties: - share name, - folder to be shared, - read-only, - automount it in the guest (feature of VBox Guest additions), - disable after session (after reboot/poweroff, VMWare feature). Only the first three options are interesting enough for QEMU's interface. What about extending the current '-net user,smb' option as follows (lines are wrapped for readability): smb=[sharename:]folder, [smb=sharename2:folder2[:readonly]] (etc) If 'sharename' is omitted, 'qemu' is used. Duplicate share names must result in a fatal error. 'readonly' requires specification of the sharename to avoid an ambiguity. Further share options could be added in the future as needed using ':' as separator (the comma is already parsed by -net). Alternative option (defaults to rw:): smb=[rw:|ro:][sharename:]folder, smb=[rw:|ro:][sharename2:]folder2 (etc) It would also be nice if a QMP/HMP was also available to add shares on runtime. In the current implementation, that could be done by writing a new config file, renaming it to smb.conf when done. Samba appears to have support to do this without restarting samba, but I have no idea whether it can be used in the current implementation where communication happens through stdin/stdout. By the way, how well does '-net user,smb' work with multiple connections? The documentation for guestfwd suggests that the command gets executed for each connection. Won't that cause concurrency issues in the access to databases? -- Kind regards, Peter https://lekensteyn.nl
Re: [Qemu-devel] [Qemu-trivial] [PATCH] slirp/smbd: disable printer in smb config
On Monday 03 November 2014 17:15:24 Michael Tokarev wrote: > BTW, I'm not sure `socket address' paraameter is relevant in this context > at all, -- smbd should not use it in inetd mode. It'd be interesting to > know why this option is here to start with, and whenever we really need > the new interfaces/bind-interfacs-only replacement. The socket option is unused when QEMU invokes the command directly. The only (weak) reason why it is still there is to ease testing, such that you can simply use: smbd -s smb.conf -p 1337 Without the socket option, I am afraid that you will accidentally expose the very permissive share to the network. So either append a comment explaining the above or just remove it. I am fine with removing the interfaces option from this patch (or in a future patch if you prefer that). -- Kind regards, Peter https://lekensteyn.nl
Re: [Qemu-devel] slirp-smb broken with Samba 4.1
Hi, On Friday 24 October 2014 18:55:07 Jan Kiszka wrote: > writing to you as you provided a fix for the last related issue: > > I just noticed that the samba-based share is broken again with smbd > version 4.1.11. Tried to look briefly at it, realized that it is a > permission thing (different error when qemu runs as root) but also some > more nasty problem with configuring r/w guest access to a share. If you > have experience in that area, maybe you have an idea. It appears that the issue is solved in Samba 4.2 (tested with samba-4.2.0rc1-388-ga3b333a). Using that samba version fixes the problem, I'll try to do a bisect to see where it got fixed in Samba. As an aside, would it be possible to override the samba binary at runtime, without compiling? Either an envvar (SMBD) or an option (-net user,smb=...,smbd=...). -- Kind regards, Peter https://lekensteyn.nl
[Qemu-devel] [PATCH] slirp/smbd: disable printer in smb config
The file sharing module should not handle printers, so disable it.
The options 'load printers' and 'printing' have been available since the
beginning (May 1996, commit 0e8fd3398771da2f016d72830179507f3edda51b).
Option 'disable spoolss' is available since Samba 2.0.4, commit
de5f42c9d9172592779fa2504d44544e3b6b1c0d).
Next, "socket address" was reported as deprecated, use a combination of
"interfaces" and "bind interfaces only" instead (available since October
1997, commit 79f4fb52c1ed56fd843f81b4eb0cdd2991d4d0f4).
Override cache directory to avoid writing to a global directory. Option
available since Samba 3.4.0, Jan 2009, commit
19a05bf2f485023b11b41dfae3f6459847d55ef7.
Set "usershare max shared=0" to prevent a global directory from being
used. Option available since Samba 3.0.23, February 2006, commit
5831715049f2d460ce42299963a5defdc160891b.
The most recently option was introduced with Samba 3.4.0, but previously
"state directory" was already added which exists in Samba 3.4.0. As
unknown parameters are ignored (while printing a warning), it should be
safe to add another option.
Signed-off-by: Peter Wu
---
Hi,
While trying to share a folder with a guest, I noticed that the option -net
user,smb=... would time out in the guest due to an incompatibility with Samba 4
(see also mailing list message "slirp-smb broken with Samba 4.1" from Jan Kiska
and https://bugs.debian.org/747636). FYI, the bug is fixed in newer Samba
(tested with samba-4.2.0rc1-388-ga3b333a).
While trying to fix that, I found that Samba would try to communicate with CUPS.
This patch disables that fixes some other paths as well. Looking through the
smb.conf manual for "{prefix}", it seems that no other directory is forgotten
now.
As the inetd mode is broken, I work around by starting smbd with the generated
config:
smbd -s smb.conf -p 1337
Then I forward the ports to the guest with (newline inserted for readability):
-user net,
guestfwd=tcp:0.0.0.0:139-cmd:'nc 127.0.0.1 1337',
guestfwd=tcp:0.0.0.0:445-cmd:'nc 127.0.0.1 1337'
This "works" but is certainly not optimal.
Kind regards,
Peter
---
net/slirp.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/net/slirp.c b/net/slirp.c
index c171119..bad427b 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -523,15 +523,21 @@ static int slirp_smb(SlirpState* s, const char
*exported_dir,
fprintf(f,
"[global]\n"
"private dir=%s\n"
-"socket address=127.0.0.1\n"
+"interfaces=127.0.0.1\n"
+"bind interfaces only=yes\n"
"pid directory=%s\n"
"lock directory=%s\n"
"state directory=%s\n"
+"cache directory=%s\n"
"ncalrpc dir=%s/ncalrpc\n"
"log file=%s/log.smbd\n"
"smb passwd file=%s/smbpasswd\n"
"security = user\n"
"map to guest = Bad User\n"
+"load printers = no\n"
+"printing = bsd\n"
+"disable spoolss = yes\n"
+"usershare max shares = 0\n"
"[qemu]\n"
"path=%s\n"
"read only=no\n"
@@ -544,6 +550,7 @@ static int slirp_smb(SlirpState* s, const char
*exported_dir,
s->smb_dir,
s->smb_dir,
s->smb_dir,
+s->smb_dir,
exported_dir,
passwd->pw_name
);
--
2.1.2
[Qemu-devel] [Bug 988125] Re: Generated smb.conf needs to declare state directory
** Changed in: qemu Status: New => Fix Released -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/988125 Title: Generated smb.conf needs to declare state directory Status in QEMU: Fix Released Bug description: The smb.conf generated by the userspace networking does not include a state directory directive. Samba therefore falls back to the default value. Since the user generally does not have write access to this path, smbd immediately crashes. I have attached a patch that adds the missing option. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/988125/+subscriptions
[Qemu-devel] [Bug 1377163] Re: Does not add usb-host devices as they are hotplugged
With some devices, I get a speed mismatch with ehci-usb:
qemu-system-x86_64: Warning: speed mismatch trying to attach usb device "USB
Keyboard" (( speed) to bus "ehci.0", port "1" (high speed)
nec-usb-xhci fixes the keyboard+usb-storage case, but it breaks a webcam:
qemu-system-x86_64: Warning: speed mismatch trying to attach usb device
"Chicony WebCam" (high speed) to bus "usb.0", port "4.3" (full speed)
Anyway, -device usb-host,... binds to only one device. If you disconnect that
device, it will enumerate the next available one and use that. If you only have
one port, then you might have more luck with the 'hostport' option. Example
that adds all available ports (which might not be the best solution though):
qemu-system-x86_64 -usb -device usb-ehci,id=usb $(for i in {1..6};do echo
-device usb-host,bus=usb.0,hostbus=2,hostport=1.$i;done)
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1377163
Title:
Does not add usb-host devices as they are hotplugged
Status in QEMU:
Opinion
Bug description:
A commandline such as
qemu-kvm -device usb-ehci,id=USBCtrl -device host-
usb,bus=USBCtrl.0,hostbus=3
should automatically add all devices on the given bus (here: 3) not
only initially, but also when new devices appear on that bus while
Qemu runs. Currently, all devices on the bus are added initially, but
new devices which are added to the (host) usb while Qemu runs have to
be added manually.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1377163/+subscriptions
[Qemu-devel] [Bug 1376798] Re: Qemu does not hotplug (usb) devices
Not a bug, Linux increases the Device ID each time a new device gets hotplugged. If you need to use this option, pair it with already connected drivers by parsing the output of lsusb for instance. You can still use combinations of hostbus, vendorid, productid, etc. ** Changed in: qemu Status: New => Invalid -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1376798 Title: Qemu does not hotplug (usb) devices Status in QEMU: Invalid Bug description: Qemu passes through all devices on Bus 3 with host-usb all right when issued so on the command line such as ... -device usb-ehci,id=USBCtrl -device host- usb,bus=USBCtrl.0,hostaddr=5 but it does not pass through the devices newly hotplugged to the host after Qemu has already started. One has to explicitly issue device_add host-usb,bus=USBCtrl.0 on the monitor, in order for the guest (Win7 in my case) to see those devices. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1376798/+subscriptions
[Qemu-devel] [Bug 1376938] Re: detect-zeroes=unmap fails to discard in some cases
Note that `cat /dev/zero` followed by `blkdiscard` has no issues. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1376938 Title: detect-zeroes=unmap fails to discard in some cases Status in QEMU: New Bug description: Under certain circumstances, QEMU 2.1.2 fails to discard the underlaying block. The command to start QEMU is: qemu-system-x86_64 -machine pc,accel=kvm -m 2G -smp 2 -vga std -usb -usbdevice tablet -drive if=none,id=ff,aio=native,discard=unmap ,detect-zeroes=unmap,file=/tmp/test.qcow2 -device virtio-scsi-pci -device scsi-disk,drive=ff -cdrom /media/iso/archlinux-2014.06.01-dual.iso -vnc :1 Steps to reproduce: 0. qemu-img create -f qcow2 /tmp/test.qcow2 4G 1. Boot in the guest (Arch Linux x86_64 with Linux 3.14.4) 2. cat /dev/zero > /dev/sda. Observe a file of 4109M (size measured with `du -m` 3. cat /dev/zero > /dev/sda 4. blkdiscard /dev/sda 5. Observe that test.qcow2 grows larger than 1M (13M in my testing). The results are more severe when you write other kind of data in step 2, for instance `base64 /dev/zero > /dev/sda` and then continuing with step 3 and 4 will result in a file of 3642M, after blkdiscard. It makes not much of a difference if I create an ext4 filesystem on top of it and use fstrim (or rm). To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1376938/+subscriptions
[Qemu-devel] [Bug 1376938] [NEW] detect-zeroes=unmap fails to discard in some cases
Public bug reported: Under certain circumstances, QEMU 2.1.2 fails to discard the underlaying block. The command to start QEMU is: qemu-system-x86_64 -machine pc,accel=kvm -m 2G -smp 2 -vga std -usb -usbdevice tablet -drive if=none,id=ff,aio=native,discard=unmap,detect- zeroes=unmap,file=/tmp/test.qcow2 -device virtio-scsi-pci -device scsi- disk,drive=ff -cdrom /media/iso/archlinux-2014.06.01-dual.iso -vnc :1 Steps to reproduce: 0. qemu-img create -f qcow2 /tmp/test.qcow2 4G 1. Boot in the guest (Arch Linux x86_64 with Linux 3.14.4) 2. cat /dev/zero > /dev/sda. Observe a file of 4109M (size measured with `du -m` 3. cat /dev/zero > /dev/sda 4. blkdiscard /dev/sda 5. Observe that test.qcow2 grows larger than 1M (13M in my testing). The results are more severe when you write other kind of data in step 2, for instance `base64 /dev/zero > /dev/sda` and then continuing with step 3 and 4 will result in a file of 3642M, after blkdiscard. It makes not much of a difference if I create an ext4 filesystem on top of it and use fstrim (or rm). ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1376938 Title: detect-zeroes=unmap fails to discard in some cases Status in QEMU: New Bug description: Under certain circumstances, QEMU 2.1.2 fails to discard the underlaying block. The command to start QEMU is: qemu-system-x86_64 -machine pc,accel=kvm -m 2G -smp 2 -vga std -usb -usbdevice tablet -drive if=none,id=ff,aio=native,discard=unmap ,detect-zeroes=unmap,file=/tmp/test.qcow2 -device virtio-scsi-pci -device scsi-disk,drive=ff -cdrom /media/iso/archlinux-2014.06.01-dual.iso -vnc :1 Steps to reproduce: 0. qemu-img create -f qcow2 /tmp/test.qcow2 4G 1. Boot in the guest (Arch Linux x86_64 with Linux 3.14.4) 2. cat /dev/zero > /dev/sda. Observe a file of 4109M (size measured with `du -m` 3. cat /dev/zero > /dev/sda 4. blkdiscard /dev/sda 5. Observe that test.qcow2 grows larger than 1M (13M in my testing). The results are more severe when you write other kind of data in step 2, for instance `base64 /dev/zero > /dev/sda` and then continuing with step 3 and 4 will result in a file of 3642M, after blkdiscard. It makes not much of a difference if I create an ext4 filesystem on top of it and use fstrim (or rm). To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1376938/+subscriptions
Re: [Qemu-devel] [PATCH] qemu-iotests: stop using /tmp directly
On Friday 22 August 2014 20:58:34 Benoît Canet wrote:
> The Friday 22 Aug 2014 Ă 13:25:43 (+0200), Peter Wu wrote :
> > Before this patch you could not run multiple tests concurrently as they
> > might clobber each other test files. This patch solves that by using
> > random temporary directory instead of `/tmp` (for writing output in the
> > individual tests and valgrind logs).
> >
> > Furthermore, this patch stops removing everything in `/tmp/` matching a
> > certain pattern (`/tmp/*.{err,out,time}`). These might not be a property
> > of QEMU.
> >
> > Running multiple concurrent tests in the same object directory is still
> > not supported though as the scratch directory and .bad and .notrun files
> > still interfere with each other. Also not touched is the situation that
> > /tmp/check.log and /tmp/check.sts are hard-coded (and thus unusable in
> > concurrent tests).
> >
> > Signed-off-by: Peter Wu
> > ---
> > Hi,
> >
> > This patch introduces a dependency on mktemp of coreutils. I could still
> > not get
> > concurrent tests to work fully reliably (test 030 failed randomly with QED):
>
> Do we care about the BSDs ?
>
> See the link in the anwser of:
> http://stackoverflow.com/questions/2792675/how-portable-is-mktemp1
>
> --tmpdir seems to be a GNUism.
And `-t` differs between FreeBSD and others too. There is probably nobody who
cares about locations other than /tmp, so what about:
QEMU_IOTESTS_TMPDIR=$(mktemp -d /tmp/qemu-iotests.)
What do you think of the idea of the patch in general?
Kind regards,
Peter
https://lekensteyn.nl
> > FAIL: test_ignore (__main__.TestEIO)
> > --
> > Traceback (most recent call last):
> > File "030", line 223, in test_ignore
> > self.assert_qmp(result, 'return[0]/paused', False)
> > File "/tmp/qemu/tests/qemu-iotests/iotests.py", line 233, in
> > assert_qmp
> > result = self.dictpath(d, path)
> > File "/tmp/qemu/tests/qemu-iotests/iotests.py", line 221, in dictpath
> > self.fail('invalid index "%s" in path "%s" in "%s"' % (idx, path,
> > str(d)))
> > AssertionError: invalid index "0" in path "return[0]/paused" in "[]"
> >
> > I still think that the patches are valuable though, it reduces predictable
> > file
> > names.
> >
> > Kind regards,
> > Peter
> > ---
> > tests/qemu-iotests/001 | 2 +-
> > tests/qemu-iotests/002 | 2 +-
> > tests/qemu-iotests/003 | 2 +-
> > tests/qemu-iotests/004 | 2 +-
> > tests/qemu-iotests/005 | 2 +-
> > tests/qemu-iotests/006 | 2 +-
> > tests/qemu-iotests/007 | 2 +-
> > tests/qemu-iotests/008 | 2 +-
> > tests/qemu-iotests/009 | 2 +-
> > tests/qemu-iotests/010 | 2 +-
> > tests/qemu-iotests/011 | 2 +-
> > tests/qemu-iotests/012 | 2 +-
> > tests/qemu-iotests/013 | 2 +-
> > tests/qemu-iotests/014 | 2 +-
> > tests/qemu-iotests/015 | 2 +-
> > tests/qemu-iotests/016 | 2 +-
> > tests/qemu-iotests/017 | 2 +-
> > tests/qemu-iotests/018 | 2 +-
> > tests/qemu-iotests/019 | 2 +-
> > tests/qemu-iotests/020 | 2 +-
> > tests/qemu-iotests/021 | 2 +-
> > tests/qemu-iotests/022 | 2 +-
> > tests/qemu-iotests/023 | 2 +-
> > tests/qemu-iotests/024 | 2 +-
> > tests/qemu-iotests/025 | 2 +-
> > tests/qemu-iotests/026 | 2 +-
> > tests/qemu-iotests/027 | 2 +-
> > tests/qemu-iotests/028 | 2 +-
> > tests/qemu-iotests/029 | 2 +-
> > tests/qemu-iotests/031 | 2 +-
> > tests/qemu-iotests/032 | 2 +-
> > tests/qemu-iotests/033 | 2 +-
> > tests/qemu-iotests/034 | 2 +-
> > tests/qemu-iotests/035 | 2 +-
> > tests/qemu-iotests/036 | 2 +-
> > tests/qemu-iotests/037 | 2 +-
> > tests/qemu-iotests/038 | 2 +-
> > tests/qemu-iotests/039 | 2 +-
> > tests/qemu-iotests/042 | 2 +-
> > tests/qemu-iotests/043 | 2 +-
> > tests/qemu-iotests/046 | 2 +-
> > tests/qemu-iotests/047 | 2 +-
> > tests/qemu-iotests/049 | 2 +-
> > tests/qemu-iotests/050 | 2 +-
> > tests/qemu-iotests/051 | 2 +-
> > tests/qemu-iotests/052 | 2 +-
> > tests/qemu-iotests/053 | 2 +-
> > test
[Qemu-devel] [PATCH] qemu-iotests: stop using /tmp directly
Before this patch you could not run multiple tests concurrently as they
might clobber each other test files. This patch solves that by using
random temporary directory instead of `/tmp` (for writing output in the
individual tests and valgrind logs).
Furthermore, this patch stops removing everything in `/tmp/` matching a
certain pattern (`/tmp/*.{err,out,time}`). These might not be a property
of QEMU.
Running multiple concurrent tests in the same object directory is still
not supported though as the scratch directory and .bad and .notrun files
still interfere with each other. Also not touched is the situation that
/tmp/check.log and /tmp/check.sts are hard-coded (and thus unusable in
concurrent tests).
Signed-off-by: Peter Wu
---
Hi,
This patch introduces a dependency on mktemp of coreutils. I could still not get
concurrent tests to work fully reliably (test 030 failed randomly with QED):
FAIL: test_ignore (__main__.TestEIO)
--
Traceback (most recent call last):
File "030", line 223, in test_ignore
self.assert_qmp(result, 'return[0]/paused', False)
File "/tmp/qemu/tests/qemu-iotests/iotests.py", line 233, in assert_qmp
result = self.dictpath(d, path)
File "/tmp/qemu/tests/qemu-iotests/iotests.py", line 221, in dictpath
self.fail('invalid index "%s" in path "%s" in "%s"' % (idx, path,
str(d)))
AssertionError: invalid index "0" in path "return[0]/paused" in "[]"
I still think that the patches are valuable though, it reduces predictable file
names.
Kind regards,
Peter
---
tests/qemu-iotests/001 | 2 +-
tests/qemu-iotests/002 | 2 +-
tests/qemu-iotests/003 | 2 +-
tests/qemu-iotests/004 | 2 +-
tests/qemu-iotests/005 | 2 +-
tests/qemu-iotests/006 | 2 +-
tests/qemu-iotests/007 | 2 +-
tests/qemu-iotests/008 | 2 +-
tests/qemu-iotests/009 | 2 +-
tests/qemu-iotests/010 | 2 +-
tests/qemu-iotests/011 | 2 +-
tests/qemu-iotests/012 | 2 +-
tests/qemu-iotests/013 | 2 +-
tests/qemu-iotests/014 | 2 +-
tests/qemu-iotests/015 | 2 +-
tests/qemu-iotests/016 | 2 +-
tests/qemu-iotests/017 | 2 +-
tests/qemu-iotests/018 | 2 +-
tests/qemu-iotests/019 | 2 +-
tests/qemu-iotests/020 | 2 +-
tests/qemu-iotests/021 | 2 +-
tests/qemu-iotests/022 | 2 +-
tests/qemu-iotests/023 | 2 +-
tests/qemu-iotests/024 | 2 +-
tests/qemu-iotests/025 | 2 +-
tests/qemu-iotests/026 | 2 +-
tests/qemu-iotests/027 | 2 +-
tests/qemu-iotests/028 | 2 +-
tests/qemu-iotests/029 | 2 +-
tests/qemu-iotests/031 | 2 +-
tests/qemu-iotests/032 | 2 +-
tests/qemu-iotests/033 | 2 +-
tests/qemu-iotests/034 | 2 +-
tests/qemu-iotests/035 | 2 +-
tests/qemu-iotests/036 | 2 +-
tests/qemu-iotests/037 | 2 +-
tests/qemu-iotests/038 | 2 +-
tests/qemu-iotests/039 | 2 +-
tests/qemu-iotests/042 | 2 +-
tests/qemu-iotests/043 | 2 +-
tests/qemu-iotests/046 | 2 +-
tests/qemu-iotests/047 | 2 +-
tests/qemu-iotests/049 | 2 +-
tests/qemu-iotests/050 | 2 +-
tests/qemu-iotests/051 | 2 +-
tests/qemu-iotests/052 | 2 +-
tests/qemu-iotests/053 | 2 +-
tests/qemu-iotests/054 | 2 +-
tests/qemu-iotests/058 | 2 +-
tests/qemu-iotests/059 | 2 +-
tests/qemu-iotests/060 | 2 +-
tests/qemu-iotests/061 | 2 +-
tests/qemu-iotests/062 | 2 +-
tests/qemu-iotests/063 | 2 +-
tests/qemu-iotests/064 | 2 +-
tests/qemu-iotests/066 | 2 +-
tests/qemu-iotests/067 | 2 +-
tests/qemu-iotests/068 | 2 +-
tests/qemu-iotests/069 | 2 +-
tests/qemu-iotests/070 | 2 +-
tests/qemu-iotests/071 | 2 +-
tests/qemu-iotests/072 | 2 +-
tests/qemu-iotests/073 | 2 +-
tests/qemu-iotests/075 | 2 +-
tests/qemu-iotests/076 | 2 +-
tests/qemu-iotests/077 | 2 +-
tests/qemu-iotests/078 | 2 +-
tests/qemu-iotests/079 | 2 +-
tests/qemu-iotests/080 | 2 +-
tests/qemu-iotests/081 | 2 +-
tests/qemu-iotests/082 | 2 +-
tests/qemu-iotests/083 | 2 +-
tests/qemu-iotests/084 | 2 +-
tests/qemu-iotests/086 | 2 +-
tests/qemu-iotests/087 | 2 +-
tests/qemu-iotests/088 | 2 +-
tests/qemu-iotests/089 | 2 +-
tests/qemu-iotests/090 | 2 +-
tests/qemu-iotests/092 | 2 +-
tests/qemu-iotests/check | 9 ++---
tests/qemu-iotests/common.rc | 7 ---
81 files changed, 89 insertions(+), 85 deletions(-)
diff --git a/tests/qemu-iotests/001 b/tests/qemu-iotests/001
index 4e16469..6472c67 100755
--- a/tests/qemu-iotests/001
+++ b/tests/qemu-iotests/001
@@ -25,7 +25,7 @@ seq=`basename $
[Qemu-devel] qemu-iotests failures for qcow and vpc
Hi, I just ran the qemu-iotests suite and found five failures in the vpc and qcow block types. The following command was executed with sources and objects in tmpfs: for i in qcow2 qed qcow generic vmdk raw vpc vhdx vdi \ cow parallels cloop bochs; do ./check -c writeback -$i; done QEMU version: v2.1.0-233-g8e6e2c2 plus patch "qemu-iotests: Fix 028 reference output for qed"[1]. Please find the results of failing tests below (`-c writeback` does not affect the output). -- Kind regards, Peter https://lekensteyn.nl [1]: http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg03270.html + ./check -vpc 004 006 QEMU -- /tmp/qcompile/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64 QEMU_IMG -- /tmp/qcompile/tests/qemu-iotests/../../qemu-img QEMU_IO -- /tmp/qcompile/tests/qemu-iotests/../../qemu-io QEMU_NBD -- /tmp/qcompile/tests/qemu-iotests/../../qemu-nbd IMGFMT-- vpc IMGPROTO -- file PLATFORM -- Linux/x86_64 antartica 3.16.0-rc7-rtl-1-g3f4adc1 SOCKET_SCM_HELPER -- /tmp/qcompile/tests/qemu-iotests/socket_scm_helper 004 0s ... - output mismatch (see 004.out.bad) --- /tmp/qemu/tests/qemu-iotests/004.out2013-08-13 17:27:42.097765037 +0200 +++ 004.out.bad 2014-08-19 21:57:33.125799947 +0200 @@ -9,7 +9,8 @@ write failed: Input/output error write at image boundary -write failed: Input/output error +wrote 4096/4096 bytes at offset 134217728 +4 KiB, 1 ops; 0.0002 sec (17.675 MiB/sec and 4524.8869 ops/sec) write past image boundary write failed: Input/output error @@ -28,7 +29,8 @@ read failed: Input/output error read at image boundary -read failed: Input/output error +read 4096/4096 bytes at offset 134217728 +4 KiB, 1 ops; 0.0001 sec (27.316 MiB/sec and 6993.0070 ops/sec) read past image boundary read failed: Input/output error 006 - output mismatch (see 006.out.bad) --- /tmp/qemu/tests/qemu-iotests/006.out2013-08-13 17:27:42.097765037 +0200 +++ 006.out.bad 2014-08-19 21:57:33.205799949 +0200 @@ -1,6 +1,5 @@ QA output created by 006 creating 128GB image -qemu-img: The image size is too large for file format 'vpc' Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=137438953472 *** done Failures: 004 006 Failed 2 of 2 tests + ./check -qcow 003 042 048 QEMU -- /tmp/qcompile/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64 QEMU_IMG -- /tmp/qcompile/tests/qemu-iotests/../../qemu-img QEMU_IO -- /tmp/qcompile/tests/qemu-iotests/../../qemu-io QEMU_NBD -- /tmp/qcompile/tests/qemu-iotests/../../qemu-nbd IMGFMT-- qcow IMGPROTO -- file PLATFORM -- Linux/x86_64 antartica 3.16.0-rc7-rtl-1-g3f4adc1 SOCKET_SCM_HELPER -- /tmp/qcompile/tests/qemu-iotests/socket_scm_helper 003 1s ... - output mismatch (see 003.out.bad) --- /tmp/qemu/tests/qemu-iotests/003.out2013-08-13 17:27:42.097765037 +0200 +++ 003.out.bad 2014-08-19 21:57:35.59774 +0200 @@ -6,10 +6,12 @@ 128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) == rewriting whole image == +main-loop: WARNING: I/O thread spun for 1000 iterations wrote 134217728/134217728 bytes at offset 0 128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) == verify pattern == +main-loop: WARNING: I/O thread spun for 1000 iterations read 134217728/134217728 bytes at offset 0 128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) 042 0s ... - output mismatch (see 042.out.bad) --- /tmp/qemu/tests/qemu-iotests/042.out2013-08-13 17:27:42.189766279 +0200 +++ 042.out.bad 2014-08-19 21:57:35.68175 +0200 @@ -2,14 +2,18 @@ == Creating zero size image == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=0 -No errors were found on the image. +qemu-img: Could not open 'TEST_DIR/t.qcow': Image size is too small (must be at least 2 bytes) == Converting the image == -No errors were found on the image. +qemu-img: Could not open '/tmp/qcompile/tests/qemu-iotests/scratch/t.qcow.orig': Image size is too small (must be at least 2 bytes) +qemu-img: Could not open '/tmp/qcompile/tests/qemu-iotests/scratch/t.qcow.orig' +qemu-img: Could not open 'TEST_DIR/t.qcow': Could not open 'TEST_DIR/t.qcow': No such file or directory == Converting the image, compressed == -No errors were found on the image. +qemu-img: Could not open 'TEST_DIR/t.qcow': Could not open 'TEST_DIR/t.qcow': No such file or directory == Rebasing the image == -No errors were found on the image. +qemu-img: Could not open '/tmp/qcompile/tests/qemu-iotests/scratch/t.qcow': Could not open '/tmp/qcompile/tests/qemu-iotests/scratch/t.qcow': No such file or directory +qemu-img: Could not open '/tmp/qcompile/tests/qemu-iotests/scratch/t.qcow': Could not open '/tmp/qcompile/tests/qemu-iotests/scratch/t.qcow': No such file or directory +qemu-img: Could not open 'TEST_DIR/t.qcow': Could not open 'TEST_DIR/t.qcow': No such file or directory *** done 048 0s ... - output mismatch (see 048.out.bad) --- /tmp/qemu/t
Re: [Qemu-devel] [PATCH] qemu-iotests: Fix 028 reference output for qed
On Tuesday 19 August 2014 19:33:55 Kevin Wolf wrote: > We need to filter out driver-specific options in the "Formatting..." > string printed by qemu when creating the backup image. > > Reported-by: Peter Wu > Signed-off-by: Kevin Wolf Tested-by: Peter Wu It works (./check -qed 028), thanks! -- Kind regards, Peter https://lekensteyn.nl
Re: [Qemu-devel] [PATCH 2/2] usb: mtp: reply INCOMPLETE_TRANSFER on read errors
On Tuesday 29 April 2014 10:43:52 Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann
Reviewed-by: Peter Wu
> ---
> hw/usb/dev-mtp.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 62428d8..943f930 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -50,6 +50,7 @@ enum mtp_code {
> RES_INVALID_TRANSACTION_ID = 0x2004,
> RES_OPERATION_NOT_SUPPORTED= 0x2005,
> RES_PARAMETER_NOT_SUPPORTED= 0x2006,
> +RES_INCOMPLETE_TRANSFER= 0x2007,
> RES_INVALID_STORAGE_ID = 0x2008,
> RES_INVALID_OBJECT_HANDLE = 0x2009,
> RES_SPEC_BY_FORMAT_UNSUPPORTED = 0x2014,
> @@ -946,7 +947,8 @@ static void usb_mtp_handle_data(USBDevice *dev, USBPacket
> *p)
> }
> rc = read(d->fd, d->data, dlen);
> if (rc != dlen) {
> -fprintf(stderr, "%s: TODO: handle read error\n",
> __func__);
> +memset(d->data, 0, dlen);
> +s->result->code = RES_INCOMPLETE_TRANSFER;
> }
> usb_packet_copy(p, d->data, dlen);
> }
>
Re: [Qemu-devel] [PATCH 1/2] usb: mtp: fix possible buffer overflow
On Tuesday 29 April 2014 10:43:51 Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann
Reviewed-by: Peter Wu
> ---
> hw/usb/dev-mtp.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index b6eaeae..62428d8 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -998,6 +998,14 @@ static void usb_mtp_handle_data(USBDevice *dev,
> USBPacket *p)
> cmd.argc = (le32_to_cpu(container.length) - sizeof(container))
> / sizeof(uint32_t);
> cmd.trans = le32_to_cpu(container.trans);
> +if (cmd.argc > ARRAY_SIZE(cmd.argv)) {
> +cmd.argc = ARRAY_SIZE(cmd.argv);
> +}
> +if (p->iov.size < sizeof(container) + cmd.argc *
> sizeof(uint32_t)) {
> +trace_usb_mtp_stall(s->dev.addr, "packet too small");
> +p->status = USB_RET_STALL;
> +return;
> +}
> usb_packet_copy(p, ¶ms, cmd.argc * sizeof(uint32_t));
> for (i = 0; i < cmd.argc; i++) {
> cmd.argv[i] = le32_to_cpu(params[i]);
>
Re: [Qemu-devel] [PATCH 0/9] usb: mtp: a bunch of fixes
On Friday 25 April 2014 12:48:05 Gerd Hoffmann wrote: > Hi, > > Here is a bunch of incremental fixes for the just merged > usb mtp device, coming from patch review by Peter and Stefan. > > cheers, > Gerd > > Gerd Hoffmann (9): > usb: mtp: replace debug printfs with trace points > usb: mtp: fix usb_mtp_add_u64 > usb: mtp: fix version (is decimal not bcd) > usb: mtp: fix serial (must be exact 32 chars) > usb: mtp: fix error path memory leak > usb: mtp: fix possible buffer overflow > usb: mtp: avoid empty description string > usb: mtp: drop data-out hexdump > usb: mtp: reply INCOMPLETE_TRANSFER on read errors > > hw/usb/dev-mtp.c | 29 + > trace-events | 3 +++ > 2 files changed, 20 insertions(+), 12 deletions(-) For patches 1-5 and 7-8: Reviewed-by: Peter Wu Patch 6 and 9 have some comments. Kind regards, Peter
Re: [Qemu-devel] [PATCH 9/9] usb: mtp: reply INCOMPLETE_TRANSFER on read errors
On Friday 25 April 2014 12:48:14 Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann
> ---
> hw/usb/dev-mtp.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 6bd6a82..8e1a1b7 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -50,6 +50,7 @@ enum mtp_code {
> RES_INVALID_TRANSACTION_ID = 0x2004,
> RES_OPERATION_NOT_SUPPORTED= 0x2005,
> RES_PARAMETER_NOT_SUPPORTED= 0x2006,
> +RES_INCOMPLETE_TRANSFER= 0x2007,
> RES_INVALID_STORAGE_ID = 0x2008,
> RES_INVALID_OBJECT_HANDLE = 0x2009,
> RES_SPEC_BY_FORMAT_UNSUPPORTED = 0x2014,
> @@ -946,7 +947,7 @@ static void usb_mtp_handle_data(USBDevice *dev, USBPacket
> *p) }
> rc = read(d->fd, d->data, dlen);
> if (rc != dlen) {
> -fprintf(stderr, "%s: TODO: handle read error\n",
> __func__);
> +s->result->code = RES_INCOMPLETE_TRANSFER;
> }
> usb_packet_copy(p, d->data, dlen);
> }
The bogus data packet is sent with usb_packet_copy, shouldn't you return
USB_RET_NAK for now? Something like:
if (rc == dlen) {
usb_packet_copy(p, d->data, dlen);
} else {
s->result->code = RES_INCOMPLETE_TRANSFER;
dlen = 0;
}
dlen = 0 to avoid messing with the offset.
Kind regards,
Peter
Re: [Qemu-devel] [PATCH 6/9] usb: mtp: fix possible buffer overflow
On Friday 25 April 2014 12:48:11 Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann
> ---
> hw/usb/dev-mtp.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 45f9562..82d5b64 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -998,6 +998,9 @@ static void usb_mtp_handle_data(USBDevice *dev,
> USBPacket *p) cmd.argc = (le32_to_cpu(container.length) -
> sizeof(container)) / sizeof(uint32_t);
> cmd.trans = le32_to_cpu(container.trans);
> +if (cmd.argc > ARRAY_SIZE(cmd.argv)) {
> +cmd.argc = ARRAY_SIZE(cmd.argv);
> +}
This is not sufficient, you must also check that the iov is large enough to
hold this many args.
Kind regards,
Peter
> usb_packet_copy(p, ¶ms, cmd.argc * sizeof(uint32_t));
> for (i = 0; i < cmd.argc; i++) {
> cmd.argv[i] = le32_to_cpu(params[i]);
Re: [Qemu-devel] [PULL v2 2/2] usb: mtp filesharing
On Thursday 24 April 2014 17:01:47 Gerd Hoffmann wrote: > PS: Funny thing that the reviews start coming in when I send pull > requests. The patches have been on the list a few weeks back > already (during 2.0 freeze, thats why the long delay between > [patch] and [pull]). No comments. > Should I consider going straight for a pull requests to get > reviews faster? Well, I am interested in the USB subsystem (working on usb monitoring functionality[1] and a virtual Logitech Unifying device[2]), but only just noticed this subject on the list. (I am subscribed since somewhere in the end of March.) The messages volume is also large, so it is easy to miss some. It would be nice if someone could make Launchpad not send out duplicate mails to the list. Regards, Peter [1]: https://git.lekensteyn.nl/peter/qemu/log/?h=usbdump-usbhid [2]: https://git.lekensteyn.nl/peter/qemu/log/?h=logitech-unifying
Re: [Qemu-devel] [PULL v2 2/2] usb: mtp filesharing
Hi,
For this review, I grabbed the MTP 1.1 spec from USB.org. There are some
issues which are noted below.
On Wednesday 23 April 2014 10:31:01 Gerd Hoffmann wrote:
> Implementation of a USB Media Transfer Device device for easy
> filesharing. Read-only. No access control inside qemu, it will
> happily export any file it is able to open to the guest, i.e.
> standard unix access rights for the qemu process apply.
>
> Signed-off-by: Gerd Hoffmann
> ---
> default-configs/usb.mak |1 +
> hw/usb/Makefile.objs|4 +
> hw/usb/dev-mtp.c| 1103
> +++
> trace-events| 21 +
> 4 files changed, 1129 insertions(+)
> create mode 100644 hw/usb/dev-mtp.c
> +/* --- */
> +
> +static MTPObject *usb_mtp_object_alloc(MTPState *s, uint32_t handle,
> + MTPObject *parent, char *name)
> +{
> +MTPObject *o = g_new0(MTPObject, 1);
> +
> +if (name[0] == '.') {
> +goto ignore;
> +}
> +
> +o->handle = handle;
> +o->parent = parent;
> +o->name = g_strdup(name);
> +o->nchildren = -1;
> +if (parent == NULL) {
> +o->path = g_strdup(name);
> +} else {
> +o->path = g_strdup_printf("%s/%s", parent->path, name);
> +}
> +
> +if (lstat(o->path, &o->stat) != 0) {
> +goto ignore;
> +}
> +if (S_ISREG(o->stat.st_mode)) {
> +o->format = FMT_UNDEFINED_OBJECT;
> +} else if (S_ISDIR(o->stat.st_mode)) {
> +o->format = FMT_ASSOCIATION;
> +} else {
> +goto ignore;
> +}
> +
> +if (access(o->path, R_OK) != 0) {
> +goto ignore;
> +}
> +
> +fprintf(stderr, "%s: 0x%x %s\n", __func__, o->handle, o->path);
Debugging left-over?
> +QTAILQ_INSERT_TAIL(&s->objects, o, next);
> +return o;
> +
> +ignore:
> +g_free(o->name);
> +g_free(o->path);
> +g_free(o);
> +return NULL;
> +}
> +
> +static void usb_mtp_object_free(MTPState *s, MTPObject *o)
> +{
> +int i;
> +
> +fprintf(stderr, "%s: 0x%x %s\n", __func__, o->handle, o->path);
Debugging left-over #2?
> +QTAILQ_REMOVE(&s->objects, o, next);
> +for (i = 0; i < o->nchildren; i++) {
> +usb_mtp_object_free(s, o->children[i]);
> +}
> +g_free(o->children);
> +g_free(o->name);
> +g_free(o->path);
> +g_free(o);
> +}
> +
> +static void usb_mtp_add_u8(MTPData *data, uint8_t val)
> +{
> +usb_mtp_realloc(data, 1);
> +data->data[data->length++] = val;
> +}
> +
> +static void usb_mtp_add_u16(MTPData *data, uint16_t val)
> +{
> +usb_mtp_realloc(data, 2);
> +data->data[data->length++] = (val >> 0) & 0xff;
> +data->data[data->length++] = (val >> 8) & 0xff;
> +}
> +
> +static void usb_mtp_add_u32(MTPData *data, uint32_t val)
> +{
> +usb_mtp_realloc(data, 4);
> +data->data[data->length++] = (val >> 0) & 0xff;
> +data->data[data->length++] = (val >> 8) & 0xff;
> +data->data[data->length++] = (val >> 16) & 0xff;
> +data->data[data->length++] = (val >> 24) & 0xff;
> +}
> +
> +static void usb_mtp_add_u64(MTPData *data, uint64_t val)
> +{
> +usb_mtp_realloc(data, 4);
usb_mtp_realloc(data, 8);
> +data->data[data->length++] = (val >> 0) & 0xff;
> +data->data[data->length++] = (val >> 8) & 0xff;
> +data->data[data->length++] = (val >> 16) & 0xff;
> +data->data[data->length++] = (val >> 24) & 0xff;
> +data->data[data->length++] = (val >> 32) & 0xff;
> +data->data[data->length++] = (val >> 40) & 0xff;
> +data->data[data->length++] = (val >> 48) & 0xff;
> +data->data[data->length++] = (val >> 54) & 0xff;
48 + 8 = 56. What about a loop instead?
> +}
> +
> +/* --- */
> +
> +static MTPData *usb_mtp_get_device_info(MTPState *s, MTPControl *c)
> +{
> +static const uint16_t ops[] = {
> +CMD_GET_DEVICE_INFO,
> +CMD_OPEN_SESSION,
> +CMD_CLOSE_SESSION,
> +CMD_GET_STORAGE_IDS,
> +CMD_GET_STORAGE_INFO,
> +CMD_GET_NUM_OBJECTS,
> +CMD_GET_OBJECT_HANDLES,
> +CMD_GET_OBJECT_INFO,
> +CMD_GET_OBJECT,
> +CMD_GET_PARTIAL_OBJECT,
> +};
> +static const uint16_t fmt[] = {
> +FMT_UNDEFINED_OBJECT,
> +FMT_ASSOCIATION,
> +};
> +MTPData *d = usb_mtp_data_alloc(c);
> +
> +trace_usb_mtp_op_get_device_info(s->dev.addr);
> +
> +usb_mtp_add_u16(d, 0x0100);
Sect. 5.1.1.1 says:
"This identifies the PTP version this device can support in
hundredths. For MTP devices implemented under this specification,
this shall contain the value 100 (representing 1.00)."
Is it an error in the spec (missing 0x) or should the value here really
be 0x0100 instead of 0x0064?
> +usb_mtp_add_u32(d, 0x);
> +usb_mtp_add_u16(d, 0x0101);
> +usb_mtp_add_wstr(d, L"");
> +usb_mtp_add_u16(d, 0x000
Re: [Qemu-devel] [PATCH] chardev: add baud parameter for serial host device
On Saturday 15 June 2013 16:14:23 Eric Blake wrote:
> On 06/08/2013 10:49 PM, Peter Wu wrote:
> > When QEMU starts, it always changes the serial port parameters including
> > baud rate. This confused my guest which thought it was outputting at 9600
> > baud while it was in fact changed to 115200.
> >
> > After this patch, I can use `-serial /dev/ttyS0,baud=9600` to override the
> > default baud rate of 115200. Documentation is updated as well, so that
> > users know about the new `baud` parameter for `-serial` and `-chardev
> > serial` (and its alias `-chardev tty`).
> >
> > Note that the baud option is not implemented for Windows. QEMU does not
> > change the default baud rate on Windows anyway. If somebody is going to
> > implement it, do not forget to update the documentation on "COM" devices
> > which is also of backend serial.
> >
> > Signed-off-by: Peter Wu
> > ---
> >
> > +++ b/qapi-schema.json
> > @@ -3186,7 +3186,7 @@
> >
> > # Configuration info for device and pipe chardevs.
> > #
> > # @device: The name of the special file for the device,
> >
> > -# i.e. /dev/ttyS0 on Unix or COM1: on Windows
> > +# i.e. /dev/parport0 on Unix.
> >
> > # @type: What kind of device this is.
> > #
> > # Since: 1.4
> >
> > @@ -3194,6 +3194,20 @@
> >
> > { 'type': 'ChardevHostdev', 'data': { 'device' : 'str' } }
> >
> > ##
> >
> > +# @ChardevSerial
> > +#
> > +# Configuration info for serial chardevs.
> > +#
> > +# @device: The name of the special file for the device,
> > +# i.e. /dev/ttyS0 on Unix or COM1: on Windows
> > +# @baud: #optional baud rate to set for host device. (default 115200)
> > +#
> > +# Since: 1.5
> > +##
> > +{ 'type': 'ChardevSerial', 'data': { 'device' : 'str',
> > + '*baud': 'int' } }
>
> Slick trick. However, 1.5 is already released, so it is now since 1.6,
Well, it was worth trying :-P Anyway, I do not mind if the patch is edited to
have 1.6 instead 1.5. Do you want me to send a new patch just for this tiny
change or will you edit it before applying? (perhaps after adding a note to
the commit message)
> and furthermore...
>
> > +
> > +##
> >
> > # @ChardevSocket:
> > #
> > # Configuration info for (stream) socket chardevs.
> >
> > @@ -3311,7 +3325,7 @@
> >
> > { 'type': 'ChardevDummy', 'data': { } }
> >
> > { 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile',
> >
> > - 'serial' : 'ChardevHostdev',
> > + 'serial' : 'ChardevSerial',
>
> without introspection, libvirt has no idea whether 'baud' is supported
> in the qemu it is talking to, other than trying and failing when talking
> to older qemu. This patch forms yet another reason why libvirt wants to
> learn when we add optional parameters to a pre-existing QMP command.
I have do not use libvirt, do you want me to do further things for this patch
to get accepted? Or is it more a side-note?
Regards,
Peter
signature.asc
Description: This is a digitally signed message part.
Re: [Qemu-devel] [PATCH trivial] vl: always define no_frame
On Saturday 15 June 2013 14:42:03 Michael Tokarev wrote: > Commit 047d4e151dd46 "Unbreak -no-quit for GTK, validate SDL options" broke > build of qemu without sdl, by referencing `no_frame' variable which is > defined inside #if SDL block. Fix that by defining that variable > unconditionally. > > This is a better fix for the build issue introduced by that patch than > a revert. This change keeps the new functinality introduced by that patch > and just fixes the compilation. It still is not a complete fix around the > original issue (not working -no-frame et al with -display gtk), because it > makes only the legacy interface working, not the new suboption interface, > so a few more changes are needed. Reviewed-by: Peter Wu -no-frame was deemed to be a SDL feature that was unnecessary for GTK, hence dropped. At run-time, it is checked whether -no-frame makes sense or not (that is, if `-display sdl` was specified or not). Original patch and discussion can be found at http://lists.nongnu.org/archive/html/qemu-devel/2013-06/msg01297.html. As for the discussion about legacy options[1], are there plans to remove `-sdl` (replaced by `-display sdl`) and move the `-*-grab` and `-no-frame` to `-display sdl` options? If that is the idea, what about printing a message that the mentioned options are deprecated and removed in a future version? Regards, Peter [1]: http://lists.nongnu.org/archive/html/qemu-devel/2013-06/msg02437.html > Cc: Peter Wu > Cc: qemu-triv...@nongnu.org > Signed-off-by: Michael Tokarev > --- > vl.c |2 -- > 1 file changed, 2 deletions(-) > > diff --git a/vl.c b/vl.c > index 9f8fd6e..f94ec9c 100644 > --- a/vl.c > +++ b/vl.c > @@ -199,9 +199,7 @@ static int rtc_date_offset = -1; /* -1 means no change > */ QEMUClock *rtc_clock; > int vga_interface_type = VGA_NONE; > static int full_screen = 0; > -#ifdef CONFIG_SDL > static int no_frame = 0; > -#endif > int no_quit = 0; > CharDriverState *serial_hds[MAX_SERIAL_PORTS]; > CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
[Qemu-devel] [PATCH] Unbreak -no-quit for GTK, validate SDL options
Certain options (-no-frame, -alt-grab, -ctrl-grab) only make sense with SDL.
When compiling without SDL, these options (and -no-quit) print an error message
and exit qemu.
In case QEMU is compiled with SDL support, the three aforementioned options
still do not make sense with other display types. This patch addresses that
issue by printing a warning. I have chosen not to exit QEMU afterwards because
having the option is not harmful and before this patch it would be ignored
anyway.
By delaying the sanity check from compile-time with some ifdefs to run-time,
-no-quit is now also properly supported when compiling without SDL.
Signed-off-by: Peter Wu
---
vl.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/vl.c b/vl.c
index cfd2d3e..29ab85c 100644
--- a/vl.c
+++ b/vl.c
@@ -3523,7 +3523,6 @@ int main(int argc, char **argv, char **envp)
case QEMU_OPTION_full_screen:
full_screen = 1;
break;
-#ifdef CONFIG_SDL
case QEMU_OPTION_no_frame:
no_frame = 1;
break;
@@ -3536,14 +3535,11 @@ int main(int argc, char **argv, char **envp)
case QEMU_OPTION_no_quit:
no_quit = 1;
break;
+#ifdef CONFIG_SDL
case QEMU_OPTION_sdl:
display_type = DT_SDL;
break;
#else
-case QEMU_OPTION_no_frame:
-case QEMU_OPTION_alt_grab:
-case QEMU_OPTION_ctrl_grab:
-case QEMU_OPTION_no_quit:
case QEMU_OPTION_sdl:
fprintf(stderr, "SDL support is disabled\n");
exit(1);
@@ -4084,6 +4080,15 @@ int main(int argc, char **argv, char **envp)
#endif
}
+if ((no_frame || alt_grab || ctrl_grab) && display_type != DT_SDL) {
+fprintf(stderr, "-no-frame, -alt-grab and -ctrl-grab are only valid "
+"for SDL, ignoring option\n");
+}
+if (no_quit && (display_type != DT_GTK && display_type != DT_SDL)) {
+fprintf(stderr, "-no-quit is only valid for GTK and SDL, "
+"ignoring option\n");
+}
+
#if defined(CONFIG_GTK)
if (display_type == DT_GTK) {
early_gtk_display_init();
--
1.8.3
Re: [Qemu-devel] [PATCH] Unbreak -no-quit for GTK when SDL is disabled
On Monday 10 June 2013 16:53:06 Anthony Liguori wrote: > Peter Wu writes: > > When QEMU is built without SDL support, -no-quit would print an error > > message that SDL is disabled. Since GTK also supports -no-quit, enable > > the option when GTK or SDL is enabled at compile time. > > > > While at it, do not create the no_quit variable when it is not used. > > > > Signed-off-by: Peter Wu > > Any harm in just making -no-quit unconditional? No harm, there are just 4 bytes more memory wasted and some code, not a big deal. > It's a nop for VNC and presumably for spice too but it's not incorrect > AFAICT. > > Would be nice to drop #ifdefs if we can. The readability should increase by dropping it. By the way, this integer should really be a bool. As shortly discussed on IRC, a better approach is testing for the sanity of certain usages instead of ifdef'ing stuff. Having SDL compiled in and then using VNC still does not make -alt-grab more valid. Let's drop this patch, I will submit a new one which checks the option at run- time. Regards, Peter > Regards, > > Anthony Liguori > > > --- > > > > include/sysemu/sysemu.h | 2 ++ > > vl.c| 13 + > > 2 files changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > > index 2fb71af..b9b8e52 100644 > > --- a/include/sysemu/sysemu.h > > +++ b/include/sysemu/sysemu.h > > @@ -117,7 +117,9 @@ extern int smp_cpus; > > > > extern int max_cpus; > > extern int cursor_hide; > > extern int graphic_rotate; > > > > +#if defined(CONFIG_SDL) || defined(CONFIG_GTK) > > > > extern int no_quit; > > > > +#endif > > > > extern int no_shutdown; > > extern int semihosting_enabled; > > extern int old_param; > > > > diff --git a/vl.c b/vl.c > > index cfd2d3e..74ab050 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -202,7 +202,9 @@ static int full_screen = 0; > > > > #ifdef CONFIG_SDL > > static int no_frame = 0; > > #endif > > > > +#if defined(CONFIG_SDL) || defined(CONFIG_GTK) > > > > int no_quit = 0; > > > > +#endif > > > > CharDriverState *serial_hds[MAX_SERIAL_PORTS]; > > CharDriverState *parallel_hds[MAX_PARALLEL_PORTS]; > > CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES]; > > > > @@ -3523,6 +3525,13 @@ int main(int argc, char **argv, char **envp) > > > > case QEMU_OPTION_full_screen: > > full_screen = 1; > > break; > > > > +case QEMU_OPTION_no_quit: > > +#if defined(CONFIG_SDL) || defined(CONFIG_GTK) > > +no_quit = 1; > > +#else > > +fprintf(stderr, "SDL and GTK support are disabled\n"); > > +#endif > > +break; > > > > #ifdef CONFIG_SDL > > > > case QEMU_OPTION_no_frame: > > no_frame = 1; > > > > @@ -3533,9 +3542,6 @@ int main(int argc, char **argv, char **envp) > > > > case QEMU_OPTION_ctrl_grab: > > ctrl_grab = 1; > > break; > > > > -case QEMU_OPTION_no_quit: > > -no_quit = 1; > > -break; > > > > case QEMU_OPTION_sdl: > > display_type = DT_SDL; > > break; > > > > @@ -3543,7 +3549,6 @@ int main(int argc, char **argv, char **envp) > > > > case QEMU_OPTION_no_frame: > > case QEMU_OPTION_alt_grab: > > > > case QEMU_OPTION_ctrl_grab: > > -case QEMU_OPTION_no_quit: > > case QEMU_OPTION_sdl: > > fprintf(stderr, "SDL support is disabled\n"); > > exit(1);
[Qemu-devel] [PATCH] Unbreak -no-quit for GTK when SDL is disabled
When QEMU is built without SDL support, -no-quit would print an error message that SDL is disabled. Since GTK also supports -no-quit, enable the option when GTK or SDL is enabled at compile time. While at it, do not create the no_quit variable when it is not used. Signed-off-by: Peter Wu --- include/sysemu/sysemu.h | 2 ++ vl.c| 13 + 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 2fb71af..b9b8e52 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -117,7 +117,9 @@ extern int smp_cpus; extern int max_cpus; extern int cursor_hide; extern int graphic_rotate; +#if defined(CONFIG_SDL) || defined(CONFIG_GTK) extern int no_quit; +#endif extern int no_shutdown; extern int semihosting_enabled; extern int old_param; diff --git a/vl.c b/vl.c index cfd2d3e..74ab050 100644 --- a/vl.c +++ b/vl.c @@ -202,7 +202,9 @@ static int full_screen = 0; #ifdef CONFIG_SDL static int no_frame = 0; #endif +#if defined(CONFIG_SDL) || defined(CONFIG_GTK) int no_quit = 0; +#endif CharDriverState *serial_hds[MAX_SERIAL_PORTS]; CharDriverState *parallel_hds[MAX_PARALLEL_PORTS]; CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES]; @@ -3523,6 +3525,13 @@ int main(int argc, char **argv, char **envp) case QEMU_OPTION_full_screen: full_screen = 1; break; +case QEMU_OPTION_no_quit: +#if defined(CONFIG_SDL) || defined(CONFIG_GTK) +no_quit = 1; +#else +fprintf(stderr, "SDL and GTK support are disabled\n"); +#endif +break; #ifdef CONFIG_SDL case QEMU_OPTION_no_frame: no_frame = 1; @@ -3533,9 +3542,6 @@ int main(int argc, char **argv, char **envp) case QEMU_OPTION_ctrl_grab: ctrl_grab = 1; break; -case QEMU_OPTION_no_quit: -no_quit = 1; -break; case QEMU_OPTION_sdl: display_type = DT_SDL; break; @@ -3543,7 +3549,6 @@ int main(int argc, char **argv, char **envp) case QEMU_OPTION_no_frame: case QEMU_OPTION_alt_grab: case QEMU_OPTION_ctrl_grab: -case QEMU_OPTION_no_quit: case QEMU_OPTION_sdl: fprintf(stderr, "SDL support is disabled\n"); exit(1); -- 1.8.3
[Qemu-devel] [PATCH v2] gtk: implement -full-screen
Aiming for GTK as replacement for SDL, a feature like -full-screen should also
be implemented.
Bringing the window into full-screen mode is done by activating the "Fullscreen"
menu item. This is done after showing the windows to make the cursor and menu
hidden.
v2: drop -no-frame implementation, use booleans instead of ints and ensure
consistency between ui state and menu.
Signed-off-by: Peter Wu
---
include/ui/console.h | 2 +-
ui/gtk.c | 6 +-
vl.c | 2 +-
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/include/ui/console.h b/include/ui/console.h
index 4307b5f..f1d79f9 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -339,6 +339,6 @@ int index_from_keycode(int code);
/* gtk.c */
void early_gtk_display_init(void);
-void gtk_display_init(DisplayState *ds);
+void gtk_display_init(DisplayState *ds, bool full_screen);
#endif
diff --git a/ui/gtk.c b/ui/gtk.c
index 3bc2842..1c625c0 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -1433,7 +1433,7 @@ static const DisplayChangeListenerOps dcl_ops = {
.dpy_cursor_define = gd_cursor_define,
};
-void gtk_display_init(DisplayState *ds)
+void gtk_display_init(DisplayState *ds, bool full_screen)
{
GtkDisplayState *s = g_malloc0(sizeof(*s));
char *filename;
@@ -1509,6 +1509,10 @@ void gtk_display_init(DisplayState *ds)
gtk_widget_show_all(s->window);
+if (full_screen) {
+gtk_menu_item_activate(GTK_MENU_ITEM(s->full_screen_item));
+}
+
register_displaychangelistener(&s->dcl);
global_state = s;
diff --git a/vl.c b/vl.c
index 47ab45d..cfd2d3e 100644
--- a/vl.c
+++ b/vl.c
@@ -4347,7 +4347,7 @@ int main(int argc, char **argv, char **envp)
#endif
#if defined(CONFIG_GTK)
case DT_GTK:
-gtk_display_init(ds);
+gtk_display_init(ds, full_screen);
break;
#endif
default:
--
1.8.3
Re: [Qemu-devel] [PATCH] chardev: add baud parameter for serial host device
On Monday 10 June 2013 15:28:32 Peter Wu wrote: > On Monday 10 June 2013 14:58:07 Gerd Hoffmann wrote: > > On 06/10/13 10:42, Peter Wu wrote: > > > On Monday 10 June 2013 07:56:01 Gerd Hoffmann wrote: > > >> On 06/08/13 23:49, Peter Wu wrote: > > >>> When QEMU starts, it always changes the serial port parameters > > >>> including > > >>> baud rate. This confused my guest which thought it was outputting at > > >>> 9600 > > >>> baud while it was in fact changed to 115200. > > >>> > > >>> > > >>> > > >>> > > >>> > > >>> After this patch, I can use `-serial /dev/ttyS0,baud=9600` to override > > >>> the > > >>> default baud rate of 115200. > > >> > > >> > > >> > > >> I think we should just flip the default to 9600. IIRC this is the > > >> power-on default baud rate of the 8250 uart family, so this should be > > >> the qemu default too. If a guest wants to use a higher baudrate it has > > >> to reprogram the uart anyway (and qemu will apply the guest changes to > > >> the host uart). > > > > > > > > > > > > FWIW, when I tried MODE.COM in ms-dos to change the baud rate, `stty -F > > > /dev/ttyS0 -a` still reported 115200 baud. This is on Linux 3.9 if that > > > matters. > > > > > > > > Hmm, with a linux guest changing the baudrate works just fine. Any > > chance mode.com takes a shortcut in case it thinks the rate didn't > > change? Does setting the speed first to 4800, then to 9600 work? > > I can confirm that a Linux guest can correctly control the speed. At home I > only have a USB serial, but that shouldn't matter. > 1. stty -F /dev/ttyUSB0 reports 9600 > 2. Start QEMU using: > qemu-system-x86_64 -enable-kvm -m 1G -serial /dev/ttyUSB0 \ > -cdrom ubuntu-12.04.1-desktop-amd64.iso > 3. stty -F /dev/ttyUSB0 reports 115200 before booting the live CD > 4. After boot, both the guest and host report 9600 again > 5. Changing it using stty -F /dev/ttyS0 from the guest is also visible on > both sides. > > On ms-dos I observe with 1.4.1 and 1.5.0 that the baud rate *is* changed > correctly with my USB serial converter. I will have to check with the > serial printer again whether I made a mistake before. I did actually have > to use `stty -F /dev/ttyS0 raw 9600` to avoid characters being eaten which > caused the serial printer to spit out empty lines or hang. Aha, I just checked that machine again and realised something. The serial cable has only four pins connected to a printer (DB9-DB25), namely RX, TX, DTR and RTS. The remaining cables are cut (originally it was a null modem cable, but that did not work with the printer). So, what is the likely issue here? Having a printer instead of a serial console or the hardware (cables) missing some lines? FYI, when I set the speed and options manually after starting QEMU, the printer(s) work(s) as expected, otherwise I get garbage out. Regards, Peter
Re: [Qemu-devel] [PATCH] chardev: add baud parameter for serial host device
On Monday 10 June 2013 14:58:07 Gerd Hoffmann wrote: > On 06/10/13 10:42, Peter Wu wrote: > > On Monday 10 June 2013 07:56:01 Gerd Hoffmann wrote: > >> On 06/08/13 23:49, Peter Wu wrote: > >>> When QEMU starts, it always changes the serial port parameters including > >>> baud rate. This confused my guest which thought it was outputting at > >>> 9600 > >>> baud while it was in fact changed to 115200. > >>> > >>> > >>> > >>> After this patch, I can use `-serial /dev/ttyS0,baud=9600` to override > >>> the > >>> default baud rate of 115200. > >> > >> I think we should just flip the default to 9600. IIRC this is the > >> power-on default baud rate of the 8250 uart family, so this should be > >> the qemu default too. If a guest wants to use a higher baudrate it has > >> to reprogram the uart anyway (and qemu will apply the guest changes to > >> the host uart). > > > > > > > > FWIW, when I tried MODE.COM in ms-dos to change the baud rate, `stty -F > > /dev/ttyS0 -a` still reported 115200 baud. This is on Linux 3.9 if that > > matters. > > Hmm, with a linux guest changing the baudrate works just fine. Any > chance mode.com takes a shortcut in case it thinks the rate didn't > change? Does setting the speed first to 4800, then to 9600 work? I can confirm that a Linux guest can correctly control the speed. At home I only have a USB serial, but that shouldn't matter. 1. stty -F /dev/ttyUSB0 reports 9600 2. Start QEMU using: qemu-system-x86_64 -enable-kvm -m 1G -serial /dev/ttyUSB0 \ -cdrom ubuntu-12.04.1-desktop-amd64.iso 3. stty -F /dev/ttyUSB0 reports 115200 before booting the live CD 4. After boot, both the guest and host report 9600 again 5. Changing it using stty -F /dev/ttyS0 from the guest is also visible on both sides. On ms-dos I observe with 1.4.1 and 1.5.0 that the baud rate *is* changed correctly with my USB serial converter. I will have to check with the serial printer again whether I made a mistake before. I did actually have to use `stty -F /dev/ttyS0 raw 9600` to avoid characters being eaten which caused the serial printer to spit out empty lines or hang. Regards, Peter > > Besides this comment, any other feedback on the patch itself? > > Style is fine. But it appears to paper over some bug, and I'd prefer to > find+fix the bug instead of allowing/requiring the user to set the baud > rate manually to the correct value.
Re: [Qemu-devel] [PATCH] gtk: implement -full-screen and -no-frame
On Monday 10 June 2013 14:33:28 Kevin Wolf wrote: > Am 09.06.2013 um 12:30 hat Peter Wu geschrieben: > > Aiming for GTK as replacement for SDL, features like -full-screen and > > -no-frame should also be implemented. > > > > > > > > Bringing the window into full-screen mode is done by faking activating the > > full screen menu item with a NULL menu item (which currently is not used > > by gd_menu_full_screen). This is done after showing the windows to make > > the cursor and menu hidden. > > > > > > > > Signed-off-by: Peter Wu > > --- > > > > include/ui/console.h | 2 +- > > ui/gtk.c | 10 +- > > vl.c | 2 +- > > 3 files changed, 11 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/ui/console.h b/include/ui/console.h > > index 4307b5f..7174ba9 100644 > > --- a/include/ui/console.h > > +++ b/include/ui/console.h > > @@ -339,6 +339,6 @@ int index_from_keycode(int code); > > > > > > /* gtk.c */ > > void early_gtk_display_init(void); > > > > -void gtk_display_init(DisplayState *ds); > > +void gtk_display_init(DisplayState *ds, int full_screen, int no_frame); > > Should the new arguments be bool? Probably yes, but for consistency with the existing types I kept it as int. A future patch could change all uses of "int" to "bool" where 1 or 0 are used, do you prefer to use bool here anyway? Regards, Peter
Re: [Qemu-devel] [PATCH] chardev: add baud parameter for serial host device
On Monday 10 June 2013 07:56:01 Gerd Hoffmann wrote: > On 06/08/13 23:49, Peter Wu wrote: > > When QEMU starts, it always changes the serial port parameters including > > baud rate. This confused my guest which thought it was outputting at 9600 > > baud while it was in fact changed to 115200. > > > > > > > > After this patch, I can use `-serial /dev/ttyS0,baud=9600` to override the > > default baud rate of 115200. > > I think we should just flip the default to 9600. IIRC this is the > power-on default baud rate of the 8250 uart family, so this should be > the qemu default too. If a guest wants to use a higher baudrate it has > to reprogram the uart anyway (and qemu will apply the guest changes to > the host uart). FWIW, when I tried MODE.COM in ms-dos to change the baud rate, `stty -F /dev/ttyS0 -a` still reported 115200 baud. This is on Linux 3.9 if that matters. Besides this comment, any other feedback on the patch itself? Regards, Peter
Re: [Qemu-devel] [PATCH] gtk: implement -full-screen and -no-frame
On Sunday 09 June 2013 13:33:14 Anthony Liguori wrote:
> On Sun, Jun 9, 2013 at 5:30 AM, Peter Wu wrote:
> > Aiming for GTK as replacement for SDL, features like -full-screen and
> > -no-frame should also be implemented.
> >
> > Bringing the window into full-screen mode is done by faking activating the
> > full screen menu item with a NULL menu item (which currently is not used
> > by gd_menu_full_screen). This is done after showing the windows to make
> > the cursor and menu hidden.
> >
> > Signed-off-by: Peter Wu
> > ---
> >
> > include/ui/console.h | 2 +-
> > ui/gtk.c | 10 +-
> > vl.c | 2 +-
> > 3 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/ui/console.h b/include/ui/console.h
> > index 4307b5f..7174ba9 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -339,6 +339,6 @@ int index_from_keycode(int code);
> >
> > /* gtk.c */
> > void early_gtk_display_init(void);
> >
> > -void gtk_display_init(DisplayState *ds);
> > +void gtk_display_init(DisplayState *ds, int full_screen, int no_frame);
> >
> > #endif
> >
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 3bc2842..3df2611 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -1433,7 +1433,7 @@ static const DisplayChangeListenerOps dcl_ops = {
> >
> > .dpy_cursor_define = gd_cursor_define,
> >
> > };
> >
> > -void gtk_display_init(DisplayState *ds)
> > +void gtk_display_init(DisplayState *ds, int full_screen, int no_frame)
> >
> > {
> >
> > GtkDisplayState *s = g_malloc0(sizeof(*s));
> > char *filename;
> >
> > @@ -1457,6 +1457,10 @@ void gtk_display_init(DisplayState *ds)
> >
> > s->scale_y = 1.0;
> > s->free_scale = FALSE;
> >
> > +if (no_frame) {
> > +gtk_window_set_decorated(GTK_WINDOW(s->window), FALSE);
> > +}
> > +
>
> Is this really desirable? Why do people use -no-frame? I don't think
> we should carry over features from SDL just because they are there.
I started using it because -full-screen didn't work well in SDL. In SDL, full-
screen mode changes de video mode too. At some point, I even managed to crash
QEMU in full screen mode with certain Xorg settings.
I just added it to GTK because it was possible to do so. In GTK, I -full-
screen hides the menu bar (although there is still some pixels visible on the
top!) and the border without changing modes (which is a plus!). Due to this,
there is not really a need for -no-frame for me.
Others are likely using -no-frame in a space-constrained environment where
they need to show multiple windows.
> > setlocale(LC_ALL, "");
> > bindtextdomain("qemu", CONFIG_QEMU_LOCALEDIR);
> > textdomain("qemu");
> >
> > @@ -1509,6 +1513,10 @@ void gtk_display_init(DisplayState *ds)
> >
> > gtk_widget_show_all(s->window);
> >
> > +if (full_screen) {
> > +gd_menu_full_screen(NULL, s);
> > +}
> > +
>
> You should activate the menu item by using gtk_check_menu_item_set_active().
>
> Otherwise the menu with be out of sync with the UI state.
Alright, I forgot to check that. I will this fix in a next patch where I will
leave out -no-frame as well.
Thanks for review!
Regards,
Peter
> Regards,
>
> Anthony Liguori
>
> > register_displaychangelistener(&s->dcl);
> >
> > global_state = s;
> >
> > diff --git a/vl.c b/vl.c
> > index 47ab45d..5a00710 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -4347,7 +4347,7 @@ int main(int argc, char **argv, char **envp)
> >
> > #endif
> > #if defined(CONFIG_GTK)
> >
> > case DT_GTK:
> > -gtk_display_init(ds);
> > +gtk_display_init(ds, full_screen, no_frame);
> >
> > break;
> >
> > #endif
> >
> > default:
> > --
> > 1.8.3
[Qemu-devel] [PATCH] gtk: implement -full-screen and -no-frame
Aiming for GTK as replacement for SDL, features like -full-screen and -no-frame
should also be implemented.
Bringing the window into full-screen mode is done by faking activating the full
screen menu item with a NULL menu item (which currently is not used by
gd_menu_full_screen). This is done after showing the windows to make the cursor
and menu hidden.
Signed-off-by: Peter Wu
---
include/ui/console.h | 2 +-
ui/gtk.c | 10 +-
vl.c | 2 +-
3 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/include/ui/console.h b/include/ui/console.h
index 4307b5f..7174ba9 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -339,6 +339,6 @@ int index_from_keycode(int code);
/* gtk.c */
void early_gtk_display_init(void);
-void gtk_display_init(DisplayState *ds);
+void gtk_display_init(DisplayState *ds, int full_screen, int no_frame);
#endif
diff --git a/ui/gtk.c b/ui/gtk.c
index 3bc2842..3df2611 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -1433,7 +1433,7 @@ static const DisplayChangeListenerOps dcl_ops = {
.dpy_cursor_define = gd_cursor_define,
};
-void gtk_display_init(DisplayState *ds)
+void gtk_display_init(DisplayState *ds, int full_screen, int no_frame)
{
GtkDisplayState *s = g_malloc0(sizeof(*s));
char *filename;
@@ -1457,6 +1457,10 @@ void gtk_display_init(DisplayState *ds)
s->scale_y = 1.0;
s->free_scale = FALSE;
+if (no_frame) {
+gtk_window_set_decorated(GTK_WINDOW(s->window), FALSE);
+}
+
setlocale(LC_ALL, "");
bindtextdomain("qemu", CONFIG_QEMU_LOCALEDIR);
textdomain("qemu");
@@ -1509,6 +1513,10 @@ void gtk_display_init(DisplayState *ds)
gtk_widget_show_all(s->window);
+if (full_screen) {
+gd_menu_full_screen(NULL, s);
+}
+
register_displaychangelistener(&s->dcl);
global_state = s;
diff --git a/vl.c b/vl.c
index 47ab45d..5a00710 100644
--- a/vl.c
+++ b/vl.c
@@ -4347,7 +4347,7 @@ int main(int argc, char **argv, char **envp)
#endif
#if defined(CONFIG_GTK)
case DT_GTK:
-gtk_display_init(ds);
+gtk_display_init(ds, full_screen, no_frame);
break;
#endif
default:
--
1.8.3
[Qemu-devel] [PATCH] chardev: add baud parameter for serial host device
When QEMU starts, it always changes the serial port parameters including baud
rate. This confused my guest which thought it was outputting at 9600 baud while
it was in fact changed to 115200.
After this patch, I can use `-serial /dev/ttyS0,baud=9600` to override the
default baud rate of 115200. Documentation is updated as well, so that users
know about the new `baud` parameter for `-serial` and `-chardev serial` (and its
alias `-chardev tty`).
Note that the baud option is not implemented for Windows. QEMU does not change
the default baud rate on Windows anyway. If somebody is going to implement it,
do not forget to update the documentation on "COM" devices which is also of
backend serial.
Signed-off-by: Peter Wu
---
qapi-schema.json | 18 --
qemu-char.c | 14 --
qemu-options.hx | 14 +-
3 files changed, 33 insertions(+), 13 deletions(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index 5ad6894..f76bc0c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3186,7 +3186,7 @@
# Configuration info for device and pipe chardevs.
#
# @device: The name of the special file for the device,
-# i.e. /dev/ttyS0 on Unix or COM1: on Windows
+# i.e. /dev/parport0 on Unix.
# @type: What kind of device this is.
#
# Since: 1.4
@@ -3194,6 +3194,20 @@
{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str' } }
##
+# @ChardevSerial
+#
+# Configuration info for serial chardevs.
+#
+# @device: The name of the special file for the device,
+# i.e. /dev/ttyS0 on Unix or COM1: on Windows
+# @baud: #optional baud rate to set for host device. (default 115200)
+#
+# Since: 1.5
+##
+{ 'type': 'ChardevSerial', 'data': { 'device' : 'str',
+ '*baud': 'int' } }
+
+##
# @ChardevSocket:
#
# Configuration info for (stream) socket chardevs.
@@ -3311,7 +3325,7 @@
{ 'type': 'ChardevDummy', 'data': { } }
{ 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile',
- 'serial' : 'ChardevHostdev',
+ 'serial' : 'ChardevSerial',
'parallel': 'ChardevHostdev',
'pipe' : 'ChardevHostdev',
'socket' : 'ChardevSocket',
diff --git a/qemu-char.c b/qemu-char.c
index d04b429..421730a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1450,11 +1450,11 @@ static void qemu_chr_close_tty(CharDriverState *chr)
}
}
-static CharDriverState *qemu_chr_open_tty_fd(int fd)
+static CharDriverState *qemu_chr_open_tty_fd(int fd, int baud)
{
CharDriverState *chr;
-tty_serial_init(fd, 115200, 'N', 8, 1);
+tty_serial_init(fd, baud, 'N', 8, 1);
chr = qemu_chr_open_fd(fd, fd);
chr->chr_ioctl = tty_serial_ioctl;
chr->chr_close = qemu_chr_close_tty;
@@ -3155,13 +3155,15 @@ static void qemu_chr_parse_serial(QemuOpts *opts,
ChardevBackend *backend,
Error **errp)
{
const char *device = qemu_opt_get(opts, "path");
+const int baud = qemu_opt_get_number(opts, "baud", 115200);
if (device == NULL) {
error_setg(errp, "chardev: serial/tty: no device path given");
return;
}
-backend->serial = g_new0(ChardevHostdev, 1);
+backend->serial = g_new0(ChardevSerial, 1);
backend->serial->device = g_strdup(device);
+backend->serial->baud = baud;
}
static void qemu_chr_parse_parallel(QemuOpts *opts, ChardevBackend *backend,
@@ -3590,7 +3592,7 @@ static CharDriverState *qmp_chardev_open_file(ChardevFile
*file, Error **errp)
return qemu_chr_open_win_file(out);
}
-static CharDriverState *qmp_chardev_open_serial(ChardevHostdev *serial,
+static CharDriverState *qmp_chardev_open_serial(ChardevSerial *serial,
Error **errp)
{
return qemu_chr_open_win_path(serial->device);
@@ -3639,7 +3641,7 @@ static CharDriverState *qmp_chardev_open_file(ChardevFile
*file, Error **errp)
return qemu_chr_open_fd(in, out);
}
-static CharDriverState *qmp_chardev_open_serial(ChardevHostdev *serial,
+static CharDriverState *qmp_chardev_open_serial(ChardevSerial *serial,
Error **errp)
{
#ifdef HAVE_CHARDEV_TTY
@@ -3650,7 +3652,7 @@ static CharDriverState
*qmp_chardev_open_serial(ChardevHostdev *serial,
return NULL;
}
qemu_set_nonblock(fd);
-return qemu_chr_open_tty_fd(fd);
+return qemu_chr_open_tty_fd(fd, serial->baud);
#else
Re: [Qemu-devel] [PATCH] qemu-doc, help: update options/keys for GTK/SDL
Thanks for your feedback, replies are inlined below.
On Tuesday 04 June 2013 07:57:38 Michael Tokarev wrote:
> 03.06.2013 14:54, Peter Wu wrote:
> > The GTK display type has been introduced in 1.5, replacing SDL as default.
> > Some options only work with SDL and not GTK. This patch tries to address
> > them.
> >
> > The `-display` option is updated with the new `gtk` option.
> >
> > The `-alt-grab` and `-ctrl-alt` options are not possible in GTK according
> > to commit 5104a1f65088285ddf870aa641b9061064e8757d, this is now mentioned
> > explicitly. Another related change is that only Ctrl-Alt-G can be used
> > for (un)grbabing.
> >
> > Window scaling seems also to be a SDL-specific feature and has been marked
> > as such.
>
> This is a clear improvement. However, we fail to mention VNC everywhere,
> which actually works pretty much like SDL. This might go as a separate
> change however...
I did not know that the monitor could be used inside the VNC UI. I will try to
update the docs with it. Aside, the `-display vnc=:0` option looks odd and
inconsistent with other options. It would look saner if something like `-
display vnc,display=:0` was possible.
While we are at it, the `-display sdl` option contains:
"...see the SDL documentation for other possibilities..."
Which documentation is meant? External documentation on the sdl website? The
part about Ctrl-Alt-F? The `-sdl` option, which is extensively documented?
("Enable SDL."). Or.. the `qemu-system-x86_64 --help` output? Shall I change
something like `@item sdl` to `@item sdl[,frame=on|off],...` for clarity in the
manual page?
> > During the graphical emulation, you can use special key combinations to
> > change>
> > -modes. The default key mappings are shown below, but if you use
> > @code{-alt-grab} -then the modifier is Ctrl-Alt-Shift (instead of
> > Ctrl-Alt) and if you use -@code{-ctrl-grab} then the modifier is the
> > right Ctrl key (instead of Ctrl-Alt): +modes. The default key mappings
> > are shown below. If you are using SDL display +mode instead of GTK, you
> > can use @code{-alt-grab} to change the modifier to +Ctrl-Alt-Shift
> > (instead of Ctrl-Alt) and @code{-ctrl-grab} to change it to right
> > +Ctrl key (instead of Ctrl-Alt):
> I'd made a reference to -display option here, and s/SDL display mode/SDL
> display/.
Done, I now explicitly note that it was made for sdl and mentioned the `-
display sdl` option.
> Not a comment about this particular patch, but I think we should disallow
> -alt-grab for modes where it makes no sense (or at least warn about that
> usage). For example, as stated, -alt-grab does not work for gtk, so we
> may as well warn in case `-display gtk -alt-grab'. Ditto for -display none
> maybe.
A warning message would be nice indeed. The option should be ignored, not
abort the startup as it may break configurations (in theory).
> -alt-grab is also ignored by vnc -- should we change/fix that?
As is -ctrl-grab, what about keeping it as-is? VNC is remote anyway, if -*-
grab does not change the default ctrl-alt modifier, it will provide a
consistent experience. I have no strong preference on this though.
> > @table @key
> > @item Ctrl-Alt-f
> >
> > @@ -294,7 +295,8 @@ Shrink the screen
> >
> > @item Ctrl-Alt-u
> > @kindex Ctrl-Alt-u
> >
> > -Restore the screen's un-scaled dimensions
> > +Restore the screen's un-scaled dimensions (SDL windows only, GTK does not
> > +feature window scaling)
> >
> > @item Ctrl-Alt-n
> > @kindex Ctrl-Alt-n
> >
> > @@ -308,16 +310,20 @@ Monitor
> >
> > Serial port
> > @end table
> >
> > +@item Ctrl-Alt-G
> > +@kindex Ctrl-Alt-G
> > +Toggle mouse and keyboard grab in GTK mode.
> > +
> >
> > @item Ctrl-Alt
> > @kindex Ctrl-Alt
> >
> > -Toggle mouse and keyboard grab.
> > +Toggle mouse and keyboard grab in SDL windows.
> >
> > @end table
> >
> > @kindex Ctrl-Up
> > @kindex Ctrl-Down
> > @kindex Ctrl-PageUp
> > @kindex Ctrl-PageDown
> >
> > -In the virtual consoles, you can use @key{Ctrl-Up}, @key{Ctrl-Down},
> > +In the virtual consoles of SDL, you can use @key{Ctrl-Up},
> > @key{Ctrl-Down},>
> > @key{Ctrl-PageUp} and @key{Ctrl-PageDown} to move in the back log.
>
> is it sdl-specific?
Apparently yes. In GTK, Ctrl-Up results in "5A" being inserted, Ctrl-Down in
"5B" and similar results for PageUp/PageDown. In VNC, pressing those keys have
no effect.
> > @kindex Ctrl-a h
> >
> > diff --git a/qemu-options.hx b/qemu-optio
Re: [Qemu-devel] QEMU aborts since "kvm: support using KVM_MEM_READONLY flag for regions"
On Monday 03 June 2013 10:35:28 Jordan Justen wrote: > This should be fixed by Xiao's "fix double free the memslot in > kvm_set_phys_mem" patch. Confirmed, with this one-liner patch the bug is gone. > On Mon, Jun 3, 2013 at 10:23 AM, Peter Wu wrote: > > Hi, > > > > With the current git source (f10acc8b38d65a66ffa0588a036489d7fa6a593e), > > > > `qemu-system-x86_64 -enable-kvm` fails to run with the following error: > > kvm_set_phys_mem: error unregistering overlapping slot: Invalid > > argument > > > > After this message, qemu aborts. Reverting the following commit on top of > > master makes the bug go away. Removing the `-enable-kvm` option also > > allows QEMU to operate, but without KVM. > > > > commit 235e8982ad393e5611cb892df54881c872eea9e1 > > Author: Jordan Justen > > Date: Wed May 29 01:27:26 2013 -0700 > > > > kvm: support using KVM_MEM_READONLY flag for regions > > > > For readonly memory regions and rom devices in romd_mode, > > we make use of the KVM_MEM_READONLY. A slot that uses > > KVM_MEM_READONLY can be read from and code can execute from the > > region, but writes will exit to qemu. > > > > For rom devices with !romd_mode, we force the slot to be > > removed so reads or writes to the region will exit to qemu. > > (Note that a memory region in this state is not executable > > within kvm.) > > > > QEMU was built using GCC 4.8.0, runs on Arch Linux 64-bit with a > > custom kernel, Linux 3.9.1. CPU: i5-460M. A backtrace is provided > > on the bottom of this message. If you need more details, please ask. > > > > Regards, > > Peter > > > > #0 0x71a251c9 in raise () from /usr/lib/libc.so.6 > > #1 0x71a265c8 in abort () from /usr/lib/libc.so.6 > > #2 0x557dd84d in kvm_set_phys_mem (section=0x7fffe6fdca00, > > add=false) at /tmp/qemu/kvm-all.c:715 #3 0x557e132c in > > address_space_update_topology_pass (as=as@entry=0x564dae00 > > , adding=adding@entry=false, old_view=..., > > new_view=...) at /tmp/qemu/memory.c:725 #4 0x557e1f13 in > > address_space_update_topology (as=0x564dae00 ) > > at /tmp/qemu/memory.c:761 #5 memory_region_transaction_commit () at > > /tmp/qemu/memory.c:786 > > #6 0x557dfa42 in access_with_adjusted_size (addr=addr@entry=2, > > value=value@entry=0x7fffe6fdcc18, size=1, access_size_min= > out>, access_size_max=,> > > access=access@entry=0x557e , > > opaque=opaque@entry=0x5658c238) at /tmp/qemu/memory.c:399> > > #7 0x557e0f77 in memory_region_iorange_write (iorange= > out>, offset=2, width=1, data=51) at /tmp/qemu/memory.c:475 #8 > > 0x557de562 in kvm_handle_io (count=1, size=1, direction=1, > > data=, port=3326) at /tmp/qemu/kvm-all.c:1507 #9 > > kvm_cpu_exec (env=env@entry=0x5656f750) at /tmp/qemu/kvm-all.c:1659 > > #10 0x5578da05 in qemu_kvm_cpu_thread_fn (arg=0x5656f750) at > > /tmp/qemu/cpus.c:759 #11 0x76280dd2 in start_thread () from > > /usr/lib/libpthread.so.0 #12 0x71ad5cdd in clone () from > > /usr/lib/libc.so.6
[Qemu-devel] QEMU aborts since "kvm: support using KVM_MEM_READONLY flag for regions"
Hi, With the current git source (f10acc8b38d65a66ffa0588a036489d7fa6a593e), `qemu-system-x86_64 -enable-kvm` fails to run with the following error: kvm_set_phys_mem: error unregistering overlapping slot: Invalid argument After this message, qemu aborts. Reverting the following commit on top of master makes the bug go away. Removing the `-enable-kvm` option also allows QEMU to operate, but without KVM. commit 235e8982ad393e5611cb892df54881c872eea9e1 Author: Jordan Justen Date: Wed May 29 01:27:26 2013 -0700 kvm: support using KVM_MEM_READONLY flag for regions For readonly memory regions and rom devices in romd_mode, we make use of the KVM_MEM_READONLY. A slot that uses KVM_MEM_READONLY can be read from and code can execute from the region, but writes will exit to qemu. For rom devices with !romd_mode, we force the slot to be removed so reads or writes to the region will exit to qemu. (Note that a memory region in this state is not executable within kvm.) QEMU was built using GCC 4.8.0, runs on Arch Linux 64-bit with a custom kernel, Linux 3.9.1. CPU: i5-460M. A backtrace is provided on the bottom of this message. If you need more details, please ask. Regards, Peter #0 0x71a251c9 in raise () from /usr/lib/libc.so.6 #1 0x71a265c8 in abort () from /usr/lib/libc.so.6 #2 0x557dd84d in kvm_set_phys_mem (section=0x7fffe6fdca00, add=false) at /tmp/qemu/kvm-all.c:715 #3 0x557e132c in address_space_update_topology_pass (as=as@entry=0x564dae00 , adding=adding@entry=false, old_view=..., new_view=...) at /tmp/qemu/memory.c:725 #4 0x557e1f13 in address_space_update_topology (as=0x564dae00 ) at /tmp/qemu/memory.c:761 #5 memory_region_transaction_commit () at /tmp/qemu/memory.c:786 #6 0x557dfa42 in access_with_adjusted_size (addr=addr@entry=2, value=value@entry=0x7fffe6fdcc18, size=1, access_size_min=, access_size_max=, access=access@entry=0x557e , opaque=opaque@entry=0x5658c238) at /tmp/qemu/memory.c:399 #7 0x557e0f77 in memory_region_iorange_write (iorange=, offset=2, width=1, data=51) at /tmp/qemu/memory.c:475 #8 0x557de562 in kvm_handle_io (count=1, size=1, direction=1, data=, port=3326) at /tmp/qemu/kvm-all.c:1507 #9 kvm_cpu_exec (env=env@entry=0x5656f750) at /tmp/qemu/kvm-all.c:1659 #10 0x5578da05 in qemu_kvm_cpu_thread_fn (arg=0x5656f750) at /tmp/qemu/cpus.c:759 #11 0x76280dd2 in start_thread () from /usr/lib/libpthread.so.0 #12 0x71ad5cdd in clone () from /usr/lib/libc.so.6
[Qemu-devel] [PATCH] qemu-doc, help: update options/keys for GTK/SDL
The GTK display type has been introduced in 1.5, replacing SDL as default. Some
options only work with SDL and not GTK. This patch tries to address them.
The `-display` option is updated with the new `gtk` option.
The `-alt-grab` and `-ctrl-alt` options are not possible in GTK according to
commit 5104a1f65088285ddf870aa641b9061064e8757d, this is now mentioned
explicitly. Another related change is that only Ctrl-Alt-G can be used for
(un)grbabing.
Window scaling seems also to be a SDL-specific feature and has been marked as
such.
Signed-off-by: Peter Wu
---
qemu-doc.texi | 18 --
qemu-options.hx | 28
2 files changed, 28 insertions(+), 18 deletions(-)
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 8022890..76bb4cd 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -275,9 +275,10 @@ targets do not need a disk image.
@c man begin OPTIONS
During the graphical emulation, you can use special key combinations to change
-modes. The default key mappings are shown below, but if you use
@code{-alt-grab}
-then the modifier is Ctrl-Alt-Shift (instead of Ctrl-Alt) and if you use
-@code{-ctrl-grab} then the modifier is the right Ctrl key (instead of
Ctrl-Alt):
+modes. The default key mappings are shown below. If you are using SDL display
+mode instead of GTK, you can use @code{-alt-grab} to change the modifier to
+Ctrl-Alt-Shift (instead of Ctrl-Alt) and @code{-ctrl-grab} to change it to
right
+Ctrl key (instead of Ctrl-Alt):
@table @key
@item Ctrl-Alt-f
@@ -294,7 +295,8 @@ Shrink the screen
@item Ctrl-Alt-u
@kindex Ctrl-Alt-u
-Restore the screen's un-scaled dimensions
+Restore the screen's un-scaled dimensions (SDL windows only, GTK does not
+feature window scaling)
@item Ctrl-Alt-n
@kindex Ctrl-Alt-n
@@ -308,16 +310,20 @@ Monitor
Serial port
@end table
+@item Ctrl-Alt-G
+@kindex Ctrl-Alt-G
+Toggle mouse and keyboard grab in GTK mode.
+
@item Ctrl-Alt
@kindex Ctrl-Alt
-Toggle mouse and keyboard grab.
+Toggle mouse and keyboard grab in SDL windows.
@end table
@kindex Ctrl-Up
@kindex Ctrl-Down
@kindex Ctrl-PageUp
@kindex Ctrl-PageDown
-In the virtual consoles, you can use @key{Ctrl-Up}, @key{Ctrl-Down},
+In the virtual consoles of SDL, you can use @key{Ctrl-Up}, @key{Ctrl-Down},
@key{Ctrl-PageUp} and @key{Ctrl-PageDown} to move in the back log.
@kindex Ctrl-a h
diff --git a/qemu-options.hx b/qemu-options.hx
index bf94862..f7b0172 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -338,7 +338,7 @@ STEXI
@item -name @var{name}
@findex -name
Sets the @var{name} of the guest.
-This name will be displayed in the SDL window caption.
+This name will be displayed in the SDL and GTK window captions.
The @var{name} will also be used for the VNC server.
Also optionally set the top visible process name in Linux.
ETEXI
@@ -804,7 +804,7 @@ ETEXI
DEF("display", HAS_ARG, QEMU_OPTION_display,
"-display sdl[,frame=on|off][,alt_grab=on|off][,ctrl_grab=on|off]\n"
-"[,window_close=on|off]|curses|none|\n"
+"[,window_close=on|off]|gtk|curses|none|\n"
"vnc=[,]\n"
"select display type\n", QEMU_ARCH_ALL)
STEXI
@@ -813,6 +813,8 @@ STEXI
Select type of display to use. This option is a replacement for the
old style -sdl/-curses/... options. Valid values for @var{type} are
@table @option
+@item gtk
+Display video output via GTK which includes a menu bar.
@item sdl
Display video output via SDL (usually in a separate graphics
window; see the SDL documentation for other possibilities).
@@ -839,7 +841,7 @@ DEF("nographic", 0, QEMU_OPTION_nographic,
STEXI
@item -nographic
@findex -nographic
-Normally, QEMU uses SDL to display the VGA output. With this option,
+Normally, QEMU uses GTK to display the VGA output. With this option,
you can totally disable graphical output so that QEMU is a simple
command line application. The emulated serial port is redirected on
the console. Therefore, you can still use QEMU to debug a Linux kernel
@@ -852,7 +854,7 @@ DEF("curses", 0, QEMU_OPTION_curses,
STEXI
@item -curses
@findex -curses
-Normally, QEMU uses SDL to display the VGA output. With this option,
+Normally, QEMU uses GTK to display the VGA output. With this option,
QEMU can display the VGA output when in text mode using a
curses/ncurses interface. Nothing is displayed in graphical mode.
ETEXI
@@ -869,27 +871,29 @@ workspace more convenient.
ETEXI
DEF("alt-grab", 0, QEMU_OPTION_alt_grab,
-"-alt-grab use Ctrl-Alt-Shift to grab mouse (instead of Ctrl-Alt)\n",
+"-alt-grab use Ctrl-Alt-Shift to grab mouse in SDL (instead of
Ctrl-Alt)\n",
QEMU_ARCH_ALL)
STEXI
@item -alt-grab
@findex -alt-grab
-Use Ctrl-Alt-Shift to grab mouse (instead of Ctrl-Alt). Note that this also
-affects the special keys (for fullscreen, monitor-mod
