Re: [PATCH v2] mailmap: Add entry for Yu-Chen Lin
Acked-by: Yu-Chen Lin -- Best Regards, Yu-Chen Lin #8243 Yu-Chen Lin 於 2020-02-06 20:55 寫道： > > I have two mail address, add entries for > showing author and email correctly. > > Signed-off-by: Yu-Chen Lin > --- > v1 -> v2: > * Change subject > > .mailmap | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/.mailmap b/.mailmap > index 3816e4effe..3fbf3902a3 100644 > --- a/.mailmap > +++ b/.mailmap > @@ -151,7 +151,8 @@ Xiaoqiang Zhao > Xinhua Cao > Xiong Zhang > Yin Yin > -yuchenlin > +Yu-Chen Lin > +Yu-Chen Lin > YunQiang Su > YunQiang Su > Yuri Pudgorodskiy > -- > 2.17.1 >
[Bug 1849644] Re: QEMU VNC websocket proxy requires non-standard 'binary' subprotocol
Hi, Samuel This patch has been viewed by Daniel. Please see https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00264.html However, Daniel has not sent a PR which includes this patch. Thanks. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1849644 Title: QEMU VNC websocket proxy requires non-standard 'binary' subprotocol Status in QEMU: New Bug description: When running a machine using "-vnc" and the "websocket" option QEMU seems to require the subprotocol called 'binary'. This subprotocol does not exist in the WebSocket specification. In fact it has never existed in the spec, in one of the very early drafts of WebSockets it was briefly mentioned but it never made it to a final version. When the WebSocket server requires a non-standard subprotocol any WebSocket client that works correctly won't be able to connect. One example of such a client is noVNC, it tells the server that it doesn't want to use any subprotocol. QEMU's WebSocket proxy doesn't let noVNC connect. If noVNC is modified to ask for 'binary' it will work, this is, however, incorrect behavior. Looking at the code in "io/channel-websock.c" it seems it's quite hard-coded to binary: Look at line 58 and 433 here: https://git.qemu.org/?p=qemu.git;a=blob;f=io/channel-websock.c This code has to be made more dynamic, and shouldn't require binary. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1849644/+subscriptions
[Bug 1849644] Re: QEMU VNC websocket proxy requires non-standard 'binary' subprotocol
I have sent a patch about this problem. Please see https://lists.nongnu.org/archive/html/qemu- devel/2019-11/msg03924.html -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1849644 Title: QEMU VNC websocket proxy requires non-standard 'binary' subprotocol Status in QEMU: New Bug description: When running a machine using "-vnc" and the "websocket" option QEMU seems to require the subprotocol called 'binary'. This subprotocol does not exist in the WebSocket specification. In fact it has never existed in the spec, in one of the very early drafts of WebSockets it was briefly mentioned but it never made it to a final version. When the WebSocket server requires a non-standard subprotocol any WebSocket client that works correctly won't be able to connect. One example of such a client is noVNC, it tells the server that it doesn't want to use any subprotocol. QEMU's WebSocket proxy doesn't let noVNC connect. If noVNC is modified to ask for 'binary' it will work, this is, however, incorrect behavior. Looking at the code in "io/channel-websock.c" it seems it's quite hard-coded to binary: Look at line 58 and 433 here: https://git.qemu.org/?p=qemu.git;a=blob;f=io/channel-websock.c This code has to be made more dynamic, and shouldn't require binary. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1849644/+subscriptions
Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] vhost-user-blk: prevent using uninitialized vqs
Raphael Norwitz 於 2019-08-23 04:16 寫道： > > Same rational as: e6cc11d64fc998c11a4dfcde8fda3fc33a74d844 > > Of the 3 virtqueues, seabios only sets cmd, leaving ctrl > and event without a physical address. This can cause > vhost_verify_ring_part_mapping to return ENOMEM, causing > the following logs: > > qemu-system-x86_64: Unable to map available ring for ring 0 > qemu-system-x86_64: Verify ring failure on region 0 > > This has already been fixed for vhost scsi devices and was > recently vhost-user scsi devices. This commit fixes it for > vhost-user-blk devices. > > Suggested-by: Phillippe Mathieu-Daude > Signed-off-by: Raphael Norwitz Reviewed-by: yuchenlin Thanks. > > > --- > hw/block/vhost-user-blk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c > index 0b8c5df..63da9bb 100644 > --- a/hw/block/vhost-user-blk.c > +++ b/hw/block/vhost-user-blk.c > @@ -421,7 +421,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) > } > > s->inflight = g_new0(struct vhost_inflight, 1); > - s->vqs = g_new(struct vhost_virtqueue, s->num_queues); > + s->vqs = g_new0(struct vhost_virtqueue, s->num_queues); > s->watch = 0; > s->connected = false; > > -- > 1.9.4 > >
[Qemu-devel] [Bug 1809304] Re: qemu-img convert is freezing for some DMG files.
I re-test the dmg img with QEMU 4.0 again, and it works. In my opinion, the bug can be closed as "Fix released". Thanks. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1809304 Title: qemu-img convert is freezing for some DMG files. Status in QEMU: Fix Committed Bug description: Recently, I created a file using hdiutil from MacOS (using Zlib compression): $ hdiutil create -volname MyVolName -srcfolder /path/to/my/vol/ -ov -format UDZO myvolname.dmg But, when I try to convert this volume using qemu-img convert, this command is freezing. I'm using the upstream version to test it. It is freezing inside the binary search method to retrieve the chunk. But, I still don't know why. I'm attaching the file as an example. It can be mounted using MacOS or other Linux apps like hfsleuth and darling-dmg. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1809304/+subscriptions
Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] vmdk: Fix comment regarding max l1_size coverage
On 2019-04-24 15:49, Sam Eiderman wrote:
Commit b0651b8c246d ("vmdk: Move l1_size check into vmdk_add_extent")
extended the l1_size check from VMDK4 to VMDK3 but did not update the
default coverage in the moved comment.
The previous vmdk4 calculation:
(512 * 1024 * 1024) * 512(l2 entries) * 65536(grain) = 16PB
The added vmdk3 calculation:
(512 * 1024 * 1024) * 4096(l2 entries) * 512(grain) = 1PB
Adding the calculation of vmdk3 to the comment.
In any case, VMware does not offer virtual disks more than 2TB for
vmdk4/vmdk3 or 64TB for the new undocumented seSparse format which is
not implemented yet in qemu.
Reviewed-by: Karl Heubaum
Reviewed-by: Eyal Moscovici
Reviewed-by: Liran Alon
Reviewed-by: Arbel Moshe
Signed-off-by: Sam Eiderman
---
block/vmdk.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index de8cb859f8..fc7378da78 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -426,10 +426,15 @@ static int vmdk_add_extent(BlockDriverState *bs,
return -EFBIG;
}
if (l1_size > 512 * 1024 * 1024) {
-/* Although with big capacity and small l1_entry_sectors, we
can get a
+/*
+ * Although with big capacity and small l1_entry_sectors, we
can get a
* big l1_size, we don't want unbounded value to allocate the
table.
- * Limit it to 512M, which is 16PB for default cluster and L2
table
- * size */
+ * Limit it to 512M, which is:
+ * 16PB - for default "Hosted Sparse Extent" (VMDK4)
+ *cluster size: 64KB, L2 table size: 512 entries
+ * 1PB - for default "ESXi Host Sparse Extent"
(VMDK3/vmfsSparse)
+ *cluster size: 512B, L2 table size: 4096 entries
+ */
error_setg(errp, "L1 size too big");
return -EFBIG;
}
The calculation of VMDK3 can be verified in end of page No.9 of the spec
(https://www.vmware.com/support/developer/vddk/vmdk_50_technote.pdf).
Also the VMDK4 can be checked in the section Grain Table and Grain in
page No.8 of the spec.
Reviewed-by: yuchenlin
Re: [Qemu-devel] [PATCH] e1000: Delay flush queue when receive RCTL
On 2019-03-25 12:26, Jason Wang wrote:
On 2019/3/21 上午9:35, yuchenlin wrote:
Ping?
On 2019-03-13 14:56, yuchen...@synology.com wrote:
From: yuchenlin
Due to too early RCT0 interrput, win10x32 may hang on booting.
This problem can be reproduced by doing power cycle on win10x32
guest.
In our environment, we have 10 win10x32 and stress power cycle.
The problem will happen about 20 rounds.
Below shows some log with comment:
The normal case:
22831@1551928392.984687:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
22831@1551928392.985655:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
22831@1551928392.985801:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
22831@1551928393.056710:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: ICR read: 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
22831@1551928393.077548:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: ICR read: 0
e1000: set_ics 2, ICR 0, IMR 0
e1000: set_ics 2, ICR 2, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
22831@1551928393.102974:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
22831@1551928393.103267:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: RCTL: 255, mac_reg[RCTL] = 0x40002 <- win10x32 says it can
handle
RX now
e1000: set_ics 0, ICR 2, IMR 9d <- unmask interrupt
e1000: RCTL: 255, mac_reg[RCTL] = 0x48002
e1000: set_ics 80, ICR 2, IMR 9d <- interrupt and work!
...
The bad case:
27744@1551930483.117766:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
27744@1551930483.118398:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
27744@1551930483.198063:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: ICR read: 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
27744@1551930483.218675:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: ICR read: 0
e1000: set_ics 2, ICR 0, IMR 0
e1000: set_ics 2, ICR 2, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
27744@1551930483.241768:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
27744@1551930483.241979:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: RCTL: 255, mac_reg[RCTL] = 0x40002 <- win10x32 says it can
handle
RX now
e1000: set_ics 80, ICR 2, IMR 0 <- flush queue (caused by setting
RCTL)
e1000: set_ics 0, ICR 82, IMR 9d <- unmask interrupt and because
0x82&0x9d
!= 0 generate interrupt, hang on here...
Do you mean the interrupt handler is not ready in guest actually?
From my observation, I think yes.
We used to have similar workarounds like autoneg timer, I wonder if we
can reuse that.
IMO, we can't re-use the autoneg timer. The autoneg seems not always be
triggered.
Thanks
Thanks
To workaround this problem, simply delay flush queue. Also stop
receiving
when timer is going to run.
Tested on CentOS, Win7SP1x64 and Win10x32.
Signed-off-by: yuchenlin
---
hw/net/e1000.c | 24 ++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 5e144cb4e4..9b39bccfb2 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -120,6 +120,8 @@ typedef struct E1000State_st {
bool mit_irq_level; /* Tracks interrupt pin level. */
uint32_t mit_ide; /* Tracks E1000_TXD_CMD_IDE bit. */
+ QEMUTimer *flush_queue_timer;
+
/* Compatibility flags for migration to/from qemu 1.3.0 and older */
#define E1000_FLAG_AUTONEG_BIT 0
#define E1000_FLAG_MIT_BIT 1
@@ -366,6 +368,7 @@ static void e1000_reset(void *opaque)
timer_del(d->autoneg_timer);
timer_del(d->mit_timer);
+ timer_del(d->flush_queue_timer);
d->mit_timer_on = 0;
d->mit_irq_level = 0;
d->mit_ide = 0;
@@ -391,6 +394,14 @@ set_ctrl(E1000State *s, int index, uint32_t val)
s->mac_reg[CTRL] = val & ~E1000_CTRL_RST;
}
+static void
+e1000_flush_queue_timer(void *opaque)
+{
+ E1000State *s = opaque;
+
+ qemu_flush_queued_packets(qemu_get_queue(s->nic));
+}
+
static void
set_rx_control(E1000State *s, int index, uint32_t val)
{
@@ -399,7 +410,8 @@ set_rx_control(E1000State *s, int index, uint32_t
val)
s->rxbuf_min_shift = ((val / E1000_RCTL_RDMTS_QU
Re: [Qemu-devel] [PATCH] e1000: Delay flush queue when receive RCTL
Ping?
On 2019-03-13 14:56, yuchen...@synology.com wrote:
From: yuchenlin
Due to too early RCT0 interrput, win10x32 may hang on booting.
This problem can be reproduced by doing power cycle on win10x32 guest.
In our environment, we have 10 win10x32 and stress power cycle.
The problem will happen about 20 rounds.
Below shows some log with comment:
The normal case:
22831@1551928392.984687:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
22831@1551928392.985655:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
22831@1551928392.985801:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
22831@1551928393.056710:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: ICR read: 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
22831@1551928393.077548:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: ICR read: 0
e1000: set_ics 2, ICR 0, IMR 0
e1000: set_ics 2, ICR 2, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
22831@1551928393.102974:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
22831@1551928393.103267:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: RCTL: 255, mac_reg[RCTL] = 0x40002 <- win10x32 says it can
handle
RX now
e1000: set_ics 0, ICR 2, IMR 9d <- unmask interrupt
e1000: RCTL: 255, mac_reg[RCTL] = 0x48002
e1000: set_ics 80, ICR 2, IMR 9d <- interrupt and work!
...
The bad case:
27744@1551930483.117766:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
27744@1551930483.118398:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
27744@1551930483.198063:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: ICR read: 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
27744@1551930483.218675:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: ICR read: 0
e1000: set_ics 2, ICR 0, IMR 0
e1000: set_ics 2, ICR 2, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
27744@1551930483.241768:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
27744@1551930483.241979:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: RCTL: 255, mac_reg[RCTL] = 0x40002 <- win10x32 says it can
handle
RX now
e1000: set_ics 80, ICR 2, IMR 0 <- flush queue (caused by setting RCTL)
e1000: set_ics 0, ICR 82, IMR 9d <- unmask interrupt and because
0x82&0x9d
!= 0 generate interrupt, hang on here...
To workaround this problem, simply delay flush queue. Also stop
receiving
when timer is going to run.
Tested on CentOS, Win7SP1x64 and Win10x32.
Signed-off-by: yuchenlin
---
hw/net/e1000.c | 24 ++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 5e144cb4e4..9b39bccfb2 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -120,6 +120,8 @@ typedef struct E1000State_st {
bool mit_irq_level;/* Tracks interrupt pin level. */
uint32_t mit_ide; /* Tracks E1000_TXD_CMD_IDE bit. */
+QEMUTimer *flush_queue_timer;
+
/* Compatibility flags for migration to/from qemu 1.3.0 and older */
#define E1000_FLAG_AUTONEG_BIT 0
#define E1000_FLAG_MIT_BIT 1
@@ -366,6 +368,7 @@ static void e1000_reset(void *opaque)
timer_del(d->autoneg_timer);
timer_del(d->mit_timer);
+timer_del(d->flush_queue_timer);
d->mit_timer_on = 0;
d->mit_irq_level = 0;
d->mit_ide = 0;
@@ -391,6 +394,14 @@ set_ctrl(E1000State *s, int index, uint32_t val)
s->mac_reg[CTRL] = val & ~E1000_CTRL_RST;
}
+static void
+e1000_flush_queue_timer(void *opaque)
+{
+E1000State *s = opaque;
+
+qemu_flush_queued_packets(qemu_get_queue(s->nic));
+}
+
static void
set_rx_control(E1000State *s, int index, uint32_t val)
{
@@ -399,7 +410,8 @@ set_rx_control(E1000State *s, int index, uint32_t
val)
s->rxbuf_min_shift = ((val / E1000_RCTL_RDMTS_QUAT) & 3) + 1;
DBGOUT(RX, "RCTL: %d, mac_reg[RCTL] = 0x%x\n", s->mac_reg[RDT],
s->mac_reg[RCTL]);
-qemu_flush_queued_packets(qemu_get_queue(s->nic));
+timer_mod(s->flush_queue_timer,
+ qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 1000);
}
static void
@@ -837,7 +849,7 @@ e1000_can_receive(NetClientState *nc)
E1000
[Qemu-devel] [PATCH] e1000: Delay flush queue when receive RCTL
From: yuchenlin
Due to too early RCT0 interrput, win10x32 may hang on booting.
This problem can be reproduced by doing power cycle on win10x32 guest.
In our environment, we have 10 win10x32 and stress power cycle.
The problem will happen about 20 rounds.
Below shows some log with comment:
The normal case:
22831@1551928392.984687:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
22831@1551928392.985655:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
22831@1551928392.985801:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
22831@1551928393.056710:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: ICR read: 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
22831@1551928393.077548:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: ICR read: 0
e1000: set_ics 2, ICR 0, IMR 0
e1000: set_ics 2, ICR 2, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
22831@1551928393.102974:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
22831@1551928393.103267:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: RCTL: 255, mac_reg[RCTL] = 0x40002 <- win10x32 says it can handle
RX now
e1000: set_ics 0, ICR 2, IMR 9d <- unmask interrupt
e1000: RCTL: 255, mac_reg[RCTL] = 0x48002
e1000: set_ics 80, ICR 2, IMR 9d <- interrupt and work!
...
The bad case:
27744@1551930483.117766:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
27744@1551930483.118398:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
27744@1551930483.198063:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: ICR read: 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
27744@1551930483.218675:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: set_ics 0, ICR 0, IMR 0
e1000: ICR read: 0
e1000: set_ics 2, ICR 0, IMR 0
e1000: set_ics 2, ICR 2, IMR 0
e1000: RCTL: 0, mac_reg[RCTL] = 0x0
27744@1551930483.241768:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
27744@1551930483.241979:e1000x_rx_disabled Received packet dropped
because receive is disabled RCTL = 0
e1000: RCTL: 255, mac_reg[RCTL] = 0x40002 <- win10x32 says it can handle
RX now
e1000: set_ics 80, ICR 2, IMR 0 <- flush queue (caused by setting RCTL)
e1000: set_ics 0, ICR 82, IMR 9d <- unmask interrupt and because 0x82&0x9d
!= 0 generate interrupt, hang on here...
To workaround this problem, simply delay flush queue. Also stop receiving
when timer is going to run.
Tested on CentOS, Win7SP1x64 and Win10x32.
Signed-off-by: yuchenlin
---
hw/net/e1000.c | 24 ++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 5e144cb4e4..9b39bccfb2 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -120,6 +120,8 @@ typedef struct E1000State_st {
bool mit_irq_level;/* Tracks interrupt pin level. */
uint32_t mit_ide; /* Tracks E1000_TXD_CMD_IDE bit. */
+QEMUTimer *flush_queue_timer;
+
/* Compatibility flags for migration to/from qemu 1.3.0 and older */
#define E1000_FLAG_AUTONEG_BIT 0
#define E1000_FLAG_MIT_BIT 1
@@ -366,6 +368,7 @@ static void e1000_reset(void *opaque)
timer_del(d->autoneg_timer);
timer_del(d->mit_timer);
+timer_del(d->flush_queue_timer);
d->mit_timer_on = 0;
d->mit_irq_level = 0;
d->mit_ide = 0;
@@ -391,6 +394,14 @@ set_ctrl(E1000State *s, int index, uint32_t val)
s->mac_reg[CTRL] = val & ~E1000_CTRL_RST;
}
+static void
+e1000_flush_queue_timer(void *opaque)
+{
+E1000State *s = opaque;
+
+qemu_flush_queued_packets(qemu_get_queue(s->nic));
+}
+
static void
set_rx_control(E1000State *s, int index, uint32_t val)
{
@@ -399,7 +410,8 @@ set_rx_control(E1000State *s, int index, uint32_t val)
s->rxbuf_min_shift = ((val / E1000_RCTL_RDMTS_QUAT) & 3) + 1;
DBGOUT(RX, "RCTL: %d, mac_reg[RCTL] = 0x%x\n", s->mac_reg[RDT],
s->mac_reg[RCTL]);
-qemu_flush_queued_packets(qemu_get_queue(s->nic));
+timer_mod(s->flush_queue_timer,
+ qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 1000);
}
static void
@@ -837,7 +849,7 @@ e1000_can_receive(NetClientState *nc)
E1000State *s = qemu_get_nic_opaque(nc);
re
[Qemu-devel] [PATCH] vmdk: false positive of compat6 with hwversion not set
From: yuchenlin
In vmdk_co_create_opts, when it finds hw_version is undefined, it will
set it to 4, which misleading the compat6 and hwversion in
vmdk_co_do_create. Simply set hw_version to NULL after free, let
the logic in vmdk_co_do_create to decide the value of hw_version.
This bug can be reproduced by:
$ qemu-img convert -O vmdk -o subformat=streamOptimized,compat6
/home/yuchenlin/syno.qcow2 /home/yuchenlin/syno.vmdk
qemu-img: /home/yuchenlin/syno.vmdk: error while converting vmdk:
compat6 cannot be enabled with hwversion set
Signed-off-by: yuchenlin
---
block/vmdk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 096e8eb662..e3bbd18803 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2260,7 +2260,7 @@ static int coroutine_fn vmdk_co_create_opts(const char
*filename, QemuOpts *opts
compat6 = qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false);
if (strcmp(hw_version, "undefined") == 0) {
g_free(hw_version);
-hw_version = g_strdup("4");
+hw_version = NULL;
}
fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT);
zeroed_grain = qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false);
--
2.17.1
[Qemu-devel] [PATCH] qemu-iotests: add test case for dmg
Recently, some bugs in dmg file have been fixed. To prevent reading dmg
is broken someday in the future, add a simple test which ensures the
conversion from dmg to raw should not hang or face any I/O error.
Signed-off-by: yuchenlin
---
tests/qemu-iotests/236| 53 ++
tests/qemu-iotests/236.out| 4 ++
tests/qemu-iotests/check | 7 +++
tests/qemu-iotests/group | 1 +
.../sample_images/simple-dmg.dmg.bz2 | Bin 0 -> 3479 bytes
5 files changed, 65 insertions(+)
create mode 100755 tests/qemu-iotests/236
create mode 100644 tests/qemu-iotests/236.out
create mode 100644 tests/qemu-iotests/sample_images/simple-dmg.dmg.bz2
diff --git a/tests/qemu-iotests/236 b/tests/qemu-iotests/236
new file mode 100755
index 00..6f085d573d
--- /dev/null
+++ b/tests/qemu-iotests/236
@@ -0,0 +1,53 @@
+#!/bin/bash
+#
+# Test case for dmg
+#
+# Copyright (C) 2019 yuchenlin
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=npes87...@gmail.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1 # failure is the default!
+
+_cleanup()
+{
+rm -f "$TEST_IMG.raw"
+_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+
+_supported_fmt dmg
+_supported_proto file
+_supported_os Linux
+
+echo
+echo "== Testing conversion to raw should success =="
+_use_sample_img simple-dmg.dmg.bz2
+if ! $QEMU_IMG convert -f $IMGFMT -O raw "$TEST_IMG" "$TEST_IMG.raw" ; then
+exit 1
+fi
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/236.out b/tests/qemu-iotests/236.out
new file mode 100644
index 00..a92fc657dd
--- /dev/null
+++ b/tests/qemu-iotests/236.out
@@ -0,0 +1,4 @@
+QA output created by 236
+
+== Testing conversion to raw should success ==
+*** done
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 89ed275988..895e1e3dcb 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -237,6 +237,7 @@ image format options
-vhdx test vhdx
-vmdk test vmdk
-luks test luks
+-dmgtest dmg
image protocol options
-file test file (default)
@@ -304,6 +305,12 @@ testlist options
xpand=false
;;
+-dmg)
+IMGFMT=dmg
+IMGFMT_GENERIC=false
+xpand=false
+;;
+
-qed)
IMGFMT=qed
xpand=false
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 61a6d98ebd..623ede30f5 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -233,3 +233,4 @@
233 auto quick
234 auto quick migration
235 auto quick
+236 rw auto quick
diff --git a/tests/qemu-iotests/sample_images/simple-dmg.dmg.bz2
b/tests/qemu-iotests/sample_images/simple-dmg.dmg.bz2
new file mode 100644
index
..05e719d03d48d6df4f0e995dd8bae1ce54c9f5f0
GIT binary patch
literal 3479
zcmV;I4QTR0T4*^jL0KkKS(36)s{k7pfB*mg|NsC0|NsC0|NsC0|NsC0|Ns5}|NsC0
z|NsC0|Nr0#eji@i`+e|w_jjtUioUNn9=bhi?FYz+8lea^Pt`n6Q`GfM=uI^|rccqR
z#)^4QQ(^vx-<6U8+BMo&{rPgMO9KU6K>tx+G-x7
zKxhH7gFqfr)EbhWqfmN7Xqi0_(|U$T%^~Te(<9O!s(P57)gFnX(HMqCnW^apCLkWB
zfB?c`7=Y8$P|yP*p{9YL8X6e}jSLZ{Kmat*Y9I=F1sjv(V%IjnFNxA1T<(7k~Y;aPez3F!fB%)byHqn^PtYN0c*2G*ZllN2a;wYb5Z@K!FpiLKS04
zAXF_RNbYzNTvjprs6=NSa4=(xVBCm>1#>twBnc>5NRuJv+UXVcw)$K?45Ra5gMtys
z5#=tTi{6o_Dolm>AW0*9z$7i33Gt*7^pYSUXUfIy=|9=(Y5F2D)W#);#I!)dBq)W~
zrFlJ*pf<%vEyY-2j;_z6FzSi8mQn!dfdGLU#z+u}O%{xk@FlI`
z^B@Tz!-52v0~!WxwY>yM5j2L!wDW07lb>SjKuGF1Bv7*3#(>;=WH?%<2(%C(%mi
z3SK&{tsN6|F+h=$fRF|SAT=$CM8UPyf@;hw?Ie+ur$Ms`?n=
z=TVuCRg<`%CTS#rC3Rqi{8+-#G7{}$If>{u-}tqnXQGZ1vrDn(W+-ydtwD_5(m9
zUlJm4YF$|a547Ok)VNY_1&|O?|T)
z42v8J{j@}JBoR7501!b0TQLa}5=%fGiZ01vc2PsfW3IL_R4y29^38$GN@}eFEu8;#~W3O
zhYo*p2)1qo0FWCdNC$P6JWDI)lRL3RsCq~;CddNVfDeUAK|}zQbl`V}JlvEcUSn8M
zpVIf%kFZ>u>OO0J!nRehCj34b%X*%F`iNsN=6_pV>qqFshZ5Cm#cE(tAoIY;2;n;
z>6lWK8VC|iQ`TBT@eh_haUvW?s0F~|jWOEDBrNGaBoJh|LX9lsM4vqVlOYszX7pdA
z{D&2t%ysWP%2Q6LEk#Gf-=1UdvZ5a1%=kY@b_PJ?B`u>CaLI_7b|;n=Za
zp~fR)pUI5+fv
[Qemu-devel] [PATCH v3 3/3] dmg: don't skip zero chunk
The dmg file has many tables which describe: "start from sector XXX to
sector XXX, the compression method is XXX and where the compressed data
resides on".
Each sector in the expanded file should be covered by a table. The table
will describe the offset of compressed data (or raw depends on the type)
in the dmg.
For example:
[---The expanded file]
[---bzip table ---]/* zeros */[---zlib---]
^
| if we want to read this sector.
we will find bzip table which contains this sector, and get the
compressed data offset, read it from dmg, uncompress it, finally write to
expanded file.
If we skip zero chunk (table), some sector cannot find the table which
will cause search_chunk() return s->n_chunks, dmg_read_chunk() return -1
and finally causing dmg_co_preadv() return EIO.
See:
[---The expanded file]
[---bzip table ---]/* zeros */[---zlib---]
^
| if we want to read this sector.
Oops, we cannot find the table contains it...
In the original implementation, we don't have zero table. When we try to
read sector inside the zero chunk. We will get EIO, and skip reading.
After this patch, we treat zero chunk the same as ignore chunk, it will
directly write zero and avoid some sector may not find the table.
After this patch:
[---The expanded file]
[---bzip table ---][--zeros--][---zlib---]
Signed-off-by: yuchenlin
Reviewed-by: Julio Faracco
Reviewed-by: Stefan Hajnoczi
---
block/dmg.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 9758482131..351f2991a7 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -130,7 +130,8 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t
chunk,
case UDRW: /* copy */
uncompressed_sectors = DIV_ROUND_UP(s->lengths[chunk], 512);
break;
-case UDIG: /* zero */
+case UDZE: /* zero */
+case UDIG: /* ignore */
/* 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. */
@@ -199,8 +200,9 @@ typedef struct DmgHeaderState {
static bool dmg_is_known_block_type(uint32_t entry_type)
{
switch (entry_type) {
+case UDZE:/* zeros */
case UDRW:/* uncompressed */
-case UDIG:/* zeroes */
+case UDIG:/* ignore */
case UDZO:/* zlib */
return true;
case UDBZ:/* bzip2 */
@@ -265,9 +267,10 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
/* sector count */
s->sectorcounts[i] = buff_read_uint64(buffer, offset + 0x10);
-/* all-zeroes sector (type 2) does not need to be "uncompressed" and
can
- * therefore be unbounded. */
-if (s->types[i] != UDIG && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
+/* all-zeroes sector (type UDZE and UDIG) does not need to be
+ * "uncompressed" and can therefore be unbounded. */
+if (s->types[i] != UDZE && s->types[i] != UDIG
+&& 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);
@@ -675,7 +678,8 @@ static inline int dmg_read_chunk(BlockDriverState *bs,
uint64_t sector_num)
return -1;
}
break;
-case UDIG: /* zero */
+case UDZE: /* zeros */
+case UDIG: /* ignore */
/* see dmg_read, it is treated specially. No buffer needs to be
* pre-filled, the zeroes can be set directly. */
break;
@@ -710,7 +714,8 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset,
uint64_t bytes,
/* 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] == UDIG) { /* all zeroes block entry */
+if (s->types[s->current_chunk] == UDZE
+|| s->types[s->current_chunk] == UDIG) { /* all zeroes block entry
*/
qemu_iovec_memset(qiov, i * 512, 0, 512);
continue;
}
--
2.17.1
[Qemu-devel] [PATCH v3 2/3] dmg: use enumeration type instead of hard coding number
Signed-off-by: yuchenlin
Reviewed-by: Julio Faracco
Reviewed-by: Stefan Hajnoczi
---
block/dmg.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 65a0140487..9758482131 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -267,7 +267,7 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
/* 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) {
+if (s->types[i] != UDIG && 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);
@@ -710,7 +710,7 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset,
uint64_t bytes,
/* 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 */
+if (s->types[s->current_chunk] == UDIG) { /* all zeroes block entry */
qemu_iovec_memset(qiov, i * 512, 0, 512);
continue;
}
--
2.17.1
[Qemu-devel] [PATCH v3 0/3] dmg: fixing reading in dmg
There are two bugs in dmg reading. First, it may hang in binary search. this problem is solved by patch 1. Second, because of lacking zero chunk table, reading zero sector will return EIO. this problem is solved by patch 2 and 3. Thanks v2 -> v3: * fix potential overflow (Thanks Stefan) * add Reviewed tag in patch 2 and 3 v1 -> v2: * fix typos in patch 1 * add patch 2 and patch 3 yuchenlin (3): dmg: fix binary search dmg: use enumeration type instead of hard coding number dmg: don't skip zero chunk block/dmg.c | 29 +++-- 1 file changed, 19 insertions(+), 10 deletions(-) -- 2.17.1
[Qemu-devel] [PATCH v3 1/3] dmg: fix binary search
There is a possible hang in original binary search implementation. That is
if chunk1 = 4, chunk2 = 5, chunk3 = 4, and we go else case.
The chunk1 will be still 4, and so on.
Signed-off-by: yuchenlin
---
block/dmg.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 50e91aef6d..65a0140487 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -572,16 +572,20 @@ static inline uint32_t search_chunk(BDRVDMGState *s,
uint64_t sector_num)
{
/* binary search */
uint32_t chunk1 = 0, chunk2 = s->n_chunks, chunk3;
-while (chunk1 != chunk2) {
+while (chunk1 <= chunk2) {
chunk3 = (chunk1 + chunk2) / 2;
if (s->sectors[chunk3] > sector_num) {
-chunk2 = chunk3;
+if (chunk3 == 0) {
+goto err;
+}
+chunk2 = chunk3 - 1;
} else if (s->sectors[chunk3] + s->sectorcounts[chunk3] > sector_num) {
return chunk3;
} else {
-chunk1 = chunk3;
+chunk1 = chunk3 + 1;
}
}
+err:
return s->n_chunks; /* error */
}
--
2.17.1
Re: [Qemu-devel] [Qemu-block] [PATCH] dmg: Fixing wrong dmg block type value for block terminator.
On 2018-12-28 22:50, Julio Faracco wrote:
This is a trivial patch to fix a wrong value for block terminator.
The old value was 0x7fff which is wrong. It was not affecting the
code because QEMU dmg block is not handling block terminator right now.
Neverthless, it should be fixed.
Signed-off-by: Julio Faracco
Reviewed-by: yuchenlin
---
block/dmg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/dmg.c b/block/dmg.c
index 50e91aef6d..2c806e3389 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -54,7 +54,7 @@ enum {
UDBZ,
ULFO,
UDCM = 0x7ffe, /* Comments */
-UDLE /* Last Entry */
+UDLE = 0x /* Last Entry */
};
static int dmg_probe(const uint8_t *buf, int buf_size, const char
*filename)
[Qemu-devel] [Bug 1809304] Re: qemu-img convert is freezing for some DMG files.
Because of lacking zero chunk table, reading zero sector will return EIO. I have submitted a series to fix this problem. Please refer to this series: http://lists.nongnu.org/archive/html/qemu- devel/2018-12/msg05637.html Thanks, Yu-Chen Lin -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1809304 Title: qemu-img convert is freezing for some DMG files. Status in QEMU: New Bug description: Recently, I created a file using hdiutil from MacOS (using Zlib compression): $ hdiutil create -volname MyVolName -srcfolder /path/to/my/vol/ -ov -format UDZO myvolname.dmg But, when I try to convert this volume using qemu-img convert, this command is freezing. I'm using the upstream version to test it. It is freezing inside the binary search method to retrieve the chunk. But, I still don't know why. I'm attaching the file as an example. It can be mounted using MacOS or other Linux apps like hfsleuth and darling-dmg. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1809304/+subscriptions
[Qemu-devel] [PATCH v2 1/3] dmg: fix binary search
There is a possible hang in original binary search implementation. That is
if chunk1 = 4, chunk2 = 5, chunk3 = 4, and we go else case.
The chunk1 will be still 4, and so on.
Signed-off-by: yuchenlin
---
block/dmg.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 50e91aef6d..0e05702f5d 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -572,14 +572,14 @@ static inline uint32_t search_chunk(BDRVDMGState *s,
uint64_t sector_num)
{
/* binary search */
uint32_t chunk1 = 0, chunk2 = s->n_chunks, chunk3;
-while (chunk1 != chunk2) {
+while (chunk1 <= chunk2) {
chunk3 = (chunk1 + chunk2) / 2;
if (s->sectors[chunk3] > sector_num) {
-chunk2 = chunk3;
+chunk2 = chunk3 - 1;
} else if (s->sectors[chunk3] + s->sectorcounts[chunk3] > sector_num) {
return chunk3;
} else {
-chunk1 = chunk3;
+chunk1 = chunk3 + 1;
}
}
return s->n_chunks; /* error */
--
2.17.1
[Qemu-devel] [PATCH v2 2/3] dmg: use enumeration type instead of hard coding number
Signed-off-by: yuchenlin
---
block/dmg.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 0e05702f5d..6b0a057bf8 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -267,7 +267,7 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
/* 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) {
+if (s->types[i] != UDIG && 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);
@@ -706,7 +706,7 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset,
uint64_t bytes,
/* 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 */
+if (s->types[s->current_chunk] == UDIG) { /* all zeroes block entry */
qemu_iovec_memset(qiov, i * 512, 0, 512);
continue;
}
--
2.17.1
[Qemu-devel] [PATCH v2 3/3] dmg: don't skip zero chunk
The dmg file has many tables which describe: "start from sector XXX to
sector XXX, the compression method is XXX and where the compressed data
resides on".
Each sector in the expanded file should be covered by a table. The table
will describe the offset of compressed data (or raw depends on the type)
in the dmg.
For example:
[---The expanded file]
[---bzip table ---]/* zeros */[---zlib---]
^
| if we want to read this sector.
we will find bzip table which contains this sector, and get the
compressed data offset, read it from dmg, uncompress it, finally write to
expanded file.
If we skip zero chunk (table), some sector cannot find the table which
will cause search_chunk() return s->n_chunks, dmg_read_chunk() return -1
and finally causing dmg_co_preadv() return EIO.
See:
[---The expanded file]
[---bzip table ---]/* zeros */[---zlib---]
^
| if we want to read this sector.
Oops, we cannot find the table contains it...
In the original implementation, we don't have zero table. When we try to
read sector inside the zero chunk. We will get EIO, and skip reading.
After this patch, we treat zero chunk the same as ignore chunk, it will
directly write zero and avoid some sector may not find the table.
After this patch:
[---The expanded file]
[---bzip table ---][--zeros--][---zlib---]
Signed-off-by: yuchenlin
---
block/dmg.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 6b0a057bf8..137fe9c1ff 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -130,7 +130,8 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t
chunk,
case UDRW: /* copy */
uncompressed_sectors = DIV_ROUND_UP(s->lengths[chunk], 512);
break;
-case UDIG: /* zero */
+case UDZE: /* zero */
+case UDIG: /* ignore */
/* 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. */
@@ -199,8 +200,9 @@ typedef struct DmgHeaderState {
static bool dmg_is_known_block_type(uint32_t entry_type)
{
switch (entry_type) {
+case UDZE:/* zeros */
case UDRW:/* uncompressed */
-case UDIG:/* zeroes */
+case UDIG:/* ignore */
case UDZO:/* zlib */
return true;
case UDBZ:/* bzip2 */
@@ -265,9 +267,10 @@ static int dmg_read_mish_block(BDRVDMGState *s,
DmgHeaderState *ds,
/* sector count */
s->sectorcounts[i] = buff_read_uint64(buffer, offset + 0x10);
-/* all-zeroes sector (type 2) does not need to be "uncompressed" and
can
- * therefore be unbounded. */
-if (s->types[i] != UDIG && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
+/* all-zeroes sector (type UDZE and UDIG) does not need to be
+ * "uncompressed" and can therefore be unbounded. */
+if (s->types[i] != UDZE && s->types[i] != UDIG
+&& 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);
@@ -671,7 +674,8 @@ static inline int dmg_read_chunk(BlockDriverState *bs,
uint64_t sector_num)
return -1;
}
break;
-case UDIG: /* zero */
+case UDZE: /* zeros */
+case UDIG: /* ignore */
/* see dmg_read, it is treated specially. No buffer needs to be
* pre-filled, the zeroes can be set directly. */
break;
@@ -706,7 +710,8 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset,
uint64_t bytes,
/* 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] == UDIG) { /* all zeroes block entry */
+if (s->types[s->current_chunk] == UDZE
+|| s->types[s->current_chunk] == UDIG) { /* all zeroes block entry
*/
qemu_iovec_memset(qiov, i * 512, 0, 512);
continue;
}
--
2.17.1
[Qemu-devel] [PATCH v2 0/3] dmg: fixing reading in dmg
There are two bugs in dmg reading. First, it may hang in binary search. this problem is solved by patch 1. Second, because of lacking zero chunk table, reading zero sector will return EIO. thie problem is solved by patch 2 and 3. Thanks v1 - >v2: * fix typos in patch 1 * add patch 2 and patch 3 yuchenlin (3): dmg: fix binary search dmg: use enumeration type instead of hard coding number dmg: don't skip zero chunk block/dmg.c | 25 +++-- 1 file changed, 15 insertions(+), 10 deletions(-) -- 2.17.1
[Qemu-devel] [Bug 1809304] Re: qemu-img convert is freezing for some DMG files.
I have submitted a patch to prevent hanging in binary search. See: http://lists.nongnu.org/archive/html/qemu-devel/2018-12/msg05453.html?fbclid=IwAR0ObsaZ4kVMVv6MWIdq0ZCAN5tGhDsd9GmFv8_v7HBTl94Cu5EkRZ3z4n4 Thanks, Yu-Chen Lin -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1809304 Title: qemu-img convert is freezing for some DMG files. Status in QEMU: New Bug description: Recently, I created a file using hdiutil from MacOS (using Zlib compression): $ hdiutil create -volname MyVolName -srcfolder /path/to/my/vol/ -ov -format UDZO myvolname.dmg But, when I try to convert this volume using qemu-img convert, this command is freezing. I'm using the upstream version to test it. It is freezing inside the binary search method to retrieve the chunk. But, I still don't know why. I'm attaching the file as an example. It can be mounted using MacOS or other Linux apps like hfsleuth and darling-dmg. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1809304/+subscriptions
[Qemu-devel] [PATCH] dmg: fix binary search
There is a possible hang in original binary searsh implemtation. That is
if chunk1 = 4, chunk2 = 5, chunk3 = 4, and we go else case.
The chunk1 will be still 4, and so on.
Signed-off-by: yuchenlin
---
block/dmg.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/dmg.c b/block/dmg.c
index 50e91aef6d..0e05702f5d 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -572,14 +572,14 @@ static inline uint32_t search_chunk(BDRVDMGState *s,
uint64_t sector_num)
{
/* binary search */
uint32_t chunk1 = 0, chunk2 = s->n_chunks, chunk3;
-while (chunk1 != chunk2) {
+while (chunk1 <= chunk2) {
chunk3 = (chunk1 + chunk2) / 2;
if (s->sectors[chunk3] > sector_num) {
-chunk2 = chunk3;
+chunk2 = chunk3 - 1;
} else if (s->sectors[chunk3] + s->sectorcounts[chunk3] > sector_num) {
return chunk3;
} else {
-chunk1 = chunk3;
+chunk1 = chunk3 + 1;
}
}
return s->n_chunks; /* error */
--
2.17.1
Re: [Qemu-devel] [PATCH] vga_int: remove unused function protype
On 2018-10-29 17:44, Gerd Hoffmann wrote: On Mon, Oct 22, 2018 at 04:00:53PM +0800, yuchen...@synology.com wrote: From: yuchenlin Signed-off-by: yuchenlin --- hw/display/vga_int.h | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h index 6e4fa48a79..55c418eab5 100644 --- a/hw/display/vga_int.h +++ b/hw/display/vga_int.h @@ -166,7 +166,6 @@ MemoryRegion *vga_init_io(VGACommonState *s, Object *obj, const MemoryRegionPortio **vbe_ports); void vga_common_reset(VGACommonState *s); -void vga_sync_dirty_bitmap(VGACommonState *s); void vga_dirty_log_start(VGACommonState *s); void vga_dirty_log_stop(VGACommonState *s); Added to vga queue. thanks, Gerd Hi, Gerd Laurent has sent a pull request for this trivial commit. See: http://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05896.html Thanks, yuchenlin
[Qemu-devel] [PATCH] vga_int: remove unused function protype
From: yuchenlin Signed-off-by: yuchenlin --- hw/display/vga_int.h | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h index 6e4fa48a79..55c418eab5 100644 --- a/hw/display/vga_int.h +++ b/hw/display/vga_int.h @@ -166,7 +166,6 @@ MemoryRegion *vga_init_io(VGACommonState *s, Object *obj, const MemoryRegionPortio **vbe_ports); void vga_common_reset(VGACommonState *s); -void vga_sync_dirty_bitmap(VGACommonState *s); void vga_dirty_log_start(VGACommonState *s); void vga_dirty_log_stop(VGACommonState *s); -- 2.18.0
Re: [Qemu-devel] [PATCH] vhost-scsi: prevent using uninitialized vqs
Ping? On 2018-10-12 17:07, yuchen...@synology.com wrote: From: yuchenlin There are 3 virtqueues (ctrl, event and cmd) for virtio scsi device, but seabios will only set the physical address for the 3rd one (cmd). Then in vhost_virtqueue_start(), virtio_queue_get_desc_addr() will be 0 for ctrl and event vq. In this case, ctrl and event vq are not initialized. vhost_verify_ring_mappings may use uninitialized vhost_virtqueue such that vhost_verify_ring_part_mapping returns ENOMEM. When encountered this problem, we got the following logs: qemu-system-x86_64: Unable to map available ring for ring 0 qemu-system-x86_64: Verify ring failure on region 0 Signed-off-by: Forrest Liu Signed-off-by: yuchenlin --- hw/scsi/vhost-scsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c index becf550085..7f21b4f9d6 100644 --- a/hw/scsi/vhost-scsi.c +++ b/hw/scsi/vhost-scsi.c @@ -183,7 +183,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) } vsc->dev.nvqs = VHOST_SCSI_VQ_NUM_FIXED + vs->conf.num_queues; -vsc->dev.vqs = g_new(struct vhost_virtqueue, vsc->dev.nvqs); +vsc->dev.vqs = g_new0(struct vhost_virtqueue, vsc->dev.nvqs); vsc->dev.vq_index = 0; vsc->dev.backend_features = 0;
[Qemu-devel] [PATCH] vhost-scsi: prevent using uninitialized vqs
From: yuchenlin There are 3 virtqueues (ctrl, event and cmd) for virtio scsi device, but seabios will only set the physical address for the 3rd one (cmd). Then in vhost_virtqueue_start(), virtio_queue_get_desc_addr() will be 0 for ctrl and event vq. In this case, ctrl and event vq are not initialized. vhost_verify_ring_mappings may use uninitialized vhost_virtqueue such that vhost_verify_ring_part_mapping returns ENOMEM. When encountered this problem, we got the following logs: qemu-system-x86_64: Unable to map available ring for ring 0 qemu-system-x86_64: Verify ring failure on region 0 Signed-off-by: Forrest Liu Signed-off-by: yuchenlin --- hw/scsi/vhost-scsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c index becf550085..7f21b4f9d6 100644 --- a/hw/scsi/vhost-scsi.c +++ b/hw/scsi/vhost-scsi.c @@ -183,7 +183,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) } vsc->dev.nvqs = VHOST_SCSI_VQ_NUM_FIXED + vs->conf.num_queues; -vsc->dev.vqs = g_new(struct vhost_virtqueue, vsc->dev.nvqs); +vsc->dev.vqs = g_new0(struct vhost_virtqueue, vsc->dev.nvqs); vsc->dev.vq_index = 0; vsc->dev.backend_features = 0; -- 2.18.0
Re: [Qemu-devel] [PATCH v3] vmdk: align end of file to a sector boundary
On 2018-10-08 10:38, Fam Zheng wrote: On Fri, 10/05 10:00, yuchenlin wrote: Ping? Hi, This was merged as 51b3c6b73acae1e3fd3c7d441fc86dd17356695f. Fam Hi, Thank you for your information. yuchenlin On 2018-09-13 16:34, Fam Zheng wrote: > On Thu, 09/13 16:29, yuchen...@synology.com wrote: > > From: yuchenlin > > > > There is a rare case which the size of last compressed cluster > > is larger than the cluster size, which will cause the file is > > not aligned at the sector boundary. > > > > There are three reasons to do it. First, if vmdk doesn't align at > > the sector boundary, there may be many undefined behaviors, > > such as, in vbox it will show VMDK: Compressed image is corrupted > > 'syno-vm-disk1.vmdk' (VERR_ZIP_CORRUPTED) when we try to import an > > ova with unaligned vmdk. Second, all the cluster_sector is aligned > > to sector, the last one should be like this, too. Third, it ease > > reading with sector based I/Os. > > > > Signed-off-by: yuchenlin > > Reviewed-by: Fam Zheng
Re: [Qemu-devel] [PATCH v3] vmdk: align end of file to a sector boundary
Ping? On 2018-09-13 16:34, Fam Zheng wrote: On Thu, 09/13 16:29, yuchen...@synology.com wrote: From: yuchenlin There is a rare case which the size of last compressed cluster is larger than the cluster size, which will cause the file is not aligned at the sector boundary. There are three reasons to do it. First, if vmdk doesn't align at the sector boundary, there may be many undefined behaviors, such as, in vbox it will show VMDK: Compressed image is corrupted 'syno-vm-disk1.vmdk' (VERR_ZIP_CORRUPTED) when we try to import an ova with unaligned vmdk. Second, all the cluster_sector is aligned to sector, the last one should be like this, too. Third, it ease reading with sector based I/Os. Signed-off-by: yuchenlin Reviewed-by: Fam Zheng
[Qemu-devel] [PATCH v3] vmdk: align end of file to a sector boundary
From: yuchenlin
There is a rare case which the size of last compressed cluster
is larger than the cluster size, which will cause the file is
not aligned at the sector boundary.
There are three reasons to do it. First, if vmdk doesn't align at
the sector boundary, there may be many undefined behaviors,
such as, in vbox it will show VMDK: Compressed image is corrupted
'syno-vm-disk1.vmdk' (VERR_ZIP_CORRUPTED) when we try to import an
ova with unaligned vmdk. Second, all the cluster_sector is aligned
to sector, the last one should be like this, too. Third, it ease
reading with sector based I/Os.
Signed-off-by: yuchenlin
---
v2 -> v3:
* Update commit message
thanks
block/vmdk.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/block/vmdk.c b/block/vmdk.c
index a9d0084e36..2c9e86d98f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1698,6 +1698,27 @@ static int coroutine_fn
vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov)
{
+if (bytes == 0) {
+/* The caller will write bytes 0 to signal EOF.
+ * When receive it, we align EOF to a sector boundary. */
+BDRVVmdkState *s = bs->opaque;
+int i, ret;
+int64_t length;
+
+for (i = 0; i < s->num_extents; i++) {
+length = bdrv_getlength(s->extents[i].file->bs);
+if (length < 0) {
+return length;
+}
+length = QEMU_ALIGN_UP(length, BDRV_SECTOR_SIZE);
+ret = bdrv_truncate(s->extents[i].file, length,
+PREALLOC_MODE_OFF, NULL);
+if (ret < 0) {
+return ret;
+}
+}
+return 0;
+}
return vmdk_co_pwritev(bs, offset, bytes, qiov, 0);
}
--
2.18.0
Re: [Qemu-devel] [PATCH v2] vmdk: align end of file to a sector boundary
On 2018-09-13 10:54, Fam Zheng wrote:
On Thu, 09/13 10:31, yuchen...@synology.com wrote:
From: yuchenlin
There is a rare case which the size of last compressed cluster
is larger than the cluster size, which will cause the file is
not aligned at the sector boundary.
The code looks good to me. Can you also explain why it is important to
align
file size to sector boundary in the comment?
Fam
In my opinion, there are three reasons to do it. First, if vmdk doesn't
align at the sector boundary, there may be many undefined behaviors,
such as in vbox it will show VMDK: Compressed image is corrupted
'88-disk1.vmdk' (VERR_ZIP_CORRUPTED) when we try to import an ova with
unaligned vmdk. Second, all the cluster_sector is aligned
to sector, the last one should be like this, too. Third, it ease reading
with sector based I/Os.
What do you think?
yuchenlin
Signed-off-by: yuchenlin
---
v1 -> v2:
* Add more detail comment.
* Add QEMU_ALIGN_UP to show the intention more clearly.
* Add newline in the end of bdrv_truncate.
thanks
block/vmdk.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/block/vmdk.c b/block/vmdk.c
index a9d0084e36..2c9e86d98f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1698,6 +1698,27 @@ static int coroutine_fn
vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov)
{
+if (bytes == 0) {
+/* The caller will write bytes 0 to signal EOF.
+ * When receive it, we align EOF to a sector boundary. */
+BDRVVmdkState *s = bs->opaque;
+int i, ret;
+int64_t length;
+
+for (i = 0; i < s->num_extents; i++) {
+length = bdrv_getlength(s->extents[i].file->bs);
+if (length < 0) {
+return length;
+}
+length = QEMU_ALIGN_UP(length, BDRV_SECTOR_SIZE);
+ret = bdrv_truncate(s->extents[i].file, length,
+PREALLOC_MODE_OFF, NULL);
+if (ret < 0) {
+return ret;
+}
+}
+return 0;
+}
return vmdk_co_pwritev(bs, offset, bytes, qiov, 0);
}
--
2.18.0
[Qemu-devel] [PATCH v2] vmdk: align end of file to a sector boundary
From: yuchenlin
There is a rare case which the size of last compressed cluster
is larger than the cluster size, which will cause the file is
not aligned at the sector boundary.
Signed-off-by: yuchenlin
---
v1 -> v2:
* Add more detail comment.
* Add QEMU_ALIGN_UP to show the intention more clearly.
* Add newline in the end of bdrv_truncate.
thanks
block/vmdk.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/block/vmdk.c b/block/vmdk.c
index a9d0084e36..2c9e86d98f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1698,6 +1698,27 @@ static int coroutine_fn
vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov)
{
+if (bytes == 0) {
+/* The caller will write bytes 0 to signal EOF.
+ * When receive it, we align EOF to a sector boundary. */
+BDRVVmdkState *s = bs->opaque;
+int i, ret;
+int64_t length;
+
+for (i = 0; i < s->num_extents; i++) {
+length = bdrv_getlength(s->extents[i].file->bs);
+if (length < 0) {
+return length;
+}
+length = QEMU_ALIGN_UP(length, BDRV_SECTOR_SIZE);
+ret = bdrv_truncate(s->extents[i].file, length,
+PREALLOC_MODE_OFF, NULL);
+if (ret < 0) {
+return ret;
+}
+}
+return 0;
+}
return vmdk_co_pwritev(bs, offset, bytes, qiov, 0);
}
--
2.18.0
Re: [Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary
On 2018-09-12 19:54, Fam Zheng wrote:
On Tue, 08/28 11:17, yuchen...@synology.com wrote:
From: yuchenlin
There is a rare case which the size of last compressed cluster
is larger than the cluster size, which will cause the file is
not aligned at the sector boundary.
Signed-off-by: yuchenlin
---
block/vmdk.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/block/vmdk.c b/block/vmdk.c
index a9d0084e36..a8ae7c65d2 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1698,6 +1698,24 @@ static int coroutine_fn
vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov)
{
+if (bytes == 0) {
+/* align end of file to a sector boundary. */
+BDRVVmdkState *s = bs->opaque;
+int i, ret;
+int64_t length;
+
+for (i = 0; i < s->num_extents; i++) {
+length = bdrv_getlength(s->extents[i].file->bs);
+if (length < 0) {
+return length;
+}
Could you add "length = QEMU_ALIGN_UP(length, BDRV_SECTOR_SIZE);" to
show the
intention more clearly?
Fam
Thank you for your effort, I will do it.
yuchenlin
+ret = bdrv_truncate(s->extents[i].file, length,
PREALLOC_MODE_OFF, NULL);
+if (ret < 0) {
+return ret;
+}
+}
+return 0;
+}
return vmdk_co_pwritev(bs, offset, bytes, qiov, 0);
}
--
2.17.0
Re: [Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary
Fam Zheng 於 2018-09-12 17:34 寫道：
> On Tue, 08/28 11:17, yuchen...@synology.com wrote: > From: yuchenlin
> > > There is a rare case which the size of last
> compressed cluster > is larger than the cluster size, which will cause the
> file is > not aligned at the sector boundary. I don't understand. Doesn't it
> mean that if you force the alignment by truncating out the extra bytes, some
> data is lost?
You can take qcow2_co_pwritev_compressed in block/qcow2.c as an example.
The bdrv_getlength will return the length in bytes which is always a multiple
of BDRV_SECTOR_SIZE.
After truncates this size, the vmdk is extended to align sector size.
> > > Signed-off-by: yuchenlin > --- > block/vmdk.c |
> > > 18 ++ > 1 file changed, 18 insertions(+) > > diff --git
> > > a/block/vmdk.c b/block/vmdk.c > index a9d0084e36..a8ae7c65d2 100644 > ---
> > > a/block/vmdk.c > +++ b/block/vmdk.c > @@ -1698,6 +1698,24 @@ static int
> > > coroutine_fn > vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t
> > > offset, > uint64_t bytes, QEMUIOVector *qiov) > { > + if (bytes == 0) {
> > > Where is this bytes == 0 condition from?
From the end of convert_do_copy in qemu-img.c.
if (s->compressed && !s->ret) {
/* signal EOF to align */
ret = blk_pwrite_compressed(s->target, 0, NULL, 0);
if (ret < 0) {
return ret;
}
}
It signals the EOF to the block driver.
> > + /* align end of file to a sector boundary. */ > + BDRVVmdkState *s =
> > bs->opaque; > + int i, ret; > + int64_t length; > + > + for (i = 0; i <
> > s->num_extents; i++) { > + length = bdrv_getlength(s->extents[i].file->bs);
> > > + if (length < 0) { > + return length; > + } > + ret =
> > bdrv_truncate(s->extents[i].file, length, PREALLOC_MODE_OFF, NULL); > + if
> > (ret < 0) { > + return ret; > + } > + } > + return 0; > + } > return
> > vmdk_co_pwritev(bs, offset, bytes, qiov, 0); > } > > -- > 2.17.0 > Fam
yuchenlin
Re: [Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary
Ping!
yuchen...@synology.com 於 2018-08-28 11:18 寫道：
> From: yuchenlin There is a rare case which the size
> of last compressed cluster is larger than the cluster size, which will cause
> the file is not aligned at the sector boundary. Signed-off-by: yuchenlin
> --- block/vmdk.c | 18 ++ 1 file
> changed, 18 insertions(+) diff --git a/block/vmdk.c b/block/vmdk.c index
> a9d0084e36..a8ae7c65d2 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@
> -1698,6 +1698,24 @@ static int coroutine_fn
> vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, uint64_t
> bytes, QEMUIOVector *qiov) { + if (bytes == 0) { + /* align end of file to a
> sector boundary. */ + BDRVVmdkState *s = bs->opaque; + int i, ret; + int64_t
> length; + + for (i = 0; i < s->num_extents; i++) { + length =
> bdrv_getlength(s->extents[i].file->bs); + if (length < 0) { + return length;
> + } + ret = bdrv_truncate(s->extents[i].file, length, PREALLOC_MODE_OFF,
> NULL); + if (ret < 0) { + return ret; + } + } + return 0; + } return
> vmdk_co_pwritev(bs, offset, bytes, qiov, 0); } -- 2.17.0
[Qemu-devel] [PATCH] vmdk: align end of file to a sector boundary
From: yuchenlin
There is a rare case which the size of last compressed cluster
is larger than the cluster size, which will cause the file is
not aligned at the sector boundary.
Signed-off-by: yuchenlin
---
block/vmdk.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/block/vmdk.c b/block/vmdk.c
index a9d0084e36..a8ae7c65d2 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1698,6 +1698,24 @@ static int coroutine_fn
vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov)
{
+if (bytes == 0) {
+/* align end of file to a sector boundary. */
+BDRVVmdkState *s = bs->opaque;
+int i, ret;
+int64_t length;
+
+for (i = 0; i < s->num_extents; i++) {
+length = bdrv_getlength(s->extents[i].file->bs);
+if (length < 0) {
+return length;
+}
+ret = bdrv_truncate(s->extents[i].file, length, PREALLOC_MODE_OFF,
NULL);
+if (ret < 0) {
+return ret;
+}
+}
+return 0;
+}
return vmdk_co_pwritev(bs, offset, bytes, qiov, 0);
}
--
2.17.0
Re: [Qemu-devel] [PATCH 0/2] Refine some vdi code
Hi, Stefan I agree that redundancy of If else may helps people to understand the code. However, CONFIG_VDI_WRITE only contributes: #if defined(CONFIG_VDI_WRITE) .bdrv_co_pwritev = vdi_co_pwritev, #endif I think we don't need CONFIG_VDI_WRITE to document the code. As its name implies, vdi_co_pwritev shows the code parts for vdi write support. I appreciated your time and effort for reviews. Regards, yuchenlin Stefan Weil 於 2018-07-30 15:13 寫道： > Am 30.07.2018 um 04:46 schrieb yuchen...@synology.com: > From: yuchenlin > > > This series refine some code in vdi.c, includes: > > > * Remvoe CONFIG_VDI_WRITE because there is no reason to leave an always > on > and cannot configure option in the code-side. > * decouple if else if > chain to get more readability. > > Thanks, > yuchenlin > > yuchenlin (2): > > vdi: remove CONFIG_VDI_WRITE > vdi: refine code for vdi_open > > block/vdi.c > | 32 ++-- > 1 file changed, 18 insertions(+), 14 > deletions(-) Technically these changes are fine, but personally I prefer my > old code. If else is rendundant here, but redundancy helps humans to > understand the code. CONFIG_VDI_WRITE still has a similar function as it > documents which code parts are relevant for write support. Stefan
[Qemu-devel] [PATCH 1/2] vdi: remove CONFIG_VDI_WRITE
From: yuchenlin
The CONFIG_VDI_WRITE is here when the first time vdi is added.
But there is no reason to leave an always on and cannot configure option
in the code-side.
Signed-off-by: yuchenlin
---
block/vdi.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index 6555cffb88..12f92e7891 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -70,9 +70,6 @@
/* Enable debug messages. */
//~ #define CONFIG_VDI_DEBUG
-/* Support write operations on VDI images. */
-#define CONFIG_VDI_WRITE
-
/* Support non-standard block (cluster) size. This is untested.
* Maybe it will be needed for very large images.
*/
@@ -1016,9 +1013,7 @@ static BlockDriver bdrv_vdi = {
.bdrv_make_empty = vdi_make_empty,
.bdrv_co_preadv = vdi_co_preadv,
-#if defined(CONFIG_VDI_WRITE)
.bdrv_co_pwritev= vdi_co_pwritev,
-#endif
.bdrv_get_info = vdi_get_info,
--
2.17.0
[Qemu-devel] [PATCH 0/2] Refine some vdi code
From: yuchenlin This series refine some code in vdi.c, includes: * Remvoe CONFIG_VDI_WRITE because there is no reason to leave an always on and cannot configure option in the code-side. * decouple if else if chain to get more readability. Thanks, yuchenlin yuchenlin (2): vdi: remove CONFIG_VDI_WRITE vdi: refine code for vdi_open block/vdi.c | 32 ++-- 1 file changed, 18 insertions(+), 14 deletions(-) -- 2.17.0
[Qemu-devel] [PATCH 2/2] vdi: refine code for vdi_open
From: yuchenlin
When the condition of each if or else if is true,
the code flow will goto fail. Which means we can decouple
if else if chain to get some readability.
Signed-off-by: yuchenlin
---
block/vdi.c | 27 ++-
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index 12f92e7891..28fc6210a7 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -405,35 +405,41 @@ static int vdi_open(BlockDriverState *bs, QDict *options,
int flags,
")", header.signature);
ret = -EINVAL;
goto fail;
-} else if (header.version != VDI_VERSION_1_1) {
+}
+if (header.version != VDI_VERSION_1_1) {
error_setg(errp, "unsupported VDI image (version %" PRIu32 ".%" PRIu32
")", header.version >> 16, header.version & 0x);
ret = -ENOTSUP;
goto fail;
-} else if (header.offset_bmap % SECTOR_SIZE != 0) {
+}
+if (header.offset_bmap % SECTOR_SIZE != 0) {
/* We only support block maps which start on a sector boundary. */
error_setg(errp, "unsupported VDI image (unaligned block map offset "
"0x%" PRIx32 ")", header.offset_bmap);
ret = -ENOTSUP;
goto fail;
-} else if (header.offset_data % SECTOR_SIZE != 0) {
+}
+if (header.offset_data % SECTOR_SIZE != 0) {
/* We only support data blocks which start on a sector boundary. */
error_setg(errp, "unsupported VDI image (unaligned data offset 0x%"
PRIx32 ")", header.offset_data);
ret = -ENOTSUP;
goto fail;
-} else if (header.sector_size != SECTOR_SIZE) {
+}
+if (header.sector_size != SECTOR_SIZE) {
error_setg(errp, "unsupported VDI image (sector size %" PRIu32
" is not %u)", header.sector_size, SECTOR_SIZE);
ret = -ENOTSUP;
goto fail;
-} else if (header.block_size != DEFAULT_CLUSTER_SIZE) {
+}
+if (header.block_size != DEFAULT_CLUSTER_SIZE) {
error_setg(errp, "unsupported VDI image (block size %" PRIu32
" is not %" PRIu64 ")",
header.block_size, DEFAULT_CLUSTER_SIZE);
ret = -ENOTSUP;
goto fail;
-} else if (header.disk_size >
+}
+if (header.disk_size >
(uint64_t)header.blocks_in_image * header.block_size) {
error_setg(errp, "unsupported VDI image (disk size %" PRIu64 ", "
"image bitmap has room for %" PRIu64 ")",
@@ -441,15 +447,18 @@ static int vdi_open(BlockDriverState *bs, QDict *options,
int flags,
(uint64_t)header.blocks_in_image * header.block_size);
ret = -ENOTSUP;
goto fail;
-} else if (!qemu_uuid_is_null(_link)) {
+}
+if (!qemu_uuid_is_null(_link)) {
error_setg(errp, "unsupported VDI image (non-NULL link UUID)");
ret = -ENOTSUP;
goto fail;
-} else if (!qemu_uuid_is_null(_parent)) {
+}
+if (!qemu_uuid_is_null(_parent)) {
error_setg(errp, "unsupported VDI image (non-NULL parent UUID)");
ret = -ENOTSUP;
goto fail;
-} else if (header.blocks_in_image > VDI_BLOCKS_IN_IMAGE_MAX) {
+}
+if (header.blocks_in_image > VDI_BLOCKS_IN_IMAGE_MAX) {
error_setg(errp, "unsupported VDI image "
"(too many blocks %u, max is %u)",
header.blocks_in_image, VDI_BLOCKS_IN_IMAGE_MAX);
--
2.17.0
[Qemu-devel] [PATCH v3] vmdk: return ERROR when cluster sector is larger than vmdk limitation
From: yuchenlin <yuchen...@synology.com>
VMDK has a hard limitation of extent size, which is due to the size of grain
table entry is 32 bits. It means it can only point to a grain located at
offset = 2^32. To avoid writing the user data beyond limitation and record a
useless offset
in grain table. We should return ERROR here.
Signed-off-by: yuchenlin <yuchen...@synology.com>
---
v2->v3:
- use (1ULL << 32) to clearly show the limitation of offset is 2^32.
thanks
block/vmdk.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/block/vmdk.c b/block/vmdk.c
index f94c49a9c0..84f8bbe480 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -47,6 +47,8 @@
#define VMDK4_FLAG_MARKER (1 << 17)
#define VMDK4_GD_AT_END 0xULL
+#define VMDK_EXTENT_MAX_SECTORS (1ULL << 32)
+
#define VMDK_GTE_ZEROED 0x1
/* VMDK internal error codes */
@@ -1250,6 +1252,10 @@ static int get_cluster_offset(BlockDriverState *bs,
return zeroed ? VMDK_ZEROED : VMDK_UNALLOC;
}
+if (extent->next_cluster_sector >= VMDK_EXTENT_MAX_SECTORS) {
+return VMDK_ERROR;
+}
+
cluster_sector = extent->next_cluster_sector;
extent->next_cluster_sector += extent->cluster_sectors;
--
2.16.2
[Qemu-devel] [PATCH v2] vmdk: return ERROR when cluster sector is larger than vmdk limitation
From: yuchenlin <yuchen...@synology.com>
VMDK has a hard limitation of extent size, which is due to the size of grain
table entry is 32 bits. It means it can only point to a grain located at
offset = 2^32. To avoid writing the user data beyond limitation and record a
useless offset
in grain table. We should return ERROR here.
Signed-off-by: yuchenlin <yuchen...@synology.com>
---
v1->v2:
- change commit message
- check before allocating
- should be >=
- the unit is sector now
thanks
block/vmdk.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/block/vmdk.c b/block/vmdk.c
index f94c49a9c0..a1c21dbbba 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -47,6 +47,8 @@
#define VMDK4_FLAG_MARKER (1 << 17)
#define VMDK4_GD_AT_END 0xULL
+#define VMDK_EXTENT_MAX_SECTORS (4294967296)
+
#define VMDK_GTE_ZEROED 0x1
/* VMDK internal error codes */
@@ -1250,6 +1252,10 @@ static int get_cluster_offset(BlockDriverState *bs,
return zeroed ? VMDK_ZEROED : VMDK_UNALLOC;
}
+if (extent->next_cluster_sector >= VMDK_EXTENT_MAX_SECTORS) {
+return VMDK_ERROR;
+}
+
cluster_sector = extent->next_cluster_sector;
extent->next_cluster_sector += extent->cluster_sectors;
--
2.16.2
[Qemu-devel] [PATCH] vmdk: return ENOTSUP before offset overflow
From: yuchenlin <yuchen...@synology.com>
VMDK has a hard limitation of extent size, which is due to the size of grain
table entry is 32 bits. It means it can only point to a grain located at
offset = 2^32. To prevent offset overflow and record a useless offset
in grain table. We should return un-support here.
Signed-off-by: yuchenlin <yuchen...@synology.com>
---
block/vmdk.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/block/vmdk.c b/block/vmdk.c
index f94c49a9c0..d8fc961940 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -47,6 +47,9 @@
#define VMDK4_FLAG_MARKER (1 << 17)
#define VMDK4_GD_AT_END 0xULL
+/* 2TB */
+#define VMDK_EXTENT_SIZE_LIMIT (219902322)
+
#define VMDK_GTE_ZEROED 0x1
/* VMDK internal error codes */
@@ -1645,6 +1648,9 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t
offset,
return ret;
}
if (m_data.valid) {
+if (cluster_offset > VMDK_EXTENT_SIZE_LIMIT) {
+return -ENOTSUP;
+}
/* update L2 tables */
if (vmdk_L2update(extent, _data,
cluster_offset >> BDRV_SECTOR_BITS)
--
2.16.2
