Re: [Qemu-block] [Qemu-devel] segfault in parallel blockjobs (iotest 30)
On Wed, 11/08 18:50, Anton Nefedov wrote:
>
>
> On 8/11/2017 5:45 PM, Alberto Garcia wrote:
> > On Tue 07 Nov 2017 05:19:41 PM CET, Anton Nefedov wrote:
> > > BlockBackend gets deleted by another job's stream_complete(), deferred
> > > to the main loop, so the fact that the job is put to sleep by
> > > bdrv_drain_all_begin() doesn't really stop it from execution.
> >
> > I was debugging this a bit, and the block_job_defer_to_main_loop() call
> > happens _after_ all jobs have been paused, so I think that when the BDS
> > is drained then stream_run() finishes the last iteration without
> > checking if it's paused.
> >
> > Without your patch (i.e. with a smaller STREAM_BUFFER_SIZE) then I
> > assume that the function would have to continue looping and
> > block_job_sleep_ns() would make the job coroutine yield, effectively
> > pausing the job and preventing the crash.
> >
> > I can fix the crash by adding block_job_pause_point(&s->common) at the
> > end of stream_run() (where the 'out' label is).
> >
> > I'm thinking that perhaps we should add the pause point directly to
> > block_job_defer_to_main_loop(), to prevent any block job from running
> > the exit function when it's paused.
> >
>
> Is it possible that the exit function is already deferred when the jobs
> are being paused? (even though it's at least less likely to happen)
>
> Then should we flush the bottom halves somehow in addition to putting
> the jobs to sleep? And also then it all probably has to happen before
> bdrv_reopen_queue()
Or we can stash away the BH temporarily during pause period:
---
diff --git a/blockjob.c b/blockjob.c
index 3a0c49137e..7058ff7ae1 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -127,6 +127,9 @@ void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job)
static void block_job_pause(BlockJob *job)
{
job->pause_count++;
+if (job->defer_to_main_loop_bh) {
+qemu_bh_cancel(job->defer_to_main_loop_bh);
+}
}
static void block_job_resume(BlockJob *job)
@@ -136,6 +139,9 @@ static void block_job_resume(BlockJob *job)
if (job->pause_count) {
return;
}
+if (job->defer_to_main_loop_bh) {
+qemu_bh_schedule(job->defer_to_main_loop_bh);
+}
block_job_enter(job);
}
@@ -900,6 +906,9 @@ static void block_job_defer_to_main_loop_bh(void *opaque)
/* Prevent race with block_job_defer_to_main_loop() */
aio_context_acquire(data->aio_context);
+qemu_bh_delete(data->job->defer_to_main_loop_bh);
+data->job->defer_to_main_loop_bh = NULL;
+
/* Fetch BDS AioContext again, in case it has changed */
aio_context = blk_get_aio_context(data->job->blk);
if (aio_context != data->aio_context) {
@@ -927,7 +936,9 @@ void block_job_defer_to_main_loop(BlockJob *job,
data->fn = fn;
data->opaque = opaque;
job->deferred_to_main_loop = true;
+assert(!job->defer_to_main_loop_bh);
+job->defer_to_main_loop_bh = aio_bh_new(qemu_get_aio_context(),
+ block_job_defer_to_main_loop_bh, data);
-aio_bh_schedule_oneshot(qemu_get_aio_context(),
-block_job_defer_to_main_loop_bh, data);
+qemu_bh_schedule(job->defer_to_main_loop_bh);
}
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 67c0968fa5..28d29b3be6 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -97,6 +97,11 @@ typedef struct BlockJob {
*/
bool deferred_to_main_loop;
+/**
+ * The main loop BH for "defer to main loop" operation.
+ */
+QEMUBH *defer_to_main_loop_bh;
+
/** Element of the list of block jobs */
QLIST_ENTRY(BlockJob) job_list;
Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/7] various NBD fixes for 2.11
On Wed, 11/08 17:05, Eric Blake wrote: > But I'm not sure if there is an easy way to tell patchew to include a > fixup patch and re-test the series, short of sending a v3. Yeah, I don't know a way either. Fam
Re: [Qemu-block] [Qemu-devel] Intermittent hang of iotest 194 (bdrv_drain_all after non-shared storage migration)
On Thu, 11/09 01:48, Max Reitz wrote: > Hi, > > More exciting news from the bdrv_drain() front! > > I've noticed in the past that iotest 194 sometimes hangs. I usually run > the tests on tmpfs, but I've just now verified that it happens on my SSD > just as well. > > So the reproducer is a plain: > > while ./check -raw 194; do; done I cannot produce it on my machine. > > (No difference between raw or qcow2, though.) > > And then, after a couple of runs (or a couple ten), it will just hang. > The reason is that the source VM lingers around and doesn't quit > voluntarily -- the test itself was successful, but it just can't exit. > > If you force it to exit by killing the VM (e.g. through pkill -11 qemu), > this is the backtrace: > > #0 0x7f7cfc297e06 in ppoll () at /lib64/libc.so.6 > #1 0x563b846bcac9 in ppoll (__ss=0x0, __timeout=0x0, > __nfds=, __fds=) at > /usr/include/bits/poll2.h:77 Looking at the 0 timeout it seems we are in the aio_poll(ctx, blocking=false); branches of BDRV_POLL_WHILE? Is it a busy loop? If so I wonder what is making progress and causing the return value to be true in aio_poll(). > #2 0x563b846bcac9 in qemu_poll_ns (fds=, > nfds=, timeout=) at util/qemu-timer.c:322 > #3 0x563b846be711 in aio_poll (ctx=ctx@entry=0x563b856e3e80, > blocking=) at util/aio-posix.c:629 > #4 0x563b8463afa4 in bdrv_drain_recurse > (bs=bs@entry=0x563b865568a0, begin=begin@entry=true) at block/io.c:201 > #5 0x563b8463baff in bdrv_drain_all_begin () at block/io.c:381 > #6 0x563b8463bc99 in bdrv_drain_all () at block/io.c:411 > #7 0x563b8459888b in block_migration_cleanup (opaque= out>) at migration/block.c:714 > #8 0x563b845883be in qemu_savevm_state_cleanup () at > migration/savevm.c:1251 > #9 0x563b845811fd in migration_thread (opaque=0x563b856f1da0) at > migration/migration.c:2298 > #10 0x7f7cfc56f36d in start_thread () at /lib64/libpthread.so.0 > #11 0x7f7cfc2a3e1f in clone () at /lib64/libc.so.6 > > > And when you make bdrv_drain_all_begin() print what we are trying to > drain, you can see that it's the format node (managed by the "raw" > driver in this case). So what is the value of bs->in_flight? > > So I thought, before I put more time into this, let's ask whether the > test author has any ideas. :-) Fam
Re: [Qemu-block] [Qemu-devel] [PATCH v4 06/45] hw/block: Replace fprintf(stderr, "*\n" with error_report()
On 11/08/2017 07:56 PM, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
>
[...]
> diff --git a/hw/block/tc58128.c b/hw/block/tc58128.c
> index 1d9f7ee000..d274c9aafe 100644
> --- a/hw/block/tc58128.c
> +++ b/hw/block/tc58128.c
OMG you found a rare pearl, I enjoyed reading this file git history :)
Not a single logical change since his origin...
http://web.archive.org/web/20070917001736/perso.enst.fr/~polti/realisations/shix20
<3 <3
> @@ -50,8 +50,8 @@ static void init_dev(tc58128_dev * dev, const char
> *filename)
> dev->flash_contents[1] = (blocks >> 8) & 0xff;
> dev->flash_contents[2] = (blocks >> 16) & 0xff;
> dev->flash_contents[3] = (blocks >> 24) & 0xff;
> - fprintf(stderr, "loaded %d bytes for %s into flash\n", ret,
> - filename);
> +error_report("loaded %d bytes for %s into flash", ret,
> + filename);
This one should be info_report().
> }
> }
> }
> @@ -60,26 +60,26 @@ static void handle_command(tc58128_dev * dev, uint8_t
> command)
> {
> switch (command) {
> case 0xff:
> - fprintf(stderr, "reset flash device\n");
> - dev->state = WAIT;
> - break;
> +error_report("reset flash device");
info_report() or even cleaner trace_tc58128_command()?
> +dev->state = WAIT;
> +break;
> case 0x00:
> - fprintf(stderr, "read mode 1\n");
> - dev->state = READ1;
> - dev->address_cycle = 0;
> - break;
> +error_report("read mode 1");
Ditto.
> +dev->state = READ1;
> +dev->address_cycle = 0;
> +break;
> case 0x01:
> - fprintf(stderr, "read mode 2\n");
> - dev->state = READ2;
> - dev->address_cycle = 0;
> - break;
> +error_report("read mode 2");
Ditto.
> +dev->state = READ2;
> +dev->address_cycle = 0;
> +break;
> case 0x50:
> - fprintf(stderr, "read mode 3\n");
> - dev->state = READ3;
> - dev->address_cycle = 0;
> - break;
> +error_report("read mode 3");
Ditto.
> +dev->state = READ3;
> +dev->address_cycle = 0;
> +break;
> default:
> - fprintf(stderr, "unknown flash command 0x%02x\n", command);
> +error_report("unknown flash command 0x%02x", command);
> abort();
> }
> }
> @@ -103,8 +103,8 @@ static void handle_address(tc58128_dev * dev, uint8_t
> data)
> break;
> case 2:
> dev->address += data * 528;
> - fprintf(stderr, "address pointer in flash: 0x%08x\n",
> - dev->address);
> +error_report("address pointer in flash: 0x%08x",
> + dev->address);
warn_report()?
> break;
> default:
> /* Invalid data */
> @@ -119,10 +119,6 @@ static void handle_address(tc58128_dev * dev, uint8_t
> data)
>
> static uint8_t handle_read(tc58128_dev * dev)
> {
> -#if 0
> -if (dev->address % 0x10 == 0)
> - fprintf(stderr, "reading flash at address 0x%08x\n", dev->address);
> -#endif
> return dev->flash_contents[dev->address++];
> }
[Qemu-block] [PATCH 3/5] iotests: Make 055 less flaky
First of all, test 055 does a valiant job of invoking pause_drive()
sometimes, but that is worth nothing without blkdebug. So the first
thing to do is to sprinkle a couple of "blkdebug::" in there -- with the
exception of the transaction tests, because the blkdebug break points
make the transaction QMP command hang (which is bad). In that case, we
can get away with throttling the block job that it effectively is
paused.
Then, 055 usually does not pause the drive before starting a block job
that should be cancelled. This means that the backup job might be
completed already before block-job-cancel is invoked; thus making the
test either fail (currently) or moot if cancel_and_wait() ignored this
condition. Fix this by pausing the drive before starting the job.
Signed-off-by: Max Reitz
---
tests/qemu-iotests/055 | 25 -
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index e1206caf9b..8a5d9fd269 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -48,7 +48,7 @@ class TestSingleDrive(iotests.QMPTestCase):
def setUp(self):
qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img,
str(image_len))
-self.vm = iotests.VM().add_drive(test_img)
+self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
self.vm.add_drive(blockdev_target_img, interface="none")
if iotests.qemu_default_machine == 'pc':
self.vm.add_drive(None, 'media=cdrom', 'ide')
@@ -65,10 +65,11 @@ class TestSingleDrive(iotests.QMPTestCase):
def do_test_cancel(self, cmd, target):
self.assert_no_active_block_jobs()
+self.vm.pause_drive('drive0')
result = self.vm.qmp(cmd, device='drive0', target=target, sync='full')
self.assert_qmp(result, 'return', {})
-event = self.cancel_and_wait()
+event = self.cancel_and_wait(resume=True)
self.assert_qmp(event, 'data/type', 'backup')
def test_cancel_drive_backup(self):
@@ -166,7 +167,7 @@ class TestSetSpeed(iotests.QMPTestCase):
def setUp(self):
qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img,
str(image_len))
-self.vm = iotests.VM().add_drive(test_img)
+self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
self.vm.add_drive(blockdev_target_img, interface="none")
self.vm.launch()
@@ -246,6 +247,8 @@ class TestSetSpeed(iotests.QMPTestCase):
def test_set_speed_invalid_blockdev_backup(self):
self.do_test_set_speed_invalid('blockdev-backup', 'drive1')
+# Note: We cannot use pause_drive() here, or the transaction command
+# would stall. Instead, we limit the block job speed here.
class TestSingleTransaction(iotests.QMPTestCase):
def setUp(self):
qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img,
str(image_len))
@@ -271,7 +274,8 @@ class TestSingleTransaction(iotests.QMPTestCase):
'type': cmd,
'data': { 'device': 'drive0',
'target': target,
- 'sync': 'full' },
+ 'sync': 'full',
+ 'speed': 64 * 1024 },
}
])
@@ -289,12 +293,12 @@ class TestSingleTransaction(iotests.QMPTestCase):
def do_test_pause(self, cmd, target, image):
self.assert_no_active_block_jobs()
-self.vm.pause_drive('drive0')
result = self.vm.qmp('transaction', actions=[{
'type': cmd,
'data': { 'device': 'drive0',
'target': target,
- 'sync': 'full' },
+ 'sync': 'full',
+ 'speed': 64 * 1024 },
}
])
self.assert_qmp(result, 'return', {})
@@ -302,7 +306,9 @@ class TestSingleTransaction(iotests.QMPTestCase):
result = self.vm.qmp('block-job-pause', device='drive0')
self.assert_qmp(result, 'return', {})
-self.vm.resume_drive('drive0')
+result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
+self.assert_qmp(result, 'return', {})
+
self.pause_job('drive0')
result = self.vm.qmp('query-block-jobs')
@@ -461,7 +467,7 @@ class TestDriveCompression(iotests.QMPTestCase):
pass
def do_prepare_drives(self, fmt, args, attach_target):
-self.vm = iotests.VM().add_drive(test_img)
+self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
qemu_img('create', '-f', fmt, blockdev_target_img,
str(TestDriveCompression.image_len), *args)
@@ -500,10 +506,11 @@ class TestDriveCompression(iotests.QMPTestCase):
self.assert_no_active_block_jobs()
+self.vm.pause_drive('drive0')
result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True,
**args)
self.assert_qmp(result, 'return', {})
-
[Qemu-block] [PATCH 5/5] iotests: Make 136 less flaky
136 executes some AIO requests without a final aio_flush; then it
advances the virtual clock and thus expects the last access time of the
device to be less than the current time when queried (i.e. idle_time_ns
to be greater than 0). However, without the aio_flush, some requests
may be settled after the clock_step invocation. In that case,
idle_time_ns would be 0 and the test fails.
Fix this by adding an aio_flush if any AIO request but some other
aio_flush has been executed.
Signed-off-by: Max Reitz
---
tests/qemu-iotests/136 | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136
index 4b994897af..88b97ea7c6 100644
--- a/tests/qemu-iotests/136
+++ b/tests/qemu-iotests/136
@@ -238,6 +238,18 @@ sector = "%d"
for i in range(failed_wr_ops):
ops.append("aio_write %d 512" % bad_offset)
+# We need an extra aio_flush to settle all outstanding AIO
+# operations before we can advance the virtual clock, so that
+# the last access happens before clock_step and idle_time_ns
+# will be greater than 0
+extra_flush = 0
+if rd_ops + wr_ops + invalid_rd_ops + invalid_wr_ops + \
+failed_rd_ops + failed_wr_ops > 0:
+extra_flush = 1
+
+if extra_flush > 0:
+ops.append("aio_flush")
+
if failed_wr_ops > 0:
highest_offset = max(highest_offset, bad_offset + 512)
@@ -251,7 +263,7 @@ sector = "%d"
self.total_wr_bytes += wr_ops * wr_size
self.total_wr_ops += wr_ops
self.total_wr_merged += wr_merged
-self.total_flush_ops += flush_ops
+self.total_flush_ops += flush_ops + extra_flush
self.invalid_rd_ops += invalid_rd_ops
self.invalid_wr_ops += invalid_wr_ops
self.failed_rd_ops += failed_rd_ops
--
2.13.6
[Qemu-block] [PATCH 4/5] iotests: Make 083 less flaky
083 has (at least) two issues: 1. By launching the nbd-fault-injector in background, it may not be scheduled until the first grep on its output file is executed. However, until then, that file may not have been created yet -- so it either does not exist yet (thus making the grep emit an error), or it does exist but contains stale data (thus making the rest of the test case work connect to a wrong address). Fix this by explicitly overwriting the output file before executing nbd-fault-injector. 2. The nbd-fault-injector prints things other than "Listening on...". It also prints a "Closing connection" message from time to time. We currently invoke sed on the whole file in the hope of it only containing the "Listening on..." line yet. That hope is sometimes shattered by the brutal reality of race conditions, so invoke grep before sed. Signed-off-by: Max Reitz --- tests/qemu-iotests/083 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083 index 0306f112da..2f6444eeb9 100755 --- a/tests/qemu-iotests/083 +++ b/tests/qemu-iotests/083 @@ -86,6 +86,7 @@ EOF rm -f "$TEST_DIR/nbd.sock" +echo > "$TEST_DIR/nbd-fault-injector.out" $PYTHON nbd-fault-injector.py $extra_args "$nbd_addr" "$TEST_DIR/nbd-fault-injector.conf" >"$TEST_DIR/nbd-fault-injector.out" 2>&1 & # Wait for server to be ready @@ -94,7 +95,7 @@ EOF done # Extract the final address (port number has now been assigned in tcp case) - nbd_addr=$(sed 's/Listening on \(.*\)$/\1/' "$TEST_DIR/nbd-fault-injector.out") +nbd_addr=$(grep 'Listening on ' "$TEST_DIR/nbd-fault-injector.out" | sed 's/Listening on \(.*\)$/\1/') if [ "$proto" = "tcp" ]; then nbd_url="nbd+tcp://$nbd_addr/$export_name" -- 2.13.6
[Qemu-block] [PATCH 1/5] iotests: Make 030 less flaky
This patch fixes two race conditions in 030:
1. The first is in TestENSPC.test_enospc(). After resuming the job,
querying it to confirm it is no longer paused may fail because in the
meantime it might have completed already. The same was fixed in
TestEIO.test_ignore() already (in commit
2c3b44da07d341557a8203cc509ea07fe3605e11).
2. The second is in TestSetSpeed.test_set_speed_invalid(): Here, a
stream job is started on a drive without any break points, with a
block-job-set-speed invoked subsequently. However, without any break
points, the job might have completed in the meantime (on tmpfs at
least); or it might complete before cancel_and_wait() which expects
the job to still exist. This can be fixed like everywhere else by
pausing the drive (installing break points) before starting the job
and letting cancel_and_wait() resume it.
Signed-off-by: Max Reitz
---
tests/qemu-iotests/030 | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 18838948fa..457984b8e9 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -666,6 +666,7 @@ class TestENOSPC(TestErrors):
if event['event'] == 'BLOCK_JOB_ERROR':
self.assert_qmp(event, 'data/device', 'drive0')
self.assert_qmp(event, 'data/operation', 'read')
+error = True
result = self.vm.qmp('query-block-jobs')
self.assert_qmp(result, 'return[0]/paused', True)
@@ -676,9 +677,11 @@ class TestENOSPC(TestErrors):
self.assert_qmp(result, 'return', {})
result = self.vm.qmp('query-block-jobs')
+if result == {'return': []}:
+# Race; likely already finished. Check.
+continue
self.assert_qmp(result, 'return[0]/paused', False)
self.assert_qmp(result, 'return[0]/io-status', 'ok')
-error = True
elif event['event'] == 'BLOCK_JOB_COMPLETED':
self.assertTrue(error, 'job completed unexpectedly')
self.assert_qmp(event, 'data/type', 'stream')
@@ -792,13 +795,14 @@ class TestSetSpeed(iotests.QMPTestCase):
self.assert_no_active_block_jobs()
+self.vm.pause_drive('drive0')
result = self.vm.qmp('block-stream', device='drive0')
self.assert_qmp(result, 'return', {})
result = self.vm.qmp('block-job-set-speed', device='drive0', speed=-1)
self.assert_qmp(result, 'error/class', 'GenericError')
-self.cancel_and_wait()
+self.cancel_and_wait(resume=True)
if __name__ == '__main__':
iotests.main(supported_fmts=['qcow2', 'qed'])
--
2.13.6
[Qemu-block] [PATCH 2/5] iotests: Add missing 'blkdebug::' in 040
040 tries to invoke pause_drive() on a drive that does not use blkdebug.
Good idea, but let's use blkdebug to make it actually work.
Signed-off-by: Max Reitz
---
tests/qemu-iotests/040 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index c284d08796..90b5b4f2ad 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -289,7 +289,7 @@ class TestSetSpeed(ImageCommitTestCase):
qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' %
mid_img, test_img)
qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x1 0 512', test_img)
qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288',
mid_img)
-self.vm = iotests.VM().add_drive(test_img)
+self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
self.vm.launch()
def tearDown(self):
--
2.13.6
[Qemu-block] [PATCH 0/5] iotests: Make some tests less flaky
There are a couple of tests that fail (on my machine) from time to time (and by that I mean that recently I've rarely ever had a test run with both 083 and 136 working on first try). This series should fix most (at least the issues I am aware of). Notes: - 083 might have another issue, but if so it occurs extremely rarely and so I was unable to debug it. - 129 is flaky, too, because it tries to use block jobs with BB throttling -- however, block jobs ignore that these days. Making it use a throttle filter node will require quite a bit of work. See http://lists.nongnu.org/archive/html/qemu-block/2017-11/msg00111.html for more. - 194 sometimes hangs because the source VM fails to drain its drive. This is probably not an issue with the test, but actually an issue in qemu. See http://lists.nongnu.org/archive/html/qemu-block/2017-11/msg00256.html for more. "All tests have passed, let's ship it!" -- Me, 2:36 am (Editor's note: "all" means raw/file, qcow2/file, and raw/nbd.) Max Reitz (5): iotests: Make 030 less flaky iotests: Add missing 'blkdebug::' in 040 iotests: Make 055 less flaky iotests: Make 083 less flaky iotests: Make 136 less flaky tests/qemu-iotests/030 | 8 ++-- tests/qemu-iotests/040 | 2 +- tests/qemu-iotests/055 | 25 - tests/qemu-iotests/083 | 3 ++- tests/qemu-iotests/136 | 14 +- 5 files changed, 38 insertions(+), 14 deletions(-) -- 2.13.6
[Qemu-block] Intermittent hang of iotest 194 (bdrv_drain_all after non-shared storage migration)
Hi, More exciting news from the bdrv_drain() front! I've noticed in the past that iotest 194 sometimes hangs. I usually run the tests on tmpfs, but I've just now verified that it happens on my SSD just as well. So the reproducer is a plain: while ./check -raw 194; do; done (No difference between raw or qcow2, though.) And then, after a couple of runs (or a couple ten), it will just hang. The reason is that the source VM lingers around and doesn't quit voluntarily -- the test itself was successful, but it just can't exit. If you force it to exit by killing the VM (e.g. through pkill -11 qemu), this is the backtrace: #0 0x7f7cfc297e06 in ppoll () at /lib64/libc.so.6 #1 0x563b846bcac9 in ppoll (__ss=0x0, __timeout=0x0, __nfds=, __fds=) at /usr/include/bits/poll2.h:77 #2 0x563b846bcac9 in qemu_poll_ns (fds=, nfds=, timeout=) at util/qemu-timer.c:322 #3 0x563b846be711 in aio_poll (ctx=ctx@entry=0x563b856e3e80, blocking=) at util/aio-posix.c:629 #4 0x563b8463afa4 in bdrv_drain_recurse (bs=bs@entry=0x563b865568a0, begin=begin@entry=true) at block/io.c:201 #5 0x563b8463baff in bdrv_drain_all_begin () at block/io.c:381 #6 0x563b8463bc99 in bdrv_drain_all () at block/io.c:411 #7 0x563b8459888b in block_migration_cleanup (opaque=) at migration/block.c:714 #8 0x563b845883be in qemu_savevm_state_cleanup () at migration/savevm.c:1251 #9 0x563b845811fd in migration_thread (opaque=0x563b856f1da0) at migration/migration.c:2298 #10 0x7f7cfc56f36d in start_thread () at /lib64/libpthread.so.0 #11 0x7f7cfc2a3e1f in clone () at /lib64/libc.so.6 And when you make bdrv_drain_all_begin() print what we are trying to drain, you can see that it's the format node (managed by the "raw" driver in this case). So I thought, before I put more time into this, let's ask whether the test author has any ideas. :-) Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/7] various NBD fixes for 2.11
On 11/08/2017 04:24 PM, no-re...@patchew.org wrote:
> Hi,
>
> This series failed automatic build test. Please find the testing commands and
> their output below. If you have docker installed, you can probably reproduce
> it
> locally.
>
> 140 - output mismatch (see 140.out.bad)
> --- /tmp/qemu-test/src/tests/qemu-iotests/140.out 2017-11-08
> 22:20:18.0 +
> +++ /tmp/qemu-test/build/tests/qemu-iotests/140.out.bad 2017-11-08
> 22:22:51.888146974 +
> @@ -5,11 +5,11 @@
> {"return": {}}
> {"return": {}}
> {"return": {}}
> -read 65536/65536 bytes at offset 0
> -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: request for write
> access conflicts with read-only export
> {"return": {}}
Fixup in the form of 2.5/7 already sent.
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-de...@freelists.org
But I'm not sure if there is an easy way to tell patchew to include a
fixup patch and re-test the series, short of sending a v3.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-block] [PATCH v4 42/45] util: Replace fprintf(stderr, "*\n" with error_report()
Replace a large number of the fprintf(stderr, "*\n" calls with
error_report(). The functions were renamed with these commands and then
compiler issues where manually fixed.
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}'
\
{} +
find ./* -type f -exec sed -i \
'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
The error in aio_poll() was removed manually.
Signed-off-by: Alistair Francis
Cc: Kevin Wolf
Cc: Markus Armbruster
Cc: Paolo Bonzini
Cc: Stefan Weil
Cc: qemu-block@nongnu.org
Reviewed-by: Philippe Mathieu-Daudé
---
V4:
- Remove the error in aio_poll()
util/aio-posix.c | 1 +
util/coroutine-sigaltstack.c | 2 +-
util/error.c | 4 ++--
util/main-loop.c | 2 +-
util/mmap-alloc.c| 3 ++-
util/module.c| 6 +++---
util/osdep.c | 4 ++--
util/oslib-posix.c | 3 ++-
util/oslib-win32.c | 3 ++-
util/qemu-coroutine.c| 5 +++--
util/qemu-progress.c | 3 ++-
util/qemu-thread-posix.c | 5 +++--
util/qemu-thread-win32.c | 5 +++--
util/qemu-timer-common.c | 3 ++-
util/qht.c | 2 +-
15 files changed, 30 insertions(+), 21 deletions(-)
diff --git a/util/aio-posix.c b/util/aio-posix.c
index 1427f49b4a..8eeba93da6 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -15,6 +15,7 @@
#include "qemu/osdep.h"
#include "qemu-common.h"
+#include "qemu/error-report.h"
#include "block/block.h"
#include "qemu/rcu_queue.h"
#include "qemu/sockets.h"
diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
index f6fc49a0e5..96a01c2c88 100644
--- a/util/coroutine-sigaltstack.c
+++ b/util/coroutine-sigaltstack.c
@@ -80,7 +80,7 @@ static void __attribute__((constructor)) coroutine_init(void)
ret = pthread_key_create(&thread_state_key, qemu_coroutine_thread_cleanup);
if (ret != 0) {
-fprintf(stderr, "unable to create leader key: %s\n", strerror(errno));
+error_report("unable to create leader key: %s", strerror(errno));
abort();
}
}
diff --git a/util/error.c b/util/error.c
index 3efdd69162..f524e138ec 100644
--- a/util/error.c
+++ b/util/error.c
@@ -32,8 +32,8 @@ Error *error_fatal;
static void error_handle_fatal(Error **errp, Error *err)
{
if (errp == &error_abort) {
-fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
-err->func, err->src, err->line);
+error_report("Unexpected error in %s() at %s:%d:",
+ err->func, err->src, err->line);
error_report_err(err);
abort();
}
diff --git a/util/main-loop.c b/util/main-loop.c
index 7558eb5f53..d8369716b2 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -95,7 +95,7 @@ static int qemu_signal_init(void)
sigdelset(&set, SIG_IPI);
sigfd = qemu_signalfd(&set);
if (sigfd == -1) {
-fprintf(stderr, "failed to create signalfd\n");
+error_report("failed to create signalfd");
return -errno;
}
diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
index 3ec029a9ea..11887aac69 100644
--- a/util/mmap-alloc.c
+++ b/util/mmap-alloc.c
@@ -11,6 +11,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/error-report.h"
#include "qemu/mmap-alloc.h"
#include "qemu/host-utils.h"
@@ -51,7 +52,7 @@ size_t qemu_mempath_getpagesize(const char *mem_path)
} while (ret != 0 && errno == EINTR);
if (ret != 0) {
-fprintf(stderr, "Couldn't statfs() memory path: %s\n",
+error_report("Couldn't statfs() memory path: %s",
strerror(errno));
exit(1);
}
diff --git a/util/module.c b/util/module.c
index c90973721f..1153e3ebb0 100644
--- a/util/module.c
+++ b/util/modu
[Qemu-block] [PATCH v4 02/45] Replace all occurances of __FUNCTION__ with __func__
Replace all occurs of __FUNCTION__ except for the check in checkpatch
with the non GCC specific __func__.
One line in hcd-musb.c was manually tweaked to pass checkpatch.
Signed-off-by: Alistair Francis
Cc: Gerd Hoffmann
Cc: Andrzej Zaborowski
Cc: Stefano Stabellini
Cc: Anthony Perard
Cc: John Snow
Cc: Aurelien Jarno
Cc: Yongbok Kim
Cc: Peter Crosthwaite
Cc: Stefan Hajnoczi
Cc: Fam Zheng
Cc: Juan Quintela
Cc: "Dr. David Alan Gilbert"
Cc: qemu-...@nongnu.org
Cc: qemu-block@nongnu.org
Cc: xen-de...@lists.xenproject.org
Reviewed-by: Eric Blake
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Anthony PERARD
Reviewed-by: Juan Quintela
---
hw/arm/nseries.c | 2 +-
hw/arm/omap1.c | 42 +-
hw/arm/omap2.c | 12 ++--
hw/arm/palm.c | 14 +++---
hw/arm/pxa2xx.c| 46 +++---
hw/arm/pxa2xx_gpio.c | 6 +++---
hw/arm/pxa2xx_pic.c| 4 ++--
hw/arm/tosa.c | 10 +-
hw/audio/hda-codec.c | 10 +-
hw/audio/intel-hda.c | 28 ++--
hw/audio/wm8750.c | 4 ++--
hw/block/nand.c| 4 ++--
hw/block/onenand.c | 8
hw/bt/core.c | 10 +-
hw/bt/hci-csr.c| 14 +++---
hw/bt/hci.c| 26 +-
hw/bt/hid.c| 2 +-
hw/bt/l2cap.c | 22 +++---
hw/bt/sdp.c| 6 +++---
hw/display/blizzard.c | 18 +-
hw/display/omap_dss.c | 6 +++---
hw/display/pxa2xx_lcd.c| 14 +++---
hw/display/qxl-render.c| 6 +++---
hw/display/qxl.h | 2 +-
hw/display/tc6393xb.c | 2 +-
hw/display/xenfb.c | 2 +-
hw/dma/omap_dma.c | 26 +-
hw/dma/pxa2xx_dma.c| 14 +++---
hw/gpio/max7310.c | 8
hw/gpio/omap_gpio.c| 2 +-
hw/i2c/omap_i2c.c | 6 +++---
hw/ide/ahci.c | 2 +-
hw/ide/microdrive.c| 4 ++--
hw/input/lm832x.c | 6 +++---
hw/input/pxa2xx_keypad.c | 6 +++---
hw/input/tsc2005.c | 8
hw/input/tsc210x.c | 4 ++--
hw/intc/omap_intc.c| 2 +-
hw/isa/vt82c686.c | 2 +-
hw/mips/gt64xxx_pci.c | 2 +-
hw/misc/cbus.c | 12 ++--
hw/misc/omap_clk.c | 4 ++--
hw/misc/omap_gpmc.c| 6 +++---
hw/misc/omap_l4.c | 4 ++--
hw/misc/omap_sdrc.c| 2 +-
hw/misc/omap_tap.c | 6 +++---
hw/misc/tmp105.c | 2 +-
hw/pci-host/bonito.c | 2 +-
hw/sd/pxa2xx_mmci.c| 6 +++---
hw/ssi/omap_spi.c | 6 +++---
hw/timer/omap_gptimer.c| 6 +++---
hw/timer/twl92230.c| 6 +++---
hw/usb/desc.c | 2 +-
hw/usb/dev-bluetooth.c | 4 ++--
hw/usb/hcd-musb.c | 4 ++--
hw/usb/tusb6010.c | 14 +++---
hw/xenpv/xen_domainbuild.c | 16
hw/xenpv/xen_machine_pv.c | 2 +-
include/hw/arm/omap.h | 10 +-
include/hw/arm/sharpsl.h | 2 +-
memory_mapping.c | 2 +-
migration/block.c | 4 ++--
ui/cursor.c| 6 +++---
ui/spice-display.c | 4 ++--
64 files changed, 272 insertions(+), 272 deletions(-)
diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
index 58005b6619..32687afced 100644
--- a/hw/arm/nseries.c
+++ b/hw/arm/nseries.c
@@ -463,7 +463,7 @@ static uint32_t mipid_txrx(void *opaque, uint32_t cmd, int
len)
uint8_t ret;
if (len > 9) {
-hw_error("%s: FIXME: bad SPI word width %i\n", __FUNCTION__, len);
+hw_error("%s: FIXME: bad SPI word width %i\n", __func__, len);
}
if (s->p >= ARRAY_SIZE(s->resp)) {
diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
index b3e7625130..1388200191 100644
--- a/hw/arm/omap1.c
+++ b/hw/arm/omap1.c
@@ -999,7 +999,7 @@ static uint64_t omap_id_read(void *opaque, hwaddr addr,
case omap1510:
return 0x03310115;
default:
-hw_error("%s: bad mpu model\n", __FUNCTION__);
+hw_error("%s: bad mpu model\n", __func__);
}
break;
@@ -1010,7 +1010,7 @@ static uint64_t omap_id_read(void *opaque, hwaddr addr,
case omap1510:
return 0xfb47002f;
default:
-hw_error("%s: bad mpu model\n", __FUNCTION__);
+hw_error("%s: bad mpu model\n", __func__);
}
break;
}
@@ -1716,7 +1716,7 @@ static void omap_clkm_write(void *opaque, hwaddr addr,
case 0x18: /* ARM_SYSST */
if ((s->clkm.clocking_scheme ^ (value >> 11)) & 7) {
s->clkm.clocking_scheme = (value >> 11) & 7;
-printf("%s: clocking scheme set to %s\n", __FUNCTION__,
+printf("%s: clocking scheme set to %s\n", __func__,
clks
[Qemu-block] [PATCH v4 16/45] hw/ide: Replace fprintf(stderr, "*\n" with error_report()
Replace a large number of the fprintf(stderr, "*\n" calls with
error_report(). The functions were renamed with these commands and then
compiler issues where manually fixed.
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}'
\
{} +
find ./* -type f -exec sed -i \
'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
Some lines where then manually tweaked to pass checkpatch.
Signed-off-by: Alistair Francis
Cc: qemu-block@nongnu.org
Reviewed-by: Philippe Mathieu-Daud
Acked-by: John Snow
---
V2:
- Split hw patch into individual directories
hw/ide/ahci.c | 6 +++---
hw/ide/core.c | 3 +--
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index b72dcbcc32..b87d1903c1 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -410,8 +410,8 @@ static void ahci_mem_write(void *opaque, hwaddr addr,
/* Only aligned reads are allowed on AHCI */
if (addr & 3) {
-fprintf(stderr, "ahci: Mis-aligned write to addr 0x"
-TARGET_FMT_plx "\n", addr);
+error_report("ahci: Mis-aligned write to addr 0x" TARGET_FMT_plx,
+ addr);
return;
}
@@ -1053,7 +1053,7 @@ static void process_ncq_command(AHCIState *s, int port,
uint8_t *cmd_fis,
g_assert(is_ncq(ncq_fis->command));
if (ncq_tfs->used) {
/* error - already in use */
-fprintf(stderr, "%s: tag %d already used\n", __func__, tag);
+error_report("%s: tag %d already used", __func__, tag);
return;
}
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 471d0c928b..211d67a625 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2765,8 +2765,7 @@ static int ide_drive_pio_pre_save(void *opaque)
idx = transfer_end_table_idx(s->end_transfer_func);
if (idx == -1) {
-fprintf(stderr, "%s: invalid end_transfer_func for DRQ_STAT\n",
-__func__);
+error_report("%s: invalid end_transfer_func for DRQ_STAT", __func__);
s->end_transfer_fn_idx = 2;
} else {
s->end_transfer_fn_idx = idx;
--
2.14.1
[Qemu-block] [PATCH v4 06/45] hw/block: Replace fprintf(stderr, "*\n" with error_report()
Replace a large number of the fprintf(stderr, "*\n" calls with
error_report(). The functions were renamed with these commands and then
compiler issues where manually fixed.
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}'
\
{} +
find ./* -type f -exec sed -i \
'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
Some lines where then manually tweaked to pass checkpatch.
Signed-off-by: Alistair Francis
Cc: qemu-block@nongnu.org
---
V2:
- Split hw patch into individual directories
hw/block/dataplane/virtio-blk.c | 6 +++---
hw/block/onenand.c | 8
hw/block/tc58128.c | 44 +++--
3 files changed, 27 insertions(+), 31 deletions(-)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 5556f0e64e..69dfd49191 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -178,8 +178,8 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
/* Set up guest notifier (irq) */
r = k->set_guest_notifiers(qbus->parent, nvqs, true);
if (r != 0) {
-fprintf(stderr, "virtio-blk failed to set guest notifier (%d), "
-"ensure -enable-kvm is set\n", r);
+error_report("virtio-blk failed to set guest notifier (%d), "
+"ensure -enable-kvm is set", r);
goto fail_guest_notifiers;
}
@@ -187,7 +187,7 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
for (i = 0; i < nvqs; i++) {
r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, true);
if (r != 0) {
-fprintf(stderr, "virtio-blk failed to set host notifier (%d)\n",
r);
+error_report("virtio-blk failed to set host notifier (%d)", r);
while (i--) {
virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
}
diff --git a/hw/block/onenand.c b/hw/block/onenand.c
index ed77f859e9..9591e9e82c 100644
--- a/hw/block/onenand.c
+++ b/hw/block/onenand.c
@@ -596,7 +596,7 @@ static void onenand_command(OneNANDState *s)
default:
s->status |= ONEN_ERR_CMD;
s->intstatus |= ONEN_INT;
-fprintf(stderr, "%s: unknown OneNAND command %x\n",
+error_report("%s: unknown OneNAND command %x",
__func__, s->command);
}
@@ -663,7 +663,7 @@ static uint64_t onenand_read(void *opaque, hwaddr addr,
return 0x;
}
-fprintf(stderr, "%s: unknown OneNAND register %x\n",
+error_report("%s: unknown OneNAND register %x",
__func__, offset);
return 0;
}
@@ -708,7 +708,7 @@ static void onenand_write(void *opaque, hwaddr addr,
break;
default:
-fprintf(stderr, "%s: unknown OneNAND boot command %"PRIx64"\n",
+error_report("%s: unknown OneNAND boot command %"PRIx64"",
__func__, value);
}
break;
@@ -759,7 +759,7 @@ static void onenand_write(void *opaque, hwaddr addr,
break;
default:
-fprintf(stderr, "%s: unknown OneNAND register %x\n",
+error_report("%s: unknown OneNAND register %x",
__func__, offset);
}
}
diff --git a/hw/block/tc58128.c b/hw/block/tc58128.c
index 1d9f7ee000..d274c9aafe 100644
--- a/hw/block/tc58128.c
+++ b/hw/block/tc58128.c
@@ -50,8 +50,8 @@ static void init_dev(tc58128_dev * dev, const char *filename)
dev->flash_contents[1] = (blocks >> 8) & 0xff;
dev->flash_contents[2] = (blocks >> 16) & 0xff;
dev->flash_contents[3] = (blocks >> 24) & 0xff;
- fprintf(stderr, "loaded %d bytes for %s into flash\n", ret,
- filename);
+err
[Qemu-block] [PATCH v4 04/45] tests: Replace fprintf(stderr, "*\n" with error_report()
Replace a large number of the fprintf(stderr, "*\n" calls with
error_report(). The functions were renamed with these commands and then
compiler issues where manually fixed.
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N;N; {s|fprintf(stderr,
"\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}'
\
{} +
find ./* -type f -exec sed -i \
'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
find ./* -type f -exec sed -i \
'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
{} +
Some of the error_report()'s were manually kept as fprintf(stderr, ) to
maintain standalone test cases.
Signed-off-by: Alistair Francis
Cc: "Michael S. Tsirkin"
Cc: Igor Mammedov
Cc: "Dr. David Alan Gilbert"
Cc: Gerd Hoffmann
Cc: qemu-block@nongnu.org
---
V2:
- Keep some of the fprintf() calls
- Remove a file I accidently checked in
tests/Makefile.include | 4 ++--
tests/ahci-test.c | 3 ++-
tests/bios-tables-test.c | 5 +++--
tests/i440fx-test.c| 8
tests/libqos/ahci.c| 10 +-
tests/libqos/libqos.c | 7 ---
tests/libqos/malloc.c | 13 ++--
tests/libqtest.c | 13 ++--
tests/migration-test.c | 8
tests/migration/stress.c | 36 +-
tests/qemu-iotests/socket_scm_helper.c | 11 ++-
tests/rcutorture.c | 5 +++--
tests/tcg/linux-test.c | 2 +-
tests/tcg/runcom.c | 6 +++---
tests/tcg/test-i386-fprem.c| 2 +-
tests/tcg/test_path.c | 4 ++--
tests/test-hmp.c | 5 +++--
tests/test-rcu-list.c | 5 +++--
tests/usb-hcd-ehci-test.c | 2 +-
tests/vhost-user-bridge.c | 28 +-
20 files changed, 93 insertions(+), 84 deletions(-)
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 434a2ce868..5fcd63eda5 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -621,7 +621,7 @@ tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
$(test-io-obj-y)
tests/test-timed-average$(EXESUF): tests/test-timed-average.o
$(test-util-obj-y)
tests/test-base64$(EXESUF): tests/test-base64.o $(test-util-obj-y)
-tests/ptimer-test$(EXESUF): tests/ptimer-test.o tests/ptimer-test-stubs.o
hw/core/ptimer.o
+tests/ptimer-test$(EXESUF): tests/ptimer-test.o tests/ptimer-test-stubs.o
hw/core/ptimer.o $(test-util-obj-y)
tests/test-logging$(EXESUF): tests/test-logging.o $(test-util-obj-y)
@@ -788,7 +788,7 @@ tests/migration-test$(EXESUF): tests/migration-test.o
tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o $(test-util-obj-y) \
$(qtest-obj-y) $(test-io-obj-y) $(libqos-virtio-obj-y)
$(libqos-pc-obj-y) \
$(chardev-obj-y)
-tests/qemu-iotests/socket_scm_helper$(EXESUF):
tests/qemu-iotests/socket_scm_helper.o
+tests/qemu-iotests/socket_scm_helper$(EXESUF):
tests/qemu-iotests/socket_scm_helper.o $(test-util-obj-y)
tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o $(test-util-obj-y)
tests/test-keyval$(EXESUF): tests/test-keyval.o $(test-util-obj-y)
$(test-qapi-obj-y)
tests/test-write-threshold$(EXESUF): tests/test-write-threshold.o
$(test-block-obj-y)
diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index cb84edc8fb..ac8a30ab8b 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -23,6 +23,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/error-report.h"
#include
#include "libqtest.h"
@@ -1859,7 +1860,7 @@ int main(int argc, char **argv)
ahci_pedantic = 1;
break;
default:
-fprintf(stderr, "Unrecognized ahci_test option.\n");
+error_report("Unrecognized ahci_test option.");
g_assert_not_reached
[Qemu-block] [PATCH v2 2.5/7] fixup! nbd-client: Refuse read-only client with BDRV_O_RDWR
...
Fix several iotests to comply with the new behavior (since
qemu-nbd of an internal snapshot, as well as nbd-server-add over QMP,
default to a read-only export, we must tell blockdev-add/qemu-io to
set up a read-only client).
Signed-off-by: Eric Blake
---
Will squash when applying this series to the NBD queue.
---
tests/qemu-iotests/058 | 8
tests/qemu-iotests/140 | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/058 b/tests/qemu-iotests/058
index 2253c6a6d1..5eb8784669 100755
--- a/tests/qemu-iotests/058
+++ b/tests/qemu-iotests/058
@@ -117,15 +117,15 @@ _export_nbd_snapshot sn1
echo
echo "== verifying the exported snapshot with patterns, method 1 =="
-$QEMU_IO_NBD -c 'read -P 0xa 0x1000 0x1000' "$nbd_snapshot_img" |
_filter_qemu_io
-$QEMU_IO_NBD -c 'read -P 0xb 0x2000 0x1000' "$nbd_snapshot_img" |
_filter_qemu_io
+$QEMU_IO_NBD -r -c 'read -P 0xa 0x1000 0x1000' "$nbd_snapshot_img" |
_filter_qemu_io
+$QEMU_IO_NBD -r -c 'read -P 0xb 0x2000 0x1000' "$nbd_snapshot_img" |
_filter_qemu_io
_export_nbd_snapshot1 sn1
echo
echo "== verifying the exported snapshot with patterns, method 2 =="
-$QEMU_IO_NBD -c 'read -P 0xa 0x1000 0x1000' "$nbd_snapshot_img" |
_filter_qemu_io
-$QEMU_IO_NBD -c 'read -P 0xb 0x2000 0x1000' "$nbd_snapshot_img" |
_filter_qemu_io
+$QEMU_IO_NBD -r -c 'read -P 0xa 0x1000 0x1000' "$nbd_snapshot_img" |
_filter_qemu_io
+$QEMU_IO_NBD -r -c 'read -P 0xb 0x2000 0x1000' "$nbd_snapshot_img" |
_filter_qemu_io
$QEMU_IMG convert "$TEST_IMG" -l sn1 -O qcow2 "$converted_image"
diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
index f89d0d6789..a8fc95145c 100755
--- a/tests/qemu-iotests/140
+++ b/tests/qemu-iotests/140
@@ -78,7 +78,7 @@ _send_qemu_cmd $QEMU_HANDLE \
'arguments': { 'device': 'drv' }}" \
'return'
-$QEMU_IO_PROG -f raw -c 'read -P 42 0 64k' \
+$QEMU_IO_PROG -f raw -r -c 'read -P 42 0 64k' \
"nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
| _filter_qemu_io | _filter_nbd
@@ -87,7 +87,7 @@ _send_qemu_cmd $QEMU_HANDLE \
'arguments': { 'device': 'drv' }}" \
'return'
-$QEMU_IO_PROG -f raw -c close \
+$QEMU_IO_PROG -f raw -r -c close \
"nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
| _filter_qemu_io | _filter_nbd
--
2.13.6
Re: [Qemu-block] [Qemu-devel] [PATCH v2 2/7] nbd-client: Refuse read-only client with BDRV_O_RDWR
On 11/08/2017 03:56 PM, Eric Blake wrote: > Fix iotest 147 to comply with the new behavior (since nbd-server-add > defaults to a read-only export, we must tell blockdev-add to set up a > read-only client). My bad for testing only './check -nbd' and not also './check -qcow2'; iotest 58 and 140 also need fixing. I'll post the patch for squashing while awaiting review; I'd like for this to make it into -rc1. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-block] [PATCH v2 5/7] nbd-client: Short-circuit 0-length operations
The NBD spec was recently clarified to state that clients should
not send 0-length requests to the server, as the server behavior
is undefined [1]. We know that qemu-nbd's behavior is a successful
no-op (once it has filtered for read-only exports), but other NBD
implementations might return an error. To avoid any questionable
server implementations, it is better to just short-circuit such
requests on the client side (we are relying on the block layer to
already filter out requests such as invalid offset, write to a
read-only volume, and so forth).
[1] https://github.com/NetworkBlockDevice/nbd/commit/ee926037
Signed-off-by: Eric Blake
---
block/nbd-client.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index daa4392531..0a675d0fab 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -674,6 +674,9 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t
offset,
assert(bytes <= NBD_MAX_BUFFER_SIZE);
assert(!flags);
+if (!bytes) {
+return 0;
+}
ret = nbd_co_send_request(bs, &request, NULL);
if (ret < 0) {
return ret;
@@ -705,6 +708,9 @@ int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t
offset,
assert(bytes <= NBD_MAX_BUFFER_SIZE);
+if (!bytes) {
+return 0;
+}
return nbd_co_request(bs, &request, qiov);
}
@@ -731,6 +737,9 @@ int nbd_client_co_pwrite_zeroes(BlockDriverState *bs,
int64_t offset,
request.flags |= NBD_CMD_FLAG_NO_HOLE;
}
+if (!bytes) {
+return 0;
+}
return nbd_co_request(bs, &request, NULL);
}
@@ -759,7 +768,7 @@ int nbd_client_co_pdiscard(BlockDriverState *bs, int64_t
offset, int bytes)
};
assert(!(client->info.flags & NBD_FLAG_READ_ONLY));
-if (!(client->info.flags & NBD_FLAG_SEND_TRIM)) {
+if (!(client->info.flags & NBD_FLAG_SEND_TRIM) || !bytes) {
return 0;
}
--
2.13.6
[Qemu-block] [PATCH v2 6/7] nbd-client: Stricter enforcing of structured reply spec
Ensure that the server is not sending unexpected chunk lengths
for either the NONE or the OFFSET_DATA chunk, nor unexpected
hole length for OFFSET_HOLE. This will flag any server that
responds to a zero-length read with an OFFSET_DATA as broken,
even though we previously fixed our client to never be able to
send such a request over the wire.
Signed-off-by: Eric Blake
---
block/nbd-client.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 0a675d0fab..73c9fe0905 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -216,7 +216,7 @@ static int
nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk,
offset = payload_advance64(&payload);
hole_size = payload_advance32(&payload);
-if (offset < orig_offset || hole_size > qiov->size ||
+if (!hole_size || offset < orig_offset || hole_size > qiov->size ||
offset > orig_offset + qiov->size - hole_size) {
error_setg(errp, "Protocol error: server sent chunk exceeding
requested"
" region");
@@ -281,7 +281,8 @@ static int
nbd_co_receive_offset_data_payload(NBDClientSession *s,
assert(nbd_reply_is_structured(&s->reply));
-if (chunk->length < sizeof(offset)) {
+/* The NBD spec requires at least one byte of payload */
+if (chunk->length <= sizeof(offset)) {
error_setg(errp, "Protocol error: invalid payload for "
"NBD_REPLY_TYPE_OFFSET_DATA");
return -EINVAL;
@@ -293,7 +294,7 @@ static int
nbd_co_receive_offset_data_payload(NBDClientSession *s,
be64_to_cpus(&offset);
data_size = chunk->length - sizeof(offset);
-if (offset < orig_offset || data_size > qiov->size ||
+if (!data_size || offset < orig_offset || data_size > qiov->size ||
offset > orig_offset + qiov->size - data_size) {
error_setg(errp, "Protocol error: server sent chunk exceeding
requested"
" region");
@@ -411,6 +412,11 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
" NBD_REPLY_FLAG_DONE flag set");
return -EINVAL;
}
+if (chunk->length) {
+error_setg(errp, "Protocol error: NBD_REPLY_TYPE_NONE chunk with"
+ " nonzero length");
+return -EINVAL;
+}
return 0;
}
--
2.13.6
[Qemu-block] [PATCH v2 3/7] nbd/client: Nicer trace of structured reply
It's useful to know which structured reply chunk is being processed.
Signed-off-by: Eric Blake
---
nbd/client.c | 4 +++-
nbd/trace-events | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index 3d680e63e1..1880103d2a 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -979,6 +979,7 @@ static int nbd_receive_structured_reply_chunk(QIOChannel
*ioc,
int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
{
int ret;
+const char *type;
ret = nbd_read_eof(ioc, &reply->magic, sizeof(reply->magic), errp);
if (ret <= 0) {
@@ -1008,8 +1009,9 @@ int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply,
Error **errp)
if (ret < 0) {
break;
}
+type = nbd_reply_type_lookup(reply->structured.type);
trace_nbd_receive_structured_reply_chunk(reply->structured.flags,
- reply->structured.type,
+ reply->structured.type, type,
reply->structured.handle,
reply->structured.length);
break;
diff --git a/nbd/trace-events b/nbd/trace-events
index 4a13757524..bbc75f6414 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -27,7 +27,7 @@ nbd_client_clear_queue(void) "Clearing NBD queue"
nbd_client_clear_socket(void) "Clearing NBD socket"
nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags,
uint16_t type, const char *name) "Sending request to server: { .from = %"
PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ",
.type = %" PRIu16 " (%s) }"
nbd_receive_simple_reply(int32_t error, const char *errname, uint64_t handle)
"Got simple reply: { .error = %" PRId32 " (%s), handle = %" PRIu64" }"
-nbd_receive_structured_reply_chunk(uint16_t flags, uint16_t type, uint64_t
handle, uint32_t length) "Got structured reply chunk: { flags = 0x%" PRIx16 ",
type = %d, handle = %" PRIu64 ", length = %" PRIu32 " }"
+nbd_receive_structured_reply_chunk(uint16_t flags, uint16_t type, const char
*name, uint64_t handle, uint32_t length) "Got structured reply chunk: { flags =
0x%" PRIx16 ", type = %d (%s), handle = %" PRIu64 ", length = %" PRIu32 " }"
# nbd/common.c
nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL"
--
2.13.6
[Qemu-block] [PATCH v2 4/7] nbd: Fix struct name for structured reads
A closer read of the NBD spec shows that a structured reply chunk
for a hole is not quite identical to the prefix of a data chunk,
because the hole has to also send a 32-bit size field. Although
we do not yet send holes, we should fix the misleading information
in our header and make it easier for a future patch to support
sparse reads.
Signed-off-by: Eric Blake
---
include/block/nbd.h | 18 +-
nbd/server.c| 2 +-
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 92d1723d7c..113c707a5e 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -86,15 +86,23 @@ typedef union NBDReply {
} QEMU_PACKED;
} NBDReply;
-/* Header of NBD_REPLY_TYPE_OFFSET_DATA, complete NBD_REPLY_TYPE_OFFSET_HOLE */
-typedef struct NBDStructuredRead {
-NBDStructuredReplyChunk h;
+/* Header of chunk for NBD_REPLY_TYPE_OFFSET_DATA */
+typedef struct NBDStructuredReadData {
+NBDStructuredReplyChunk h; /* h.length >= 9 */
uint64_t offset;
-} QEMU_PACKED NBDStructuredRead;
+/* At least one byte of data payload follows, calculated from h.length */
+} QEMU_PACKED NBDStructuredReadData;
+
+/* Complete chunk for NBD_REPLY_TYPE_OFFSET_HOLE */
+typedef struct NBDStructuredReadHole {
+NBDStructuredReplyChunk h; /* h.length == 12 */
+uint64_t offset;
+uint32_t length;
+} QEMU_PACKED NBDStructuredReadHole;
/* Header of all NBD_REPLY_TYPE_ERROR* errors */
typedef struct NBDStructuredError {
-NBDStructuredReplyChunk h;
+NBDStructuredReplyChunk h; /* h.length >= 6 */
uint32_t error;
uint16_t message_length;
} QEMU_PACKED NBDStructuredError;
diff --git a/nbd/server.c b/nbd/server.c
index bcf0cdb47c..6ebb7d9c2e 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1280,7 +1280,7 @@ static int coroutine_fn
nbd_co_send_structured_read(NBDClient *client,
size_t size,
Error **errp)
{
-NBDStructuredRead chunk;
+NBDStructuredReadData chunk;
struct iovec iov[] = {
{.iov_base = &chunk, .iov_len = sizeof(chunk)},
{.iov_base = data, .iov_len = size}
--
2.13.6
[Qemu-block] [PATCH v2 1/7] nbd-client: Fix error message typos
Provide missing spaces that are required when using string
concatenation to break error messages across source lines.
Signed-off-by: Eric Blake
---
block/nbd-client.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index b44d4d4a01..de6c153328 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -248,7 +248,7 @@ static int nbd_parse_error_payload(NBDStructuredReplyChunk
*chunk,
error = nbd_errno_to_system_errno(payload_advance32(&payload));
if (error == 0) {
-error_setg(errp, "Protocol error: server sent structured error chunk"
+error_setg(errp, "Protocol error: server sent structured error chunk "
"with error = 0");
return -EINVAL;
}
@@ -257,7 +257,7 @@ static int nbd_parse_error_payload(NBDStructuredReplyChunk
*chunk,
message_size = payload_advance16(&payload);
if (message_size > chunk->length - sizeof(error) - sizeof(message_size)) {
-error_setg(errp, "Protocol error: server sent structured error chunk"
+error_setg(errp, "Protocol error: server sent structured error chunk "
"with incorrect message size");
return -EINVAL;
}
@@ -408,7 +408,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
if (chunk->type == NBD_REPLY_TYPE_NONE) {
if (!(chunk->flags & NBD_REPLY_FLAG_DONE)) {
error_setg(errp, "Protocol error: NBD_REPLY_TYPE_NONE chunk
without"
- "NBD_REPLY_FLAG_DONE flag set");
+ " NBD_REPLY_FLAG_DONE flag set");
return -EINVAL;
}
return 0;
--
2.13.6
[Qemu-block] [PATCH v2 7/7] nbd/server: Fix structured read of length 0
The NBD spec was recently clarified to state that a read of length 0
should not be attempted by a compliant client; but that a server must
still handle it correctly in an unspecified manner (that is, either
a successful no-op or an error reply, but not a crash) [1]. However,
it also implies that NBD_REPLY_TYPE_OFFSET_DATA must have a non-zero
payload length, but our existing code was replying with a chunk
that a picky client could reject as invalid because it was missing
a payload.
We are already doing successful no-ops for 0-length writes and for
non-structured reads; so for consistency, we want structured reply
reads to also be a no-op. The easiest way to do this is to return
a NBD_REPLY_TYPE_NONE chunk; this is best done via a new helper
function (especially since future patches for other structured
replies may benefit from using the same helper).
[1] https://github.com/NetworkBlockDevice/nbd/commit/ee926037
Signed-off-by: Eric Blake
---
nbd/server.c | 21 -
nbd/trace-events | 1 +
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/nbd/server.c b/nbd/server.c
index 6ebb7d9c2e..df771fd42f 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1273,6 +1273,21 @@ static inline void set_be_chunk(NBDStructuredReplyChunk
*chunk, uint16_t flags,
stl_be_p(&chunk->length, length);
}
+static int coroutine_fn nbd_co_send_structured_done(NBDClient *client,
+uint64_t handle,
+Error **errp)
+{
+NBDStructuredReplyChunk chunk;
+struct iovec iov[] = {
+{.iov_base = &chunk, .iov_len = sizeof(chunk)},
+};
+
+trace_nbd_co_send_structured_done(handle);
+set_be_chunk(&chunk, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_NONE, handle, 0);
+
+return nbd_co_send_iov(client, iov, 1, errp);
+}
+
static int coroutine_fn nbd_co_send_structured_read(NBDClient *client,
uint64_t handle,
uint64_t offset,
@@ -1286,6 +1301,7 @@ static int coroutine_fn
nbd_co_send_structured_read(NBDClient *client,
{.iov_base = data, .iov_len = size}
};
+assert(size);
trace_nbd_co_send_structured_read(handle, offset, data, size);
set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_OFFSET_DATA,
handle, sizeof(chunk) - sizeof(chunk.h) + size);
@@ -1544,10 +1560,13 @@ reply:
if (ret < 0) {
ret = nbd_co_send_structured_error(req->client, request.handle,
-ret, msg, &local_err);
-} else {
+} else if (reply_data_len) {
ret = nbd_co_send_structured_read(req->client, request.handle,
request.from, req->data,
reply_data_len, &local_err);
+} else {
+ret = nbd_co_send_structured_done(req->client, request.handle,
+ &local_err);
}
} else {
ret = nbd_co_send_simple_reply(req->client, request.handle,
diff --git a/nbd/trace-events b/nbd/trace-events
index bbc75f6414..92568edce5 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -55,6 +55,7 @@ nbd_receive_request(uint32_t magic, uint16_t flags, uint16_t
type, uint64_t from
nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching
clients to AIO context %p\n"
nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients
from AIO context %p\n"
nbd_co_send_simple_reply(uint64_t handle, uint32_t error, const char *errname,
int len) "Send simple reply: handle = %" PRIu64 ", error = %" PRIu32 " (%s),
len = %d"
+nbd_co_send_structured_done(uint64_t handle) "Send structured reply done:
handle = %" PRIu64
nbd_co_send_structured_read(uint64_t handle, uint64_t offset, void *data,
size_t size) "Send structured read data reply: handle = %" PRIu64 ", offset =
%" PRIu64 ", data = %p, len = %zu"
nbd_co_send_structured_error(uint64_t handle, int err, const char *errname,
const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d
(%s), msg = '%s'"
nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char
*name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)"
--
2.13.6
[Qemu-block] [PATCH v2 0/7] various NBD fixes for 2.11
Sparked by my question on whether we were handling zero-length reads correctly according to the NBD spec, but grew to fix other things as well such as some typos, tracing weaknesses, and better handling of read-only exports. The order these patches are listed here is bisection-friendly, but applying the patches out of order is necessary to catch some of the bad behavior being fixed. As evidenced by the iotest 147 change, this WILL flag existing clients with dodgy usage when connecting to a read-only server (such as the default for nbd-server-add). But on the bright side, when libvirt drives storage migration via NBD, it is properly setting up a read-write server with nbd-server-add. Since v1 (https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg00955.html): - Tweak several commit messages now that NBD spec changes landed - merge old 3/8 and 4/8 into single 2/7 that blocks read-write connection to read-only export rather than magically downgrading it [Kevin] 001/7:[] [--] 'nbd-client: Fix error message typos' 002/7:[down] 'nbd-client: Refuse read-only client with BDRV_O_RDWR' 003/7:[] [--] 'nbd/client: Nicer trace of structured reply' 004/7:[] [--] 'nbd: Fix struct name for structured reads' 005/7:[] [-C] 'nbd-client: Short-circuit 0-length operations' 006/7:[] [--] 'nbd-client: Stricter enforcing of structured reply spec' 007/7:[] [--] 'nbd/server: Fix structured read of length 0' Eric Blake (7): nbd-client: Fix error message typos nbd-client: Refuse read-only client with BDRV_O_RDWR nbd/client: Nicer trace of structured reply nbd: Fix struct name for structured reads nbd-client: Short-circuit 0-length operations nbd-client: Stricter enforcing of structured reply spec nbd/server: Fix structured read of length 0 include/block/nbd.h| 18 +- block/nbd-client.c | 38 +++--- nbd/client.c | 4 +++- nbd/server.c | 23 +-- nbd/trace-events | 3 ++- tests/qemu-iotests/147 | 1 + 6 files changed, 71 insertions(+), 16 deletions(-) -- 2.13.6
[Qemu-block] [PATCH v2 2/7] nbd-client: Refuse read-only client with BDRV_O_RDWR
The NBD spec says that clients should not try to write/trim to
an export advertised as read-only by the server. But we failed
to check that, and would allow the block layer to use NBD with
BDRV_O_RDWR even when the server is read-only, which meant we
were depending on the server sending a proper EPERM failure for
various commands, and also exposes a leaky abstraction: using
qemu-io in read-write mode would succeed on 'w -z 0 0' because
of local short-circuiting logic, but 'w 0 0' would send a
request over the wire (where it then depends on the server, and
fails at least for qemu-nbd but might pass for other NBD
implementations).
With this patch, a client MUST request read-only mode to access
a server that is doing a read-only export, or else it will get
a message like:
can't open device nbd://localhost:10809/foo: request for write access conflicts
with read-only export
It is no longer possible to even attempt writes over the wire
(including the corner case of 0-length writes), because the block
layer enforces the explicit read-only request; this matches the
behavior of qcow2 when backed by a read-only POSIX file.
Fix iotest 147 to comply with the new behavior (since nbd-server-add
defaults to a read-only export, we must tell blockdev-add to set up a
read-only client).
CC: qemu-sta...@nongnu.org
Signed-off-by: Eric Blake
---
v2: retitle; require explicit read-only request, rather than magically
setting it (includes iotest fallout) [Kevin]
---
block/nbd-client.c | 9 +
tests/qemu-iotests/147 | 1 +
2 files changed, 10 insertions(+)
diff --git a/block/nbd-client.c b/block/nbd-client.c
index de6c153328..daa4392531 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -697,6 +697,7 @@ int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t
offset,
.len = bytes,
};
+assert(!(client->info.flags & NBD_FLAG_READ_ONLY));
if (flags & BDRV_REQ_FUA) {
assert(client->info.flags & NBD_FLAG_SEND_FUA);
request.flags |= NBD_CMD_FLAG_FUA;
@@ -717,6 +718,7 @@ int nbd_client_co_pwrite_zeroes(BlockDriverState *bs,
int64_t offset,
.len = bytes,
};
+assert(!(client->info.flags & NBD_FLAG_READ_ONLY));
if (!(client->info.flags & NBD_FLAG_SEND_WRITE_ZEROES)) {
return -ENOTSUP;
}
@@ -756,6 +758,7 @@ int nbd_client_co_pdiscard(BlockDriverState *bs, int64_t
offset, int bytes)
.len = bytes,
};
+assert(!(client->info.flags & NBD_FLAG_READ_ONLY));
if (!(client->info.flags & NBD_FLAG_SEND_TRIM)) {
return 0;
}
@@ -814,6 +817,12 @@ int nbd_client_init(BlockDriverState *bs,
logout("Failed to negotiate with the NBD server\n");
return ret;
}
+if (client->info.flags & NBD_FLAG_READ_ONLY &&
+!bdrv_is_read_only(bs)) {
+error_setg(errp,
+ "request for write access conflicts with read-only export");
+return -EACCES;
+}
if (client->info.flags & NBD_FLAG_SEND_FUA) {
bs->supported_write_flags = BDRV_REQ_FUA;
bs->supported_zero_flags |= BDRV_REQ_FUA;
diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
index db34838cd0..90f40ed245 100755
--- a/tests/qemu-iotests/147
+++ b/tests/qemu-iotests/147
@@ -43,6 +43,7 @@ class NBDBlockdevAddBase(iotests.QMPTestCase):
'driver': 'raw',
'file': {
'driver': 'nbd',
+'read-only': True,
'server': address
} }
if export is not None:
--
2.13.6
Re: [Qemu-block] Drainage in bdrv_replace_child_noperm()
On 2017-11-07 15:22, Kevin Wolf wrote:
> Am 06.11.2017 um 19:49 hat Max Reitz geschrieben:
>> Hi everyone,
>>
>> On my quest to fix some flaky iotests, I came to a bit of a halt on 129.
>> (Details: Its issue is that block jobs now generally ignore throttling
>> in a BB (because they use their own), so we have to add a throttle node
>> instead. However, when I added it, I got an abort.)
>>
>> My issue can be reproduced as follows:
>>
>> $ x86_64-softmmu/qemu-system-x86_64 \
>> -qmp stdio \
>> -object throttle-group,id=tg0 \
>> -blockdev "{'driver':'throttle','node-name':'drive0',
>> 'throttle-group':'tg0','file':{'driver':'null-co'}}" \
>> -blockdev node-name=target,driver=null-co
>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 10, "major": 2},
>> "package": " (v2.9.0-632-g4a52d43-dirty)"}, "capabilities": []}}
>> {'execute':'qmp_capabilities'}
>> {"return": {}}
>> {'execute':'blockdev-mirror','arguments':{
>> 'device':'drive0','job-id':'job0','target':'target','sync':'full',
>> 'filter-node-name':'mirror-node' }}
>> qemu-system-x86_64: block/throttle.c:213: throttle_co_drain_end:
>> Assertion `tgm->io_limits_disabled' failed.
>> [1]3524 abort (core dumped) x86_64-softmmu/qemu-system-x86_64 -qmp
>> stdio -object throttle-group,id=tg0
>>
>> Here's what happens:
>>
>> (1) bdrv_drained_begin(bs) in mirror_start_job() starts draining drive0.
>>
>> (2) bdrv_append(...) puts mirror-node above drive0. Through
>> bdrv_replace_child_noperm(), this will invoke
>> bdrv_child_cb_drained_begin() on mirror-node. This is necessary because
>> drive0 is drained, so the new parent needs to be drained as well.
>> However, note that drive0 is not yet attached to mirror-node.
>> Therefore, mirror-node cannot drain drive0 recursively.
>
> Important context: We're talking about bdrv_set_backing_hd() here.
>
> It's also not quite correct to say that drive0 is not yet attached, but
> we're in a weird half-attached state. The BdrvChild is already
> initialised and in the parent list of drive0, but it's not yet assigned
> to mirror_node->backing nor in mirror_node's child list.
>
> For this specific case it looks like this is indeed the same as not
> being attached, but I wouldn't be surprised if we saw stranger effects
> at some point.
>
>> This is seemingly fine because drive0 is drained anyway. However, this
>> is different from what would happen if we would have drained drive0 with
>> mirror-node already attached to it as its parent: Then, we would have
>> drained drive0 twice; once by itself, and another time recursively
>> through mirror-node.
>>
>> This will be important in a second...
>>
>> (3) ...and this second is now: We invoke bdrv_drained_end() on drive0.
>> Now, through bdrv_parent_drained_end() and bdrv_child_cb_drained_end()
>> that goes up to mirror-node which recursively un-drains drive0. Fine so
>> far. But once that parent un-drain is done, we un-drain drive0 by
>> itself: And this fails the assertion in the throttle driver because we
>> attempt to un-drain it twice, although we've drained it only once.
>>
>>
>> So the issue has two parts:
>>
>> (A) (Un-)Draining a parent from a child will always (?[1]) (un-)drain
>> that child, too. This seems a bit superfluous to me and I would guess
>> that it results in worst-case O(n^2) function calls to drain a block
>> graph consisting of n nodes.
>>
>> (B) In bdrv_replace_child_noperm() we try to drain the parent if the new
>> child is drained; specifically, we want it to be in a state as if it had
>> been a parent when the child was originally drained. However, we fail
>> at this because we drain the parent without the child attached, so we
>> don't drain the child twice. This bites us when we undrain everything.
>
> I think the issue is much simpler, even though it still has two parts.
> It's the old story of bdrv_drain mixing two separate concepts:
>
> 1. Wait synchronously for the completion of all my requests to this
>node. This needs to be propagated down the graph to the children.
So, flush without flushing protocol drivers? :-)
> 2. Make sure that nobody else sends new requests to this node. This
>needs to be propagated up the graph to the parents.
>
> Some callers want only 1. (usually callers of bdrv_drain_all() without a
> begin/end pair), some callers want both 1. and 2. (that's the begin/end
> construction for getting exclusive access). Not sure if there are
> callers that want only 2., but possibly.
>
> If we actually take this separation serious, the first step to a fix
> could be that BdrvChildRole.drained_begin shouldn't be propagated to the
> children.
That seems good to me, but considering that the default was to just
propagate it to every child, I thought that I was missing something.
> We may still need to drain the requests at the node itself:
> > Imagine a drained backing file of qcow2 node. Making sure that the qcow2
> node doesn't get new requests isn't eno
Re: [Qemu-block] [Qemu-devel] Drainage in bdrv_replace_child_noperm()
On 2017-11-07 06:21, Fam Zheng wrote:
> On Mon, 11/06 19:49, Max Reitz wrote:
[...]
>> I have two ideas:
>>
>> One is to assume that (un-)draining a parent will always (un-)drain all
>> children, including the one the (un-)drain comes from. This assumption
>> seems wrong, see [1], but maybe it isn't. Anyway, if so, we could just
>> explicitly drain the new child in bdrv_replace_child_noperm() after
>> having drained the parent and thus get a consistent state again.
>>
>> The other is to declare (A) wrong. Maybe when
>> BdrvChildRole.drained_{begin,end}() is invoked, we should not drain that
>> child because we can declare it the caller's responsibility to make sure
>> it's drained. This seems logical to me because usually those methods
>> are invoked when the child is drained anyway. But maybe I'm wrong. :-)
>
> I'm in favor of asking the caller to make sure all nodes involved in the graph
> manupulation are drained, it feels comparably easier to do, than fixing the
> problem in bdrv_append().
I guess my main question was whether any of the two approaches is
evidently wrong in some way, but if you don't say that (and Kevin says
the first is wrong), I'm going to assume doing the second is OK. :-)
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH v3] util/async: use atomic_mb_set in qemu_bh_cancel
On Wed, Nov 08, 2017 at 05:32:23PM +0300, Pavel Butsykin wrote: > On 08.11.2017 17:24, Sergio Lopez wrote: > > On Wed, Nov 8, 2017 at 3:15 PM, Paolo Bonzini wrote: > > > On 08/11/2017 15:10, Sergio Lopez wrote: > > > > > I'm not quite sure that the pre-fetched is involved in this issue, > > > > > because pre-fetch reading a certain addresses should be invalidated by > > > > > write on another core to the same addresses. In our case write > > > > > req->state = THREAD_DONE should invalidate read req->state == > > > > > THREAD_DONE. > > > > > I am inclined to think that there is a memory-reordering read with > > > > > write. It's a very real case for x86 and I don't see the reasons which > > > > > can prevent it: > > > > > > > > > Yes, you're right. This is actually a memory reordering issue. I'm > > > > going to rewrite that paragraph. > > > > > > Well, memory reordering _is_ caused by speculative prefetching, delayed > > > cache invalidation (store buffers), and so on. > > > > > > But it's probably better indeed to replace "pre-fetched" with > > > "outdated". Whoever commits the patch can do the substitution (I can > > > too). > > > > > > > Alternatively, if we want to explicitly mention the memory barrier, we > > can replace the third paragraph with something like this: > > > > > > This was considered to be safe, as the completion function restarts the > > loop just after the call to qemu_bh_cancel. But, as this loop lacks a HW > > memory barrier, the read of req->state may actually happen _before_ the > > call, seeing it still as THREAD_QUEUED, and ending the completion > > function without having processed a pending TPE linked at pool->head: > > > > Yes, that's better. Thank you. I have updated the commit description and sent an updated pull request for QEMU 2.11-rc1. Stefan signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH v6 16/25] block: Add 'base-directory' BDS option
On 2017-11-08 18:09, Kevin Wolf wrote: > Am 07.11.2017 um 13:07 hat Alberto Garcia geschrieben: >> On Thu 02 Nov 2017 11:06:28 PM CET, Eric Blake wrote: >> >> Using this option, one can directly override what bdrv_dirname() >> will return. This is useful if one uses e.g. qcow2 on top of quorum >> (with only protocol BDSs under the quorum BDS) and wants to be able >> to use relative backing filenames. >> >> Signed-off-by: Max Reitz > > Who would be using this option in practice? I understand that > management tools should be using always absolute filenames, and this > API doesn't seem so convenient for humans. Hmmm. I seem to recall Eric liked it a couple of years ago when he was still more on the libvirt side of things. :-) >>> >>> Ideally, libvirt should be using node names rather than filenames >>> (whether absolute or relative); but I still think it could be >>> something that turns out to be useful. >> >> But what would be the use case? For a management tool it's not more >> convenient to use relative file names, or is it? > > If we add an external API, we must maintain compatibility in future > versions, so taking the patch comes with a cost. So I agree with you > that we better wait for an actual use case rather than just "could turn > out to be useful to someone maybe". OK, fair enough. Also, adding it later would hopefully be as simple as just sending this patch again. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v6 18/25] block: Add sgfnt_runtime_opts to BlockDriver
On 2017-11-08 18:14, Kevin Wolf wrote: > Am 02.11.2017 um 17:18 hat Max Reitz geschrieben: >> On 2017-11-02 17:11, Alberto Garcia wrote: >>> On Fri 29 Sep 2017 06:53:40 PM CEST, Max Reitz wrote: QLIST_ENTRY(BlockDriver) list; + +/* Pointer to a NULL-terminated array of names of significant options that + * can be specified for bdrv_open(). A significant option is one that + * changes the data of a BDS. + * If this pointer is NULL, the array is considered empty. + * "filename" and "driver" are always considered significant. */ +const char *const *sgfnt_runtime_opts; }; >>> >>> Is sgfnt a common acronym? I actually had to look it up... >> >> I'm open for other suggestions to shorten "significant". :-) >> >> (Actually, I can't remember where I got it from. I think I just made it >> up, but then I don't know why I didn't use sgfcnt -- maybe because cnt >> looks either like count (or like something else, but that's a different >> matter)...?) > > Because in reality you were abbreviating "signifant"? ;-) I actually thought the very same, yes. ;-) > Anyway, I found the inline initialisation as part of BlockDriver rather > ugly. The places where you used a separate static const array look > much nicer. Maybe use that consistently in all places? Sure. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v6 19/25] block: Add BlockDriver.bdrv_gather_child_options
On 2017-11-08 18:18, Kevin Wolf wrote:
> Am 29.09.2017 um 18:53 hat Max Reitz geschrieben:
>> Some follow-up patches will rework the way bs->full_open_options is
>> refreshed in bdrv_refresh_filename(). The new implementation will remove
>> the need for the block drivers' bdrv_refresh_filename() implementations
>> to set bs->full_open_options; instead, it will be generic and use static
>> information from each block driver.
>>
>> However, by implementing bdrv_gather_child_options(), block drivers will
>> still be able to override the way the full_open_options of their
>> children are incorporated into their own.
>>
>> We need to implement this function for VMDK because we have to prevent
>> the generic implementation from gathering the options of all children:
>> It is not possible to specify options for the extents through the
>> runtime options.
>
> Sounds more like a bug than a feature.
Want to fix it? :-)
>> For quorum, the child names that would be used by the generic
>> implementation and the ones that we actually want to use differ. See
>> quorum_gather_child_options() for more information.
>
> :-/
>
> What was the conclusion of our discussion at KVM Forum again? I just
> remember that this caused problems in other contexts like dynamic
> reconfiguration, too.
I might have just winced a little.
The short conclusion was that it's all broken.
The long conclusion... Is that I might try to wrestle with quorum
before this series? :-/
Let's see... The conclusion was that the user needs to specify a unique
name for each quorum child. Then, this name would be equivalent to the
runtime name. Sounds good so far? Well, I might have just screamed a
little when I remembered another issue:
The generic implementation gathers the options like so:
{ /* node options */
"child0": { /* child0 options */ },
"child1": { /* child1 options */ },
... }
However, specifying child options like so doesn't work easily with QAPI
and quorum, for two reasons:
(1) Specifying arbitrary keys with specific values would need new QAPI
infrastructure. Who is going to implement this? :-)
(2) Quorum needs a fixed order for its children, at least for FIFO mode.
A JSON object does not have inherent ordering.
The simple way to fix this is to make the children list a list of
objects which contain the child name and the options:
{ /* quorum node options */
"children": [
{ "child-name": "child0",
"options": { /* child0 options */ } },
{ "child-name": "child1",
"options": { /* child1 options */ } },
... ] }
But this would still require bdrv_gather_child_options() to generate.
The other way would be to add the QAPI infrastructure to make the
generic thing work, and add a "children-order" list or something which
gives order to the children. Then the generic implementation should
work... As long as the quorum driver makes sure to update this list
whenever a child is added or removed.
Maybe the latter is actually the better choice? :-/
But it definitely means more work (because of the QAPI modifications)...
Max
>> Signed-off-by: Max Reitz
>> ---
>> include/block/block_int.h | 13 +
>> block/quorum.c| 30 ++
>> block/vmdk.c | 13 +
>> 3 files changed, 56 insertions(+)
>
> Kevin
>
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v6 19/25] block: Add BlockDriver.bdrv_gather_child_options
Am 29.09.2017 um 18:53 hat Max Reitz geschrieben: > Some follow-up patches will rework the way bs->full_open_options is > refreshed in bdrv_refresh_filename(). The new implementation will remove > the need for the block drivers' bdrv_refresh_filename() implementations > to set bs->full_open_options; instead, it will be generic and use static > information from each block driver. > > However, by implementing bdrv_gather_child_options(), block drivers will > still be able to override the way the full_open_options of their > children are incorporated into their own. > > We need to implement this function for VMDK because we have to prevent > the generic implementation from gathering the options of all children: > It is not possible to specify options for the extents through the > runtime options. Sounds more like a bug than a feature. > For quorum, the child names that would be used by the generic > implementation and the ones that we actually want to use differ. See > quorum_gather_child_options() for more information. :-/ What was the conclusion of our discussion at KVM Forum again? I just remember that this caused problems in other contexts like dynamic reconfiguration, too. > Signed-off-by: Max Reitz > --- > include/block/block_int.h | 13 + > block/quorum.c| 30 ++ > block/vmdk.c | 13 + > 3 files changed, 56 insertions(+) Kevin
Re: [Qemu-block] [PATCH v6 18/25] block: Add sgfnt_runtime_opts to BlockDriver
Am 02.11.2017 um 17:18 hat Max Reitz geschrieben: > On 2017-11-02 17:11, Alberto Garcia wrote: > > On Fri 29 Sep 2017 06:53:40 PM CEST, Max Reitz wrote: > >> QLIST_ENTRY(BlockDriver) list; > >> + > >> +/* Pointer to a NULL-terminated array of names of significant options > >> that > >> + * can be specified for bdrv_open(). A significant option is one that > >> + * changes the data of a BDS. > >> + * If this pointer is NULL, the array is considered empty. > >> + * "filename" and "driver" are always considered significant. */ > >> +const char *const *sgfnt_runtime_opts; > >> }; > > > > Is sgfnt a common acronym? I actually had to look it up... > > I'm open for other suggestions to shorten "significant". :-) > > (Actually, I can't remember where I got it from. I think I just made it > up, but then I don't know why I didn't use sgfcnt -- maybe because cnt > looks either like count (or like something else, but that's a different > matter)...?) Because in reality you were abbreviating "signifant"? ;-) Anyway, I found the inline initialisation as part of BlockDriver rather ugly. The places where you used a separate static const array look much nicer. Maybe use that consistently in all places? Kevin signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH v6 16/25] block: Add 'base-directory' BDS option
Am 07.11.2017 um 13:07 hat Alberto Garcia geschrieben: > On Thu 02 Nov 2017 11:06:28 PM CET, Eric Blake wrote: > > Using this option, one can directly override what bdrv_dirname() > will return. This is useful if one uses e.g. qcow2 on top of quorum > (with only protocol BDSs under the quorum BDS) and wants to be able > to use relative backing filenames. > > Signed-off-by: Max Reitz > >>> > >>> Who would be using this option in practice? I understand that > >>>management tools should be using always absolute filenames, and this > >>>API doesn't seem so convenient for humans. > >> > >> Hmmm. I seem to recall Eric liked it a couple of years ago when he > >> was still more on the libvirt side of things. :-) > > > > Ideally, libvirt should be using node names rather than filenames > > (whether absolute or relative); but I still think it could be > > something that turns out to be useful. > > But what would be the use case? For a management tool it's not more > convenient to use relative file names, or is it? If we add an external API, we must maintain compatibility in future versions, so taking the patch comes with a cost. So I agree with you that we better wait for an actual use case rather than just "could turn out to be useful to someone maybe". (Also, so that I don't have to post a second reply to a patch that might be dropped: "Since: 2.10" wouldn't be quite right any more either way.) Kevin
Re: [Qemu-block] [Qemu-devel] [PATCH v3] util/async: use atomic_mb_set in qemu_bh_cancel
On 08.11.2017 17:15, Paolo Bonzini wrote: On 08/11/2017 15:10, Sergio Lopez wrote: I'm not quite sure that the pre-fetched is involved in this issue, because pre-fetch reading a certain addresses should be invalidated by write on another core to the same addresses. In our case write req->state = THREAD_DONE should invalidate read req->state == THREAD_DONE. I am inclined to think that there is a memory-reordering read with write. It's a very real case for x86 and I don't see the reasons which can prevent it: Yes, you're right. This is actually a memory reordering issue. I'm going to rewrite that paragraph. Well, memory reordering _is_ caused by speculative prefetching, delayed cache invalidation (store buffers), and so on. what do you mean? If we are speaking about x86, then a write on another core (like req->state = THREAD_DONE in this issue) should invalidate prefetch read(req->state = THREAD_DONE) and this is prevented in hardware. The prefetch is locked to the L1, when another cpu invalidates the cache lines, the prefetch is invalidated also (As far as I understand it). But it's probably better indeed to replace "pre-fetched" with "outdated". Whoever commits the patch can do the substitution (I can too). Paolo
Re: [Qemu-block] segfault in parallel blockjobs (iotest 30)
On 8/11/2017 5:45 PM, Alberto Garcia wrote: On Tue 07 Nov 2017 05:19:41 PM CET, Anton Nefedov wrote: BlockBackend gets deleted by another job's stream_complete(), deferred to the main loop, so the fact that the job is put to sleep by bdrv_drain_all_begin() doesn't really stop it from execution. I was debugging this a bit, and the block_job_defer_to_main_loop() call happens _after_ all jobs have been paused, so I think that when the BDS is drained then stream_run() finishes the last iteration without checking if it's paused. Without your patch (i.e. with a smaller STREAM_BUFFER_SIZE) then I assume that the function would have to continue looping and block_job_sleep_ns() would make the job coroutine yield, effectively pausing the job and preventing the crash. I can fix the crash by adding block_job_pause_point(&s->common) at the end of stream_run() (where the 'out' label is). I'm thinking that perhaps we should add the pause point directly to block_job_defer_to_main_loop(), to prevent any block job from running the exit function when it's paused. Is it possible that the exit function is already deferred when the jobs are being paused? (even though it's at least less likely to happen) Then should we flush the bottom halves somehow in addition to putting the jobs to sleep? And also then it all probably has to happen before bdrv_reopen_queue() /Anton Somehow I had the impression that we discussed this already in the past (?) because I remember thinking about this very scenario. Berto
Re: [Qemu-block] [Qemu-devel] [PATCH v3 01/46] Replace all occurances of __FUNCTION__ with __func__
On Wed, Nov 8, 2017 at 7:00 AM, Eric Blake wrote: > On 11/08/2017 08:51 AM, Alistair Francis wrote: > > Let me rephrase the question: do we really support compilers that don't > understand __func__? The presence of numerous unconditional uses of > __func__ in the tree means the answer is no. Let's replace AUDIO_FUNC > by plain __func__. Answered elsewhere in patch 3/46 (where we DO replace AUDIO_FUNC by __func__). >>> >>> I see. >>> >>> Put 03/46 first, so we don't have to mess with AUDIO_FUNC twice? >> >> I would really like to avoid that, as the conflicts will be a bit of a >> mess. The way I see it there will be a lot of churn no matter what, >> so we don't gain much by swapping the order around. >> >> I have a new series ready to send today, so I'm going to send that >> through as I would like at least some of these patches to make it in >> 2.11. After that if you think strongly the order should be changed I >> can change it in the next version. > > I think the reorder is not that hard. Put 3/46 first (changing > AUDIO_FUNC to __func__), and then 1/46 doesn't have to touch any of the > files that used to use AUDIO_FUNC, because there is no intermediate > state using __FUNCTION__. > > If I'm reading it correctly, the rebase conflict is limited to a slight > rewording of the commit message for 3/46, and one line in 1/46 to the > definition of AUDIO_FUNC (that will no longer be present). That's true, it didn't end up being that hard. I'm launching my tests now, so I should be able to send the series out later today. Hopefully some of this can make it in 2.11 :) Thanks, Alistair > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org >
Re: [Qemu-block] [Qemu-devel] [PATCH v3 7/7] block/curl: code cleanup to comply with coding style
On 08/11/2017 15:50, Darren Kenny wrote: >> But, since checkpatch.pl doesn't flag it, and since it is easier to >> remove the leading and trailing /* and */ to enable the debug #defines >> (compared to editing every single line of the comment), I don't see a >> problem with the style chosen here. > > If that is the purpose, maybe an #if 0 is more appropriate or #ifdef > DEBUG, or similar. > > Isn't the purpose of styling to be consistent? As such should we not > be trying to use the multi-line style set out at the top of the file? Yes, and you're very welcome to submit a checkpatch.pl patch that warns about that comment style without * at the beginning of each line. On the other hand, style should not get in the way, and the version that gets least in the way for debug #defines is //. Paolo
Re: [Qemu-block] [PATCH v3 7/7] block/curl: code cleanup to comply with coding style
On 08/11/2017 11:47, Darren Kenny wrote: >> >> >> -// #define DEBUG_CURL >> -// #define DEBUG_VERBOSE >> +/* >> + #define DEBUG_CURL >> + #define DEBUG_VERBOSE >> +*/ > > Is it not more common to use the style: > > Â Â /* > Â Â Â * text > Â Â Â */ > > This is visible in almost every file at the top, where the Copyright > and License is. The most common style in QEMU is probably /* text * more text */ But for DEBUG_* macros I think // are appropriate. checkpatch should still warn about them because DEBUG_* macros should be replaced by tracepoints; but unless we do that we should keep line comments. >> >> -CURLState *s = ((CURLState*)opaque); >> +CURLState *s = opaque; > > Not sure about this one - while it may not be strictly necessary to > do the casting, from a readability point of view it is helpful to > see the cast - possibly with the outer brackets removed: > > CURLState *s = (CURLState*)opaque; I think the most common idiom here is to omit the cast. >> -// In case we have the requested data already (e.g. read-ahead), >> -// we can just call the callback and be done. >> +/* In case we have the requested data already (e.g. read-ahead), >> + we can just call the callback and be done. */ > > Please don't do a multi-line comment like this, it is much more > obvious that the second line is a comment if you use the more normal > format of as outlined earlier by me. Agreed. Paolo
Re: [Qemu-block] [Qemu-devel] [PATCH v3 01/46] Replace all occurances of __FUNCTION__ with __func__
On 11/08/2017 08:51 AM, Alistair Francis wrote: Let me rephrase the question: do we really support compilers that don't understand __func__? The presence of numerous unconditional uses of __func__ in the tree means the answer is no. Let's replace AUDIO_FUNC by plain __func__. >>> >>> Answered elsewhere in patch 3/46 (where we DO replace AUDIO_FUNC by >>> __func__). >> >> I see. >> >> Put 03/46 first, so we don't have to mess with AUDIO_FUNC twice? > > I would really like to avoid that, as the conflicts will be a bit of a > mess. The way I see it there will be a lot of churn no matter what, > so we don't gain much by swapping the order around. > > I have a new series ready to send today, so I'm going to send that > through as I would like at least some of these patches to make it in > 2.11. After that if you think strongly the order should be changed I > can change it in the next version. I think the reorder is not that hard. Put 3/46 first (changing AUDIO_FUNC to __func__), and then 1/46 doesn't have to touch any of the files that used to use AUDIO_FUNC, because there is no intermediate state using __FUNCTION__. If I'm reading it correctly, the rebase conflict is limited to a slight rewording of the commit message for 3/46, and one line in 1/46 to the definition of AUDIO_FUNC (that will no longer be present). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH v3 01/46] Replace all occurances of __FUNCTION__ with __func__
On Tue, Nov 7, 2017 at 11:52 PM, Markus Armbruster wrote: > Eric Blake writes: > >> On 11/07/2017 04:12 AM, Markus Armbruster wrote: >>> Juan Quintela writes: >>> Alistair Francis wrote: > Replace all occurs of __FUNCTION__ except for the check in checkpatch > with the non GCC specific __func__. > >> > +++ b/audio/audio_int.h > @@ -253,7 +253,7 @@ static inline int audio_ring_dist (int dst, int src, > int len) > #define AUDIO_STRINGIFY(n) AUDIO_STRINGIFY_(n) > > #if defined _MSC_VER || defined __GNUC__ > -#define AUDIO_FUNC __FUNCTION__ > +#define AUDIO_FUNC __func__ > #else > #define AUDIO_FUNC __FILE__ ":" AUDIO_STRINGIFY (__LINE__) > #endif Unrelated to this patch Do we really support other compilers than msc and gcc? >>> >>> Let me rephrase the question: do we really support compilers that don't >>> understand __func__? The presence of numerous unconditional uses of >>> __func__ in the tree means the answer is no. Let's replace AUDIO_FUNC >>> by plain __func__. >> >> Answered elsewhere in patch 3/46 (where we DO replace AUDIO_FUNC by >> __func__). > > I see. > > Put 03/46 first, so we don't have to mess with AUDIO_FUNC twice? I would really like to avoid that, as the conflicts will be a bit of a mess. The way I see it there will be a lot of churn no matter what, so we don't gain much by swapping the order around. I have a new series ready to send today, so I'm going to send that through as I would like at least some of these patches to make it in 2.11. After that if you think strongly the order should be changed I can change it in the next version. Thanks, Alistair
Re: [Qemu-block] [Qemu-devel] [PATCH v3 7/7] block/curl: code cleanup to comply with coding style
On Wed, Nov 08, 2017 at 08:26:57AM -0600, Eric Blake wrote:
On 11/08/2017 04:47 AM, Darren Kenny wrote:
Hi Jeff,
While I'm relatively new to this community, I do have some comments
about the styling in this file.
I don't see anything in the CODING_STYLE file that tells me I'm
wrong here, but it's certainly possible...
More inline.
-// #define DEBUG_CURL
-// #define DEBUG_VERBOSE
This is definitely against style (checkpatch.pl flags it),
+/*
+ #define DEBUG_CURL
+ #define DEBUG_VERBOSE
+*/
while you are correct that this is not quite usual style.
Is it not more common to use the style:
  /*
   * text
   */
But, since checkpatch.pl doesn't flag it, and since it is easier to
remove the leading and trailing /* and */ to enable the debug #defines
(compared to editing every single line of the comment), I don't see a
problem with the style chosen here.
If that is the purpose, maybe an #if 0 is more appropriate or #ifdef
DEBUG, or similar.
Isn't the purpose of styling to be consistent? As such should we not
be trying to use the multi-line style set out at the top of the file?
@@ -235,7 +236,7 @@ static size_t curl_header_cb(void *ptr, size_t
size, size_t nmemb, void *opaque)
/* Called from curl_multi_do_locked, with s->mutex held. */
static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void
*opaque)
{
-Â Â Â CURLState *s = ((CURLState*)opaque);
+Â Â Â CURLState *s = opaque;
Not sure about this one - while it may not be strictly necessary to
do the casting, from a readability point of view it is helpful to
see the cast - possibly with the outer brackets removed:
CURLState *s = (CURLState*)opaque;
I _am_ sure about it. The cast from void* is pointless (this is C, not
C++), and this is one of the changes that Jeff specifically made in v3
that was not present in v2, because I requested that we lose the cast
(in general, we try to avoid as many casts as possible).
Fair enough.
@@ -876,13 +880,13 @@ static void curl_setup_preadv(BlockDriverState
*bs, CURLAIOCB *acb)
   qemu_mutex_lock(&s->mutex);
-Â Â Â // In case we have the requested data already (e.g. read-ahead),
-Â Â Â // we can just call the callback and be done.
+Â Â Â /* In case we have the requested data already (e.g. read-ahead),
+Â Â we can just call the callback and be done. */
Please don't do a multi-line comment like this, it is much more
obvious that the second line is a comment if you use the more normal
format of as outlined earlier by me.
Indeed, while I don't mind the style used on the #define DEBUG_*, this
one could be touched up to be more idiomatic.
Thanks,
Darren.
Re: [Qemu-block] segfault in parallel blockjobs (iotest 30)
On Tue 07 Nov 2017 05:19:41 PM CET, Anton Nefedov wrote: > BlockBackend gets deleted by another job's stream_complete(), deferred > to the main loop, so the fact that the job is put to sleep by > bdrv_drain_all_begin() doesn't really stop it from execution. I was debugging this a bit, and the block_job_defer_to_main_loop() call happens _after_ all jobs have been paused, so I think that when the BDS is drained then stream_run() finishes the last iteration without checking if it's paused. Without your patch (i.e. with a smaller STREAM_BUFFER_SIZE) then I assume that the function would have to continue looping and block_job_sleep_ns() would make the job coroutine yield, effectively pausing the job and preventing the crash. I can fix the crash by adding block_job_pause_point(&s->common) at the end of stream_run() (where the 'out' label is). I'm thinking that perhaps we should add the pause point directly to block_job_defer_to_main_loop(), to prevent any block job from running the exit function when it's paused. Somehow I had the impression that we discussed this already in the past (?) because I remember thinking about this very scenario. Berto
Re: [Qemu-block] [PATCH] block: Deprecate bdrv_set_read_only() and users
On 11/08/2017 06:20 AM, Kevin Wolf wrote: >> Well, they don't only need an explicitly set option, but the important >> point is that they don't work with the default value. But I can add >> something to this effect. > > I'll squash this in if it looks good to you: > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index ab96e348e6..76bf50f813 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3134,8 +3134,11 @@ > # This option is required on the top level of blockdev-add. > # @discard: discard-related options (default: ignore) > # @cache: cache-related options > -# @read-only: whether the block device should be read-only > -# (default: false) > +# @read-only: whether the block device should be read-only (default: > false). > +# Note that some block drivers support only read-only access, > +# either generally or in certain configurations. In this > case, > +# the default value does not work and the option must be > +# specified explicitly. Yes, that looks reasonable, if we aren't interested in toying with the idea of a per-driver default instead. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH] block: Close a BlockDriverState completely even when bs->drv is NULL
Am 06.11.2017 um 15:53 hat Alberto Garcia geschrieben: > bdrv_close() skips much of its logic when bs->drv is NULL. This is > fine when we're closing a BlockDriverState that has just been created > (because e.g the initialization process failed), but it's not enough > in other cases. > > For example, when a valid qcow2 image is found to be corrupted then > QEMU marks it as such in the file header and then sets bs->drv to > NULL in order to make the BlockDriverState unusable. When that BDS is > later closed then many of its data structures are not freed (leaking > their memory) and none of its children are detached. This results in > bdrv_close_all() failing to close all BDSs and making this assertion > fail when QEMU is being shut down: > >bdrv_close_all: Assertion `QTAILQ_EMPTY(&all_bdrv_states)' failed. > > This patch makes bdrv_close() do the full uninitialization process > in all cases. This fixes the problem with corrupted images and still > works fine with freshly created BDSs. > > Signed-off-by: Alberto Garcia This doesn't apply cleanly in the test case. Does it depend on your qcow2 fixes? Also, while I think the change makes sense, it's also clear that we're trying to fix up an inconsistent state here. Maybe we could also improve the state that block drivers leave behind when marking an image as corrupt. Just setting bs->drv = NULL means that at least any internal data structures will not get cleaned up. On the other hand, we can't just call bdrv_close() from a failing request because closing requires that we drain the request first. Maybe it would be possible to call drv->bdrv_close() with a BH or something. Kevin
Re: [Qemu-block] [PATCH] block: Deprecate bdrv_set_read_only() and users
On 11/08/2017 04:04 AM, Kevin Wolf wrote:
>
> Well, they don't only need an explicitly set option, but the important
> point is that they don't work with the default value. But I can add
> something to this effect.
>
>>> +++ b/block/vvfat.c
>>> @@ -1259,7 +1259,11 @@ static int vvfat_open(BlockDriverState *bs, QDict
>>> *options, int flags,
>>> "Unable to set VVFAT to 'rw' when drive is
>>> read-only");
>>> goto fail;
>>> }
>>> -} else {
>>> +} else if (!bdrv_is_read_only(bs)) {
>>> +error_report("Opening non-rw vvfat images without an explicit "
>>> + "read-only=on option is deprecated. Future versions "
>>> + "will refuse to open the image instead of "
>>> + "automatically marking the image read-only.");
>>> /* read only is the default for safety */
>>> ret = bdrv_set_read_only(bs, true, &local_err);
>>
>> Is this also a good time to deprecate vvfat's duplication of rw vs.
>> read-only, and consolidate that into a single option? No other device
>> defaults to read-only, so the deprecation period is a good point to warn
>> that a future version may default to read-write without an explicit
>> read-only. I guess vvfat is the only driver with a device-specific QAPI
>> change (for 'rw') that might be impacted if you make that additional change.
>
> I would love to get rid of the duplication, but there's a reason why
> vvfat defaults to read-only. I think we're relatively confident that a
> read-only vvfat can be safely implemented (and hopefully is), but write
> support is really a clever hack that may or may not work reliably
> depending on how crazy the guest OS goes.
>
> So if we removed the 'rw' option, would we want 'read-only' to default
> to true for vvfat? I'm not sure if we want to go there, it would mean
> making the default value of some base BlockdevOptions depend on the
> driver.
>
> On the other hand, I'm not sure how useful 'read-only' even is apart
> from the protocol layer... Should it have been driver-specific? But it's
> too late for that anyway.
Having a driver-specific default for read-only MIGHT make sense, as a
plan for something down the road (it matches current behavior, after
all, in that some drivers force read-only as their default). I guess
now is the time to decide WHAT we want to do after the deprecation
period ends, so that we're only making an incompatible change once, and
tweak the deprecation (and resulting warning messages in the meantime)
to fit in with that plan.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH v3] util/async: use atomic_mb_set in qemu_bh_cancel
On 08.11.2017 17:24, Sergio Lopez wrote: On Wed, Nov 8, 2017 at 3:15 PM, Paolo Bonzini wrote: On 08/11/2017 15:10, Sergio Lopez wrote: I'm not quite sure that the pre-fetched is involved in this issue, because pre-fetch reading a certain addresses should be invalidated by write on another core to the same addresses. In our case write req->state = THREAD_DONE should invalidate read req->state == THREAD_DONE. I am inclined to think that there is a memory-reordering read with write. It's a very real case for x86 and I don't see the reasons which can prevent it: Yes, you're right. This is actually a memory reordering issue. I'm going to rewrite that paragraph. Well, memory reordering _is_ caused by speculative prefetching, delayed cache invalidation (store buffers), and so on. But it's probably better indeed to replace "pre-fetched" with "outdated". Whoever commits the patch can do the substitution (I can too). Alternatively, if we want to explicitly mention the memory barrier, we can replace the third paragraph with something like this: This was considered to be safe, as the completion function restarts the loop just after the call to qemu_bh_cancel. But, as this loop lacks a HW memory barrier, the read of req->state may actually happen _before_ the call, seeing it still as THREAD_QUEUED, and ending the completion function without having processed a pending TPE linked at pool->head: Yes, that's better. Thank you. --- Sergio
Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] qcow2: Check that corrupted images can be repaired in iotest 060
On 11/08/2017 06:13 AM, Alberto Garcia wrote: > We just fixed a few bugs that caused QEMU to crash when trying to > write to corrupted qcow2 images, and iotest 060 was expanded to test > all those scenarios. > > In almost all cases the corrupted images can be repaired using > qemu-img, so this patch verifies that. > > Signed-off-by: Alberto Garcia > --- > tests/qemu-iotests/060 | 10 > tests/qemu-iotests/060.out | 64 > ++ > 2 files changed, 74 insertions(+) Just in case it matters to patchew: Based-on: Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH v3 7/7] block/curl: code cleanup to comply with coding style
On 11/08/2017 04:47 AM, Darren Kenny wrote:
> Hi Jeff,
>
> While I'm relatively new to this community, I do have some comments
> about the styling in this file.
>
> I don't see anything in the CODING_STYLE file that tells me I'm
> wrong here, but it's certainly possible...
>
> More inline.
>
>> -// #define DEBUG_CURL
>> -// #define DEBUG_VERBOSE
This is definitely against style (checkpatch.pl flags it),
>> +/*
>> + #define DEBUG_CURL
>> + #define DEBUG_VERBOSE
>> +*/
>
while you are correct that this is not quite usual style.
> Is it not more common to use the style:
>
> Â Â /*
> Â Â Â * text
> Â Â Â */
But, since checkpatch.pl doesn't flag it, and since it is easier to
remove the leading and trailing /* and */ to enable the debug #defines
(compared to editing every single line of the comment), I don't see a
problem with the style chosen here.
>> @@ -235,7 +236,7 @@ static size_t curl_header_cb(void *ptr, size_t
>> size, size_t nmemb, void *opaque)
>> /* Called from curl_multi_do_locked, with s->mutex held. */
>> static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void
>> *opaque)
>> {
>> -Â Â Â CURLState *s = ((CURLState*)opaque);
>> +Â Â Â CURLState *s = opaque;
>
> Not sure about this one - while it may not be strictly necessary to
> do the casting, from a readability point of view it is helpful to
> see the cast - possibly with the outer brackets removed:
>
> CURLState *s = (CURLState*)opaque;
I _am_ sure about it. The cast from void* is pointless (this is C, not
C++), and this is one of the changes that Jeff specifically made in v3
that was not present in v2, because I requested that we lose the cast
(in general, we try to avoid as many casts as possible).
>> @@ -876,13 +880,13 @@ static void curl_setup_preadv(BlockDriverState
>> *bs, CURLAIOCB *acb)
>>
>> Â Â Â qemu_mutex_lock(&s->mutex);
>>
>> -Â Â Â // In case we have the requested data already (e.g. read-ahead),
>> -Â Â Â // we can just call the callback and be done.
>> +Â Â Â /* In case we have the requested data already (e.g. read-ahead),
>> +Â Â we can just call the callback and be done. */
>
> Please don't do a multi-line comment like this, it is much more
> obvious that the second line is a comment if you use the more normal
> format of as outlined earlier by me.
Indeed, while I don't mind the style used on the #define DEBUG_*, this
one could be touched up to be more idiomatic.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH v3] util/async: use atomic_mb_set in qemu_bh_cancel
On Wed, Nov 8, 2017 at 3:15 PM, Paolo Bonzini wrote: > On 08/11/2017 15:10, Sergio Lopez wrote: >>> I'm not quite sure that the pre-fetched is involved in this issue, >>> because pre-fetch reading a certain addresses should be invalidated by >>> write on another core to the same addresses. In our case write >>> req->state = THREAD_DONE should invalidate read req->state == THREAD_DONE. >>> I am inclined to think that there is a memory-reordering read with >>> write. It's a very real case for x86 and I don't see the reasons which >>> can prevent it: >>> >> Yes, you're right. This is actually a memory reordering issue. I'm >> going to rewrite that paragraph. > > Well, memory reordering _is_ caused by speculative prefetching, delayed > cache invalidation (store buffers), and so on. > > But it's probably better indeed to replace "pre-fetched" with > "outdated". Whoever commits the patch can do the substitution (I can too). > Alternatively, if we want to explicitly mention the memory barrier, we can replace the third paragraph with something like this: This was considered to be safe, as the completion function restarts the loop just after the call to qemu_bh_cancel. But, as this loop lacks a HW memory barrier, the read of req->state may actually happen _before_ the call, seeing it still as THREAD_QUEUED, and ending the completion function without having processed a pending TPE linked at pool->head: --- Sergio
Re: [Qemu-block] [Qemu-devel] [PATCH v3] util/async: use atomic_mb_set in qemu_bh_cancel
On 08/11/2017 15:10, Sergio Lopez wrote: >> I'm not quite sure that the pre-fetched is involved in this issue, >> because pre-fetch reading a certain addresses should be invalidated by >> write on another core to the same addresses. In our case write >> req->state = THREAD_DONE should invalidate read req->state == THREAD_DONE. >> I am inclined to think that there is a memory-reordering read with >> write. It's a very real case for x86 and I don't see the reasons which >> can prevent it: >> > Yes, you're right. This is actually a memory reordering issue. I'm > going to rewrite that paragraph. Well, memory reordering _is_ caused by speculative prefetching, delayed cache invalidation (store buffers), and so on. But it's probably better indeed to replace "pre-fetched" with "outdated". Whoever commits the patch can do the substitution (I can too). Paolo
Re: [Qemu-block] [Qemu-devel] [PATCH v3] util/async: use atomic_mb_set in qemu_bh_cancel
On Wed, Nov 8, 2017 at 2:50 PM, Pavel Butsykin wrote: > On 08.11.2017 09:34, Sergio Lopez wrote: >> This was considered to be safe, as the completion function restarts the >> loop just after the call to qemu_bh_cancel. But, under certain access >> patterns and scheduling conditions, the loop may wrongly use a >> pre-fetched elem->state value, reading it as THREAD_QUEUED, and ending >> the completion function without having processed a pending TPE linked at >> pool->head: > > > I'm not quite sure that the pre-fetched is involved in this issue, > because pre-fetch reading a certain addresses should be invalidated by > write on another core to the same addresses. In our case write > req->state = THREAD_DONE should invalidate read req->state == THREAD_DONE. > I am inclined to think that there is a memory-reordering read with > write. It's a very real case for x86 and I don't see the reasons which > can prevent it: > Yes, you're right. This is actually a memory reordering issue. I'm going to rewrite that paragraph. Thanks Pavel.
Re: [Qemu-block] segfault in parallel blockjobs (iotest 30)
On Tue 07 Nov 2017 05:19:41 PM CET, Anton Nefedov wrote: > One more drainage-related thing. > We have recently encountered an issue with parallel block jobs and it's > not quite clear how to fix it properly, so would appreciate your ideas. > > The small attached tweak makes iotest 30 (-qcow2 -nocache) fail 10/10 > times on my machine. I just wanted to quickly confirm that I can reproduce this in my system too, I just checked QEMU v2.8 (when this was introduced) and it was affected as well. Somehow we didn't detect this back in the day :( I'll take a look. Berto
Re: [Qemu-block] [Qemu-devel] [PATCH v3] util/async: use atomic_mb_set in qemu_bh_cancel
On 08.11.2017 09:34, Sergio Lopez wrote: Commit b7a745d added a qemu_bh_cancel call to the completion function as an optimization to prevent it from unnecessarily rescheduling itself. This completion function is scheduled from worker_thread, after setting the state of a ThreadPoolElement to THREAD_DONE. Great! We are seeing the same problem, and I was describing my fix, when I came across your patch :) This was considered to be safe, as the completion function restarts the loop just after the call to qemu_bh_cancel. But, under certain access patterns and scheduling conditions, the loop may wrongly use a pre-fetched elem->state value, reading it as THREAD_QUEUED, and ending the completion function without having processed a pending TPE linked at pool->head: I'm not quite sure that the pre-fetched is involved in this issue, because pre-fetch reading a certain addresses should be invalidated by write on another core to the same addresses. In our case write req->state = THREAD_DONE should invalidate read req->state == THREAD_DONE. I am inclined to think that there is a memory-reordering read with write. It's a very real case for x86 and I don't see the reasons which can prevent it: .text:0060E21E loc_60E21E: ; CODE XREF: .text:0060E2F4j .text:0060E21E mov rbx, [r12+98h] .text:0060E226 testrbx, rbx .text:0060E229 jnz short loc_60E238 .text:0060E22B jmp short exit_0 .text:0060E22B ; --- .text:0060E22D align 10h .text:0060E21E loc_60E21E: ; CODE XREF: .text:0060E2F4j .text:0060E21E mov rbx, [r12+98h] .text:0060E226 testrbx, rbx .text:0060E229 jnz short loc_60E238 .text:0060E22B jmp short exit_0 .text:0060E230 loc_60E230: ; CODE XREF: .text:0060E240j .text:0060E230 testrbp, rbp .text:0060E233 jz short exit_0 .text:0060E235 .text:0060E235 loc_60E235: ; CODE XREF: .text:0060E289j .text:0060E235 mov rbx, rbp .text:0060E238 .text:0060E238 loc_60E238: ; CODE XREF: .text:0060E229j .text:0060E238 cmp [rbx+ThreadPoolElement.state], 2 ; THREAD_DONE .text:0060E23C mov rbp, [rbx+ThreadPoolElement.all.link_next] .text:0060E240 jnz short loc_60E230 .text:0060E242 mov r15d, [rbx+ThreadPoolElement.ret] .text:0060E246 mov r13, [rbx+ThreadPoolElement.common.opaque] .text:0060E24A nop .text:0060E24B lea rax, trace_events_enabled_count .text:0060E252 mov eax, [rax] .text:0060E254 testeax, eax .text:0060E256 mov rax, rbp .text:0060E259 jnz loc_60E2F9 ... .text:0060E2BC loc_60E2BC: ; CODE XREF: .text:0060E27Cj .text:0060E2BC mov rdi, [r12+8] .text:0060E2C1 callqemu_bh_schedule .text:0060E2C6 mov rdi, [r12] .text:0060E2CA callaio_context_release .text:0060E2CF mov esi, [rbx+44h] .text:0060E2D2 mov rdi, [rbx+18h] .text:0060E2D6 callqword ptr [rbx+10h] .text:0060E2D9 mov rdi, [r12] .text:0060E2DD callaio_context_acquire .text:0060E2E2 mov rdi, [r12+8] .text:0060E2E7 callqemu_bh_cancel .text:0060E2EC mov rdi, rbx .text:0060E2EF callqemu_aio_unref .text:0060E2F4 jmp loc_60E21E The read (req->state == THREAD_DONE) can be reordered with qemu_bh_cancel(p->completion_bh) and then we get the same picture: worker thread |I/O thread | reordered read req->state req->state = THREAD_DONE; | qemu_bh_schedule(p->completion_bh) | bh->scheduled = 1; | | qemu_bh_cancel(p->completion_bh) | bh->scheduled = 0; | if (req->state == THREAD_DONE)
Re: [Qemu-block] [PATCH v6 19/25] block: Add BlockDriver.bdrv_gather_child_options
On Fri 29 Sep 2017 06:53:41 PM CEST, Max Reitz wrote: > Some follow-up patches will rework the way bs->full_open_options is > refreshed in bdrv_refresh_filename(). The new implementation will remove > the need for the block drivers' bdrv_refresh_filename() implementations > to set bs->full_open_options; instead, it will be generic and use static > information from each block driver. > > However, by implementing bdrv_gather_child_options(), block drivers will > still be able to override the way the full_open_options of their > children are incorporated into their own. > > We need to implement this function for VMDK because we have to prevent > the generic implementation from gathering the options of all children: > It is not possible to specify options for the extents through the > runtime options. > > For quorum, the child names that would be used by the generic > implementation and the ones that we actually want to use differ. See > quorum_gather_child_options() for more information. > > Signed-off-by: Max Reitz Reviewed-by: Alberto Garcia Berto
Re: [Qemu-block] [PATCH v6 18/25] block: Add sgfnt_runtime_opts to BlockDriver
On Thu 02 Nov 2017 05:18:31 PM CET, Max Reitz wrote: > On 2017-11-02 17:11, Alberto Garcia wrote: >> On Fri 29 Sep 2017 06:53:40 PM CEST, Max Reitz wrote: >>> QLIST_ENTRY(BlockDriver) list; >>> + >>> +/* Pointer to a NULL-terminated array of names of significant options >>> that >>> + * can be specified for bdrv_open(). A significant option is one that >>> + * changes the data of a BDS. >>> + * If this pointer is NULL, the array is considered empty. >>> + * "filename" and "driver" are always considered significant. */ >>> +const char *const *sgfnt_runtime_opts; >>> }; >> >> Is sgfnt a common acronym? I actually had to look it up... > > I'm open for other suggestions to shorten "significant". :-) Nah, it's ok :) Reviewed-by: Alberto Garcia Berto
Re: [Qemu-block] [Qemu-devel] [PATCH] block: Deprecate bdrv_set_read_only() and users
On Wed, Nov 08, 2017 at 12:51:27PM +0100, Kevin Wolf wrote: > Am 08.11.2017 um 11:49 hat Daniel P. Berrange geschrieben: > > On Wed, Nov 08, 2017 at 11:44:01AM +0100, Paolo Bonzini wrote: > > > On 07/11/2017 18:39, Daniel P. Berrange wrote: > > > > On Tue, Nov 07, 2017 at 06:26:38PM +0100, Kevin Wolf wrote: > > > >> bdrv_set_read_only() is used by some block drivers to override the > > > >> read-only option given by the user. This is not how read-only images > > > >> generally work in QEMU: Instead of second guessing what the user really > > > >> meant (which currently includes making an image read-only even if the > > > >> user didn't only use the default, but explicitly said read-only=off), > > > >> we > > > >> should error out if we can't provide what the user requested. > > > >> > > > >> This adds deprecation warnings to all callers of bdrv_set_read_only() > > > >> so > > > >> that the behaviour can be corrected after the usual deprecation period. > > > > > > > > All deprecations should be listed in "Deprecated features" appendix > > > > in qemu-doc.texi. This probably fits in the 'system emulator command > > > > line arguments' section, even though its talking about the need for > > > > the user to add something extra, rather than deleting something they > > > > currently use. > > > > > > I am not sure this counts as deprecation, but it should go in the > > > release notes as "future incompatible changes", and that section > > > probably should go in qemu-doc.texi itself. > > > > Yeah, adding a "Incompatible changes" appendix to the qemu-doc.texi > > would be useful, listing the planned change, and when it is actually > > made. That way apps adding support for a feature have an indication > > of any incompatiblities they might need to care about. > > You mean a section containing future incompatible changes as well as > already implemented incompatible changes? > > What would we do with the existing "Deprecated features" section? Would > it become a subsection of "Incompatible changes"? Or would we just > rename it and the subsections would stay on the same level and get > "deprecated" added to their title? Or a completely different structure? Yes, we could rename "Deprecated features" to "Deprecations & incompatible changes", And then add the word "Deprecated" to the current @section headings, and add a separate @section for things which are simply warning about future incompatible changes which aren't strictly deprcations. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-block] [PATCH v3] util/async: use atomic_mb_set in qemu_bh_cancel
On Wed, Nov 08, 2017 at 07:34:47AM +0100, Sergio Lopez wrote: > Commit b7a745d added a qemu_bh_cancel call to the completion function > as an optimization to prevent it from unnecessarily rescheduling itself. > > This completion function is scheduled from worker_thread, after setting > the state of a ThreadPoolElement to THREAD_DONE. > > This was considered to be safe, as the completion function restarts the > loop just after the call to qemu_bh_cancel. But, under certain access > patterns and scheduling conditions, the loop may wrongly use a > pre-fetched elem->state value, reading it as THREAD_QUEUED, and ending > the completion function without having processed a pending TPE linked at > pool->head: > > worker thread |I/O thread > >| speculatively read req->state > req->state = THREAD_DONE; | > qemu_bh_schedule(p->completion_bh) | > bh->scheduled = 1; | >| qemu_bh_cancel(p->completion_bh) >| bh->scheduled = 0; >| if (req->state == THREAD_DONE) >| // sees THREAD_QUEUED > > The source of the misunderstanding was that qemu_bh_cancel is now being > used by the _consumer_ rather than the producer, and therefore now needs > to have acquire semantics just like e.g. aio_bh_poll. > > In some situations, if there are no other independent requests in the > same aio context that could eventually trigger the scheduling of the > completion function, the omitted TPE and all operations pending on it > will get stuck forever. > > Signed-off-by: Sergio Lopez > --- > util/async.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan signature.asc Description: PGP signature
Re: [Qemu-block] [Qemu-devel] [PATCH] util/async: use atomic_mb_set in qemu_bh_cancel
On Tue, Nov 07, 2017 at 04:09:37PM +0100, Sergio Lopez wrote: > Commit b7a745d added a qemu_bh_cancel call to the completion function > as an optimization to prevent it from unnecessarily rescheduling itself. > > This completion function is scheduled from worker_thread, after setting > the state of a ThreadPoolElement to THREAD_DONE. > > This was considered to be safe, as the completion function restarts the > loop just after the call to qemu_bh_cancel. But, under certain access > patterns and scheduling conditions, the loop may wrongly use a > pre-fetched elem->state value, reading it as THREAD_QUEUED, and ending > the completion function without having processed a pending TPE linked at > pool->head. > > In some situations, if there are no other independent requests in the > same aio context that could eventually trigger the scheduling of the > completion function, the omitted TPE and all operations pending on it > will get stuck forever. > > Signed-off-by: Sergio Lopez > --- > util/async.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH] block: Deprecate bdrv_set_read_only() and users
Am 08.11.2017 um 11:04 hat Kevin Wolf geschrieben: > Am 07.11.2017 um 21:29 hat Eric Blake geschrieben: > > On 11/07/2017 11:26 AM, Kevin Wolf wrote: > > > bdrv_set_read_only() is used by some block drivers to override the > > > read-only option given by the user. This is not how read-only images > > > generally work in QEMU: Instead of second guessing what the user really > > > meant (which currently includes making an image read-only even if the > > > user didn't only use the default, but explicitly said read-only=off), we > > > should error out if we can't provide what the user requested. > > > > > > This adds deprecation warnings to all callers of bdrv_set_read_only() so > > > that the behaviour can be corrected after the usual deprecation period. > > > > > > Signed-off-by: Kevin Wolf > > > --- > > > block.c | 5 + > > > block/bochs.c | 13 ++--- > > > block/cloop.c | 13 ++--- > > > block/dmg.c | 12 +--- > > > block/rbd.c | 14 ++ > > > block/vvfat.c | 6 +- > > > 6 files changed, 49 insertions(+), 14 deletions(-) > > > > Dan pointed out the missing documentation, but for the code itself, the > > approach looks sane (especially since it was my attempt to make it worse > > by extending the idiom to NBD that triggered you to write this patch). > > > > Other documentation: In qapi/block-core.json, @BlockdevOptions, we > > probably ought to mention under @read-only that some block drivers > > require the use of an explicit read-only. > > Well, they don't only need an explicitly set option, but the important > point is that they don't work with the default value. But I can add > something to this effect. I'll squash this in if it looks good to you: diff --git a/qapi/block-core.json b/qapi/block-core.json index ab96e348e6..76bf50f813 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3134,8 +3134,11 @@ # This option is required on the top level of blockdev-add. # @discard: discard-related options (default: ignore) # @cache: cache-related options -# @read-only: whether the block device should be read-only -# (default: false) +# @read-only: whether the block device should be read-only (default: false). +# Note that some block drivers support only read-only access, +# either generally or in certain configurations. In this case, +# the default value does not work and the option must be +# specified explicitly. # @detect-zeroes: detect and optimize zero writes (Since 2.1) # (default: off) # @force-share: force share all permission on added nodes. signature.asc Description: PGP signature
Re: [Qemu-block] [Qemu-devel] [PATCH] block: Deprecate bdrv_set_read_only() and users
Am 08.11.2017 um 13:00 hat Paolo Bonzini geschrieben: > On 08/11/2017 12:51, Kevin Wolf wrote: > > Am 08.11.2017 um 11:49 hat Daniel P. Berrange geschrieben: > >> On Wed, Nov 08, 2017 at 11:44:01AM +0100, Paolo Bonzini wrote: > >>> I am not sure this counts as deprecation, but it should go in the > >>> release notes as "future incompatible changes", and that section > >>> probably should go in qemu-doc.texi itself. > >> > >> Yeah, adding a "Incompatible changes" appendix to the qemu-doc.texi > >> would be useful, listing the planned change, and when it is actually > >> made. That way apps adding support for a feature have an indication > >> of any incompatiblities they might need to care about. > > > > You mean a section containing future incompatible changes as well as > > already implemented incompatible changes? > > > > What would we do with the existing "Deprecated features" section? Would > > it become a subsection of "Incompatible changes"? Or would we just > > rename it and the subsections would stay on the same level and get > > "deprecated" added to their title? Or a completely different structure? > > > > I'm okay with adding a little documentation in this patch if I know what > > it should look like, but if it turns into a major overhaul of the > > documentation on incompatible changes, it's probably out of scope for > > this patch. > > For now I would just add a section to the changelog. That ensures that > we don't forget and end up doing nothing. Okay, done. Thanks! Kevin
[Qemu-block] [PATCH 0/1] qcow2: Check that corrupted images can be repaired in iotest 060
Hi, I sent the 'Misc qcow2 corruption checks' series the other day, and Kevin suggested that we check that the corrupted images can be repaired using qemu-img. This patch extends the tests that I wrote in order to do just that. Since the series is already in Max's branch I decided to write this as a follow-up patch, but if you prefer I can resend it instead. Regards, Berto Alberto Garcia (1): qcow2: Check that corrupted images can be repaired in iotest 060 tests/qemu-iotests/060 | 10 tests/qemu-iotests/060.out | 64 ++ 2 files changed, 74 insertions(+) -- 2.11.0
[Qemu-block] [PATCH 1/1] qcow2: Check that corrupted images can be repaired in iotest 060
We just fixed a few bugs that caused QEMU to crash when trying to write to corrupted qcow2 images, and iotest 060 was expanded to test all those scenarios. In almost all cases the corrupted images can be repaired using qemu-img, so this patch verifies that. Signed-off-by: Alberto Garcia --- tests/qemu-iotests/060 | 10 tests/qemu-iotests/060.out | 64 ++ 2 files changed, 74 insertions(+) diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060 index 66a8fa4aea..fae08b03bf 100755 --- a/tests/qemu-iotests/060 +++ b/tests/qemu-iotests/060 @@ -248,6 +248,8 @@ echo _make_test_img 64M poke_file "$TEST_IMG" "$rt_offset""\x00\x00\x00\x00\x00\x00\x00\x00" $QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io +# Repair the image +_check_test_img -r all echo echo "=== Testing empty refcount table with valid L1 and L2 tables ===" @@ -259,6 +261,8 @@ poke_file "$TEST_IMG" "$rt_offset" "\x00\x00\x00\x00\x00\x00\x00\x00" # allocation with an explicit offset (using qcow2_alloc_clusters_at()) # causing a refcount block to be allocated at offset 0 $QEMU_IO -c "write 0 128k" "$TEST_IMG" | _filter_qemu_io +# Repair the image +_check_test_img -r all echo echo "=== Testing empty refcount block ===" @@ -266,6 +270,8 @@ echo _make_test_img 64M poke_file "$TEST_IMG" "$rb_offset""\x00\x00\x00\x00\x00\x00\x00\x00" $QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io +# Repair the image +_check_test_img -r all echo echo "=== Testing empty refcount block with compressed write ===" @@ -276,6 +282,8 @@ poke_file "$TEST_IMG" "$rb_offset" "\x00\x00\x00\x00\x00\x00\x00\x00" # The previous write already allocated an L2 table, so now this new # write will try to allocate a compressed data cluster at offset 0. $QEMU_IO -c "write -c 0k 64k" "$TEST_IMG" | _filter_qemu_io +# Repair the image +_check_test_img -r all echo echo "=== Testing zero refcount table size ===" @@ -283,6 +291,8 @@ echo _make_test_img 64M poke_file "$TEST_IMG" "56""\x00\x00\x00\x00" $QEMU_IO -c "write 0 64k" "$TEST_IMG" 2>&1 | _filter_testdir | _filter_imgfmt +# Repair the image +_check_test_img -r all echo echo "=== Testing incorrect refcount table offset ===" diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out index cfd78f87a9..62c22701b8 100644 --- a/tests/qemu-iotests/060.out +++ b/tests/qemu-iotests/060.out @@ -187,6 +187,18 @@ read failed: Input/output error Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 qcow2: Marking image as corrupt: Preventing invalid write on metadata (overlaps with refcount table); further corruption events will be suppressed write failed: Input/output error +ERROR cluster 0 refcount=0 reference=1 +ERROR cluster 1 refcount=0 reference=1 +ERROR cluster 3 refcount=0 reference=1 +Rebuilding refcount structure +Repairing cluster 1 refcount=1 reference=0 +The following inconsistencies were found and repaired: + +0 leaked clusters +3 corruptions + +Double checking the fixed image now... +No errors were found on the image. === Testing empty refcount table with valid L1 and L2 tables === @@ -195,12 +207,40 @@ wrote 65536/65536 bytes at offset 0 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) qcow2: Marking image as corrupt: Preventing invalid allocation of refcount block at offset 0; further corruption events will be suppressed write failed: Input/output error +ERROR cluster 0 refcount=0 reference=1 +ERROR cluster 1 refcount=0 reference=1 +ERROR cluster 3 refcount=0 reference=1 +ERROR cluster 4 refcount=0 reference=1 +ERROR cluster 5 refcount=0 reference=1 +Rebuilding refcount structure +Repairing cluster 1 refcount=1 reference=0 +The following inconsistencies were found and repaired: + +0 leaked clusters +5 corruptions + +Double checking the fixed image now... +No errors were found on the image. === Testing empty refcount block === Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 qcow2: Marking image as corrupt: Preventing invalid allocation of L2 table at offset 0; further corruption events will be suppressed write failed: Input/output error +ERROR cluster 0 refcount=0 reference=1 +ERROR cluster 1 refcount=0 reference=1 +ERROR cluster 2 refcount=0 reference=1 +ERROR cluster 3 refcount=0 reference=1 +Rebuilding refcount structure +Repairing cluster 1 refcount=1 reference=0 +Repairing cluster 2 refcount=1 reference=0 +The following inconsistencies were found and repaired: + +0 leaked clusters +4 corruptions + +Double checking the fixed image now... +No errors were found on the image. === Testing empty refcount block with compressed write === @@ -209,11 +249,35 @@ wrote 65536/65536 bytes at offset 65536 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) qcow2: Marking image as corrupt: Preventing invalid allocation of compressed cluster at offset 0; further corruption events will be suppressed write fa
Re: [Qemu-block] [Qemu-devel] [PATCH] block: Deprecate bdrv_set_read_only() and users
On 08/11/2017 12:51, Kevin Wolf wrote: > Am 08.11.2017 um 11:49 hat Daniel P. Berrange geschrieben: >> On Wed, Nov 08, 2017 at 11:44:01AM +0100, Paolo Bonzini wrote: >>> I am not sure this counts as deprecation, but it should go in the >>> release notes as "future incompatible changes", and that section >>> probably should go in qemu-doc.texi itself. >> >> Yeah, adding a "Incompatible changes" appendix to the qemu-doc.texi >> would be useful, listing the planned change, and when it is actually >> made. That way apps adding support for a feature have an indication >> of any incompatiblities they might need to care about. > > You mean a section containing future incompatible changes as well as > already implemented incompatible changes? > > What would we do with the existing "Deprecated features" section? Would > it become a subsection of "Incompatible changes"? Or would we just > rename it and the subsections would stay on the same level and get > "deprecated" added to their title? Or a completely different structure? > > I'm okay with adding a little documentation in this patch if I know what > it should look like, but if it turns into a major overhaul of the > documentation on incompatible changes, it's probably out of scope for > this patch. For now I would just add a section to the changelog. That ensures that we don't forget and end up doing nothing. Paolo
Re: [Qemu-block] [Qemu-devel] [PATCH] block: Deprecate bdrv_set_read_only() and users
Am 08.11.2017 um 11:49 hat Daniel P. Berrange geschrieben: > On Wed, Nov 08, 2017 at 11:44:01AM +0100, Paolo Bonzini wrote: > > On 07/11/2017 18:39, Daniel P. Berrange wrote: > > > On Tue, Nov 07, 2017 at 06:26:38PM +0100, Kevin Wolf wrote: > > >> bdrv_set_read_only() is used by some block drivers to override the > > >> read-only option given by the user. This is not how read-only images > > >> generally work in QEMU: Instead of second guessing what the user really > > >> meant (which currently includes making an image read-only even if the > > >> user didn't only use the default, but explicitly said read-only=off), we > > >> should error out if we can't provide what the user requested. > > >> > > >> This adds deprecation warnings to all callers of bdrv_set_read_only() so > > >> that the behaviour can be corrected after the usual deprecation period. > > > > > > All deprecations should be listed in "Deprecated features" appendix > > > in qemu-doc.texi. This probably fits in the 'system emulator command > > > line arguments' section, even though its talking about the need for > > > the user to add something extra, rather than deleting something they > > > currently use. > > > > I am not sure this counts as deprecation, but it should go in the > > release notes as "future incompatible changes", and that section > > probably should go in qemu-doc.texi itself. > > Yeah, adding a "Incompatible changes" appendix to the qemu-doc.texi > would be useful, listing the planned change, and when it is actually > made. That way apps adding support for a feature have an indication > of any incompatiblities they might need to care about. You mean a section containing future incompatible changes as well as already implemented incompatible changes? What would we do with the existing "Deprecated features" section? Would it become a subsection of "Incompatible changes"? Or would we just rename it and the subsections would stay on the same level and get "deprecated" added to their title? Or a completely different structure? I'm okay with adding a little documentation in this patch if I know what it should look like, but if it turns into a major overhaul of the documentation on incompatible changes, it's probably out of scope for this patch. Kevin
Re: [Qemu-block] [PATCH v3 7/7] block/curl: code cleanup to comply with coding style
On Tue, Nov 07, 2017 at 05:27:24PM -0500, Jeff Cody wrote:
> This addresses non-functional changes to help curl.c better comply
> with the coding styles (comments, indentation, brackets, etc.).
>
> One minor code change is the combination of two if statements into
> a single if statement.
>
> Signed-off-by: Jeff Cody
> Reviewed-by: Eric Blake
As you say, just simple coding style fixes except for the single
combined if statement. Therefore:
Reviewed-by: Richard W.M. Jones
Rich.
> block/curl.c | 100
> +++
> 1 file changed, 52 insertions(+), 48 deletions(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index 35cf417..7a6dd44 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -32,8 +32,10 @@
> #include
> #include "qemu/cutils.h"
>
> -// #define DEBUG_CURL
> -// #define DEBUG_VERBOSE
> +/*
> + #define DEBUG_CURL
> + #define DEBUG_VERBOSE
> +*/
>
> #ifdef DEBUG_CURL
> #define DEBUG_CURL_PRINT 1
> @@ -76,15 +78,15 @@ static CURLMcode __curl_multi_socket_action(CURLM
> *multi_handle,
> #define CURL_TIMEOUT_DEFAULT 5
> #define CURL_TIMEOUT_MAX 1
>
> -#define CURL_BLOCK_OPT_URL "url"
> -#define CURL_BLOCK_OPT_READAHEAD "readahead"
> -#define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
> -#define CURL_BLOCK_OPT_TIMEOUT "timeout"
> -#define CURL_BLOCK_OPT_COOKIE"cookie"
> -#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret"
> -#define CURL_BLOCK_OPT_USERNAME "username"
> -#define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret"
> -#define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"
> +#define CURL_BLOCK_OPT_URL "url"
> +#define CURL_BLOCK_OPT_READAHEAD "readahead"
> +#define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
> +#define CURL_BLOCK_OPT_TIMEOUT "timeout"
> +#define CURL_BLOCK_OPT_COOKIE"cookie"
> +#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret"
> +#define CURL_BLOCK_OPT_USERNAME "username"
> +#define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret"
> +#define CURL_BLOCK_OPT_PROXY_USERNAME"proxy-username"
> #define CURL_BLOCK_OPT_PROXY_PASSWORD_SECRET "proxy-password-secret"
>
> struct BDRVCURLState;
> @@ -110,8 +112,7 @@ typedef struct CURLSocket {
> QLIST_ENTRY(CURLSocket) next;
> } CURLSocket;
>
> -typedef struct CURLState
> -{
> +typedef struct CURLState {
> struct BDRVCURLState *s;
> CURLAIOCB *acb[CURL_NUM_ACB];
> CURL *curl;
> @@ -196,22 +197,22 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd,
> int action,
>
> DPRINTF("CURL (AIO): Sock action %d on fd %d\n", action, (int)fd);
> switch (action) {
> -case CURL_POLL_IN:
> -aio_set_fd_handler(s->aio_context, fd, false,
> - curl_multi_read, NULL, NULL, state);
> -break;
> -case CURL_POLL_OUT:
> -aio_set_fd_handler(s->aio_context, fd, false,
> - NULL, curl_multi_do, NULL, state);
> -break;
> -case CURL_POLL_INOUT:
> -aio_set_fd_handler(s->aio_context, fd, false,
> - curl_multi_read, curl_multi_do, NULL, state);
> -break;
> -case CURL_POLL_REMOVE:
> -aio_set_fd_handler(s->aio_context, fd, false,
> - NULL, NULL, NULL, NULL);
> -break;
> +case CURL_POLL_IN:
> +aio_set_fd_handler(s->aio_context, fd, false,
> + curl_multi_read, NULL, NULL, state);
> +break;
> +case CURL_POLL_OUT:
> +aio_set_fd_handler(s->aio_context, fd, false,
> + NULL, curl_multi_do, NULL, state);
> +break;
> +case CURL_POLL_INOUT:
> +aio_set_fd_handler(s->aio_context, fd, false,
> + curl_multi_read, curl_multi_do, NULL, state);
> +break;
> +case CURL_POLL_REMOVE:
> +aio_set_fd_handler(s->aio_context, fd, false,
> + NULL, NULL, NULL, NULL);
> +break;
> }
>
> return 0;
> @@ -235,7 +236,7 @@ static size_t curl_header_cb(void *ptr, size_t size,
> size_t nmemb, void *opaque)
> /* Called from curl_multi_do_locked, with s->mutex held. */
> static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void
> *opaque)
> {
> -CURLState *s = ((CURLState*)opaque);
> +CURLState *s = opaque;
> size_t realsize = size * nmemb;
> int i;
>
> @@ -253,11 +254,12 @@ static size_t curl_read_cb(void *ptr, size_t size,
> size_t nmemb, void *opaque)
> memcpy(s->orig_buf + s->buf_off, ptr, realsize);
> s->buf_off += realsize;
>
> -for(i=0; i +for (i = 0; i < CURL_NUM_ACB; i++) {
> CURLAIOCB *acb = s->acb[i];
>
> -if (!acb)
> +if (!acb) {
> continue;
> +}
>
> if ((s->buf_off >= acb->end)) {
> size_t request_length = acb
Re: [Qemu-block] [PATCH v3 6/7] block/curl: fix minor memory leaks
On Tue, Nov 07, 2017 at 05:27:23PM -0500, Jeff Cody wrote: > Signed-off-by: Jeff Cody > --- > block/curl.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/block/curl.c b/block/curl.c > index 00a9879..35cf417 100644 > --- a/block/curl.c > +++ b/block/curl.c > @@ -857,6 +857,9 @@ out_noclean: > qemu_mutex_destroy(&s->mutex); > g_free(s->cookie); > g_free(s->url); > +g_free(s->username); > +g_free(s->proxyusername); > +g_free(s->proxypassword); > qemu_opts_del(opts); > return -EINVAL; > } > @@ -955,6 +958,9 @@ static void curl_close(BlockDriverState *bs) > > g_free(s->cookie); > g_free(s->url); > +g_free(s->username); > +g_free(s->proxyusername); > +g_free(s->proxypassword); username & proxyusername are allocated with g_strdup and so should obviously be freed. proxypassword is returned by qcrypto_secret_lookup_as_utf8, and it's not clear to me if we should free that or not. However examining the code of qcrypto_secret_lookup it looks as if this string is allocated by g_new0, which would indicate that it should also be freed. Therefore: Reviewed-by: Richard W.M. Jones Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
Re: [Qemu-block] [PATCH] qcow2: don't permit changing encryption parameters
Am 03.11.2017 um 15:39 hat Daniel P. Berrange geschrieben: > Currently if trying to change encryption parameters on a qcow2 image, qemu-img > will abort. We already explicitly check for attempt to change encrypt.format > but missed other parameters like encrypt.key-secret. Rather than list each > parameter, just blacklist changing of all parameters with a 'encrypt.' prefix. > > Signed-off-by: Daniel P. Berrange Thanks, applied to the block branch. Kevin
Re: [Qemu-block] [PATCH v3 5/7] block/curl: check error return of curl_global_init()
On Tue, Nov 07, 2017 at 05:27:22PM -0500, Jeff Cody wrote:
> If curl_global_init() fails, per the documentation no other curl
> functions may be called, so make sure to check the return value.
>
> Also, some minor changes to the initialization latch variable 'inited':
>
> - Make it static in the file, for clarity
> - Change the name for clarity
> - Make it a bool
>
> Signed-off-by: Jeff Cody
> Reviewed-by: Eric Blake
This is just a simple evolution of the previous code, so:
Reviewed-by: Richard W.M. Jones
Rich.
> block/curl.c | 18 --
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index 2a244e2..00a9879 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -89,6 +89,8 @@ static CURLMcode __curl_multi_socket_action(CURLM
> *multi_handle,
>
> struct BDRVCURLState;
>
> +static bool libcurl_initialized;
> +
> typedef struct CURLAIOCB {
> Coroutine *co;
> QEMUIOVector *qiov;
> @@ -686,14 +688,23 @@ static int curl_open(BlockDriverState *bs, QDict
> *options, int flags,
> double d;
> const char *secretid;
> const char *protocol_delimiter;
> +int ret;
>
> -static int inited = 0;
>
> if (flags & BDRV_O_RDWR) {
> error_setg(errp, "curl block device does not support writes");
> return -EROFS;
> }
>
> +if (!libcurl_initialized) {
> +ret = curl_global_init(CURL_GLOBAL_ALL);
> +if (ret) {
> +error_setg(errp, "libcurl initialization failed with %d", ret);
> +return -EIO;
> +}
> +libcurl_initialized = true;
> +}
> +
> qemu_mutex_init(&s->mutex);
> opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> qemu_opts_absorb_qdict(opts, options, &local_err);
> @@ -772,11 +783,6 @@ static int curl_open(BlockDriverState *bs, QDict
> *options, int flags,
> }
> }
>
> -if (!inited) {
> -curl_global_init(CURL_GLOBAL_ALL);
> -inited = 1;
> -}
> -
> DPRINTF("CURL: Opening %s\n", file);
> QSIMPLEQ_INIT(&s->free_state_waitq);
> s->aio_context = bdrv_get_aio_context(bs);
> --
> 2.9.5
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
Re: [Qemu-block] [PATCH v3 1/7] block/ssh: don't call libssh2_init() in block_init()
On Tue, Nov 07, 2017 at 05:27:18PM -0500, Jeff Cody wrote:
> We don't need libssh2 failure to be fatal (we could just opt to not
> register the driver on failure). But, it is probably a good idea to
> avoid external library calls during the block_init(), and call the
> libssh2 global init function on the first usage, returning any errors.
>
> Signed-off-by: Jeff Cody
> ---
> block/ssh.c | 37 ++---
> 1 file changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/block/ssh.c b/block/ssh.c
> index b049a16..de81ec8 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -83,12 +83,28 @@ typedef struct BDRVSSHState {
> bool unsafe_flush_warning;
> } BDRVSSHState;
>
> -static void ssh_state_init(BDRVSSHState *s)
> +static bool ssh_libinit_called;
> +
> +static int ssh_state_init(BDRVSSHState *s, Error **errp)
> {
> +int ret;
> +
> +if (!ssh_libinit_called) {
> +ret = libssh2_init(0);
> +if (ret) {
> +error_setg(errp, "libssh2 initialization failed with %d", ret);
> +return ret;
> +}
> +ssh_libinit_called = true;
> +}
> +
> +
> memset(s, 0, sizeof *s);
> s->sock = -1;
> s->offset = -1;
> qemu_co_mutex_init(&s->lock);
> +
> +return 0;
Is this thread safe? Is there a case where multiple ssh URL opens
could race off against each other and we could end up calling
libssh2_init in parallel? (According to the libssh2_init
documentation, the function is not thread-safe so should not be called
in parallel on multiple threads)
Rich.
> }
>
> static void ssh_state_free(BDRVSSHState *s)
> @@ -773,7 +789,9 @@ static int ssh_file_open(BlockDriverState *bs, QDict
> *options, int bdrv_flags,
> int ret;
> int ssh_flags;
>
> -ssh_state_init(s);
> +if (ssh_state_init(s, errp)) {
> +return -EIO;
> +}
>
> ssh_flags = LIBSSH2_FXF_READ;
> if (bdrv_flags & BDRV_O_RDWR) {
> @@ -821,8 +839,13 @@ static int ssh_create(const char *filename, QemuOpts
> *opts, Error **errp)
> BDRVSSHState s;
> ssize_t r2;
> char c[1] = { '\0' };
> +Error *local_err = NULL;
>
> -ssh_state_init(&s);
> +ret = ssh_state_init(&s, &local_err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return ret;
> +}
>
> /* Get desired file size. */
> total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> @@ -1213,14 +1236,6 @@ static BlockDriver bdrv_ssh = {
>
> static void bdrv_ssh_init(void)
> {
> -int r;
> -
> -r = libssh2_init(0);
> -if (r != 0) {
> -fprintf(stderr, "libssh2 initialization failed, %d\n", r);
> -exit(EXIT_FAILURE);
> -}
> -
> bdrv_register(&bdrv_ssh);
> }
>
> --
> 2.9.5
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
Re: [Qemu-block] [PATCH v3 2/7] block/ssh: make compliant with coding guidelines
On Tue, Nov 07, 2017 at 05:27:19PM -0500, Jeff Cody wrote:
> Signed-off-by: Jeff Cody
> Reviewed-by: Eric Blake
This one is just simple coding style fixes, so:
Reviewed-by: Richard W.M. Jones
Rich.
> block/ssh.c | 32 ++--
> 1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/block/ssh.c b/block/ssh.c
> index de81ec8..39cacc1 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -241,7 +241,7 @@ static int parse_uri(const char *filename, QDict
> *options, Error **errp)
> goto err;
> }
>
> -if(uri->user && strcmp(uri->user, "") != 0) {
> +if (uri->user && strcmp(uri->user, "") != 0) {
> qdict_put_str(options, "user", uri->user);
> }
>
> @@ -268,7 +268,7 @@ static int parse_uri(const char *filename, QDict
> *options, Error **errp)
>
> err:
> if (uri) {
> - uri_free(uri);
> +uri_free(uri);
> }
> return -EINVAL;
> }
> @@ -342,7 +342,7 @@ static int check_host_key_knownhosts(BDRVSSHState *s,
> libssh2_knownhost_readfile(knh, knh_file,
> LIBSSH2_KNOWNHOST_FILE_OPENSSH);
>
> r = libssh2_knownhost_checkp(knh, host, port, hostkey, len,
> - LIBSSH2_KNOWNHOST_TYPE_PLAIN|
> + LIBSSH2_KNOWNHOST_TYPE_PLAIN |
> LIBSSH2_KNOWNHOST_KEYENC_RAW,
> &found);
> switch (r) {
> @@ -405,15 +405,18 @@ static int compare_fingerprint(const unsigned char
> *fingerprint, size_t len,
> unsigned c;
>
> while (len > 0) {
> -while (*host_key_check == ':')
> +while (*host_key_check == ':') {
> host_key_check++;
> +}
> if (!qemu_isxdigit(host_key_check[0]) ||
> -!qemu_isxdigit(host_key_check[1]))
> +!qemu_isxdigit(host_key_check[1])) {
> return 1;
> +}
> c = hex2decimal(host_key_check[0]) * 16 +
> hex2decimal(host_key_check[1]);
> -if (c - *fingerprint != 0)
> +if (c - *fingerprint != 0) {
> return c - *fingerprint;
> +}
> fingerprint++;
> len--;
> host_key_check += 2;
> @@ -433,8 +436,8 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,
> return -EINVAL;
> }
>
> -if(compare_fingerprint((unsigned char *) fingerprint, fingerprint_len,
> - hash) != 0) {
> +if (compare_fingerprint((unsigned char *) fingerprint, fingerprint_len,
> +hash) != 0) {
> error_setg(errp, "remote host key does not match host_key_check
> '%s'",
> hash);
> return -EPERM;
> @@ -507,7 +510,7 @@ static int authenticate(BDRVSSHState *s, const char
> *user, Error **errp)
> goto out;
> }
>
> -for(;;) {
> +for (;;) {
> r = libssh2_agent_get_identity(agent, &identity, prev_identity);
> if (r == 1) { /* end of list */
> break;
> @@ -860,8 +863,8 @@ static int ssh_create(const char *filename, QemuOpts
> *opts, Error **errp)
> }
>
> r = connect_to_ssh(&s, uri_options,
> - LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE|
> - LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC,
> + LIBSSH2_FXF_READ | LIBSSH2_FXF_WRITE |
> + LIBSSH2_FXF_CREAT | LIBSSH2_FXF_TRUNC,
> 0644, errp);
> if (r < 0) {
> ret = r;
> @@ -869,7 +872,7 @@ static int ssh_create(const char *filename, QemuOpts
> *opts, Error **errp)
> }
>
> if (total_size > 0) {
> -libssh2_sftp_seek64(s.sftp_handle, total_size-1);
> +libssh2_sftp_seek64(s.sftp_handle, total_size - 1);
> r2 = libssh2_sftp_write(s.sftp_handle, c, 1);
> if (r2 < 0) {
> sftp_error_setg(errp, &s, "truncate failed");
> @@ -1108,7 +,7 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState
> *bs,
> * works for me.
> */
> if (r == 0) {
> -ssh_seek(s, offset + written, SSH_SEEK_WRITE|SSH_SEEK_FORCE);
> +ssh_seek(s, offset + written, SSH_SEEK_WRITE | SSH_SEEK_FORCE);
> co_yield(s, bs);
> goto again;
> }
> @@ -1122,8 +1125,9 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState
> *bs,
> end_of_vec = i->iov_base + i->iov_len;
> }
>
> -if (offset + written > s->attrs.filesize)
> +if (offset + written > s->attrs.filesize) {
> s->attrs.filesize = offset + written;
> +}
> }
>
> return 0;
> --
> 2.9.5
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://
Re: [Qemu-block] [PATCH v3 4/7] block/sheepdog: code beautification
On Tue, Nov 07, 2017 at 05:27:21PM -0500, Jeff Cody wrote:
No functional changes, just whitespace manipulation.
Signed-off-by: Jeff Cody
Reviewed-by: Eric Blake
Reviewed-by: Darren Kenny
---
block/sheepdog.c | 164 +++
1 file changed, 82 insertions(+), 82 deletions(-)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 459d93a..488bad3 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -400,7 +400,7 @@ typedef struct BDRVSheepdogReopenState {
int cache_flags;
} BDRVSheepdogReopenState;
-static const char * sd_strerror(int err)
+static const char *sd_strerror(int err)
{
int i;
@@ -3078,111 +3078,111 @@ static QemuOptsList sd_create_opts = {
};
static BlockDriver bdrv_sheepdog = {
-.format_name= "sheepdog",
-.protocol_name = "sheepdog",
-.instance_size = sizeof(BDRVSheepdogState),
-.bdrv_parse_filename= sd_parse_filename,
-.bdrv_file_open = sd_open,
-.bdrv_reopen_prepare= sd_reopen_prepare,
-.bdrv_reopen_commit = sd_reopen_commit,
-.bdrv_reopen_abort = sd_reopen_abort,
-.bdrv_close = sd_close,
-.bdrv_create= sd_create,
-.bdrv_has_zero_init = bdrv_has_zero_init_1,
-.bdrv_getlength = sd_getlength,
+.format_name = "sheepdog",
+.protocol_name= "sheepdog",
+.instance_size= sizeof(BDRVSheepdogState),
+.bdrv_parse_filename = sd_parse_filename,
+.bdrv_file_open = sd_open,
+.bdrv_reopen_prepare = sd_reopen_prepare,
+.bdrv_reopen_commit = sd_reopen_commit,
+.bdrv_reopen_abort= sd_reopen_abort,
+.bdrv_close = sd_close,
+.bdrv_create = sd_create,
+.bdrv_has_zero_init = bdrv_has_zero_init_1,
+.bdrv_getlength = sd_getlength,
.bdrv_get_allocated_file_size = sd_get_allocated_file_size,
-.bdrv_truncate = sd_truncate,
+.bdrv_truncate= sd_truncate,
-.bdrv_co_readv = sd_co_readv,
-.bdrv_co_writev = sd_co_writev,
-.bdrv_co_flush_to_disk = sd_co_flush_to_disk,
-.bdrv_co_pdiscard = sd_co_pdiscard,
-.bdrv_co_get_block_status = sd_co_get_block_status,
+.bdrv_co_readv= sd_co_readv,
+.bdrv_co_writev = sd_co_writev,
+.bdrv_co_flush_to_disk= sd_co_flush_to_disk,
+.bdrv_co_pdiscard = sd_co_pdiscard,
+.bdrv_co_get_block_status = sd_co_get_block_status,
-.bdrv_snapshot_create = sd_snapshot_create,
-.bdrv_snapshot_goto = sd_snapshot_goto,
-.bdrv_snapshot_delete = sd_snapshot_delete,
-.bdrv_snapshot_list = sd_snapshot_list,
+.bdrv_snapshot_create = sd_snapshot_create,
+.bdrv_snapshot_goto = sd_snapshot_goto,
+.bdrv_snapshot_delete = sd_snapshot_delete,
+.bdrv_snapshot_list = sd_snapshot_list,
-.bdrv_save_vmstate = sd_save_vmstate,
-.bdrv_load_vmstate = sd_load_vmstate,
+.bdrv_save_vmstate= sd_save_vmstate,
+.bdrv_load_vmstate= sd_load_vmstate,
-.bdrv_detach_aio_context = sd_detach_aio_context,
-.bdrv_attach_aio_context = sd_attach_aio_context,
+.bdrv_detach_aio_context = sd_detach_aio_context,
+.bdrv_attach_aio_context = sd_attach_aio_context,
-.create_opts= &sd_create_opts,
+.create_opts = &sd_create_opts,
};
static BlockDriver bdrv_sheepdog_tcp = {
-.format_name= "sheepdog",
-.protocol_name = "sheepdog+tcp",
-.instance_size = sizeof(BDRVSheepdogState),
-.bdrv_parse_filename= sd_parse_filename,
-.bdrv_file_open = sd_open,
-.bdrv_reopen_prepare= sd_reopen_prepare,
-.bdrv_reopen_commit = sd_reopen_commit,
-.bdrv_reopen_abort = sd_reopen_abort,
-.bdrv_close = sd_close,
-.bdrv_create= sd_create,
-.bdrv_has_zero_init = bdrv_has_zero_init_1,
-.bdrv_getlength = sd_getlength,
+.format_name = "sheepdog",
+.protocol_name= "sheepdog+tcp",
+.instance_size= sizeof(BDRVSheepdogState),
+.bdrv_parse_filename = sd_parse_filename,
+.bdrv_file_open = sd_open,
+.bdrv_reopen_prepare = sd_reopen_prepare,
+.bdrv_reopen_commit = sd_reopen_commit,
+.bdrv_reopen_abort= sd_reopen_abort,
+.bdrv_close = sd_close,
+.bdrv_create = sd_create,
+.bdrv_has_zero_init = bdrv_has_zero_init_1,
+.bdrv_getlength = sd_getlength,
.bdrv_get_allocated_file_size = sd_get_allocated_file_size,
-.bdrv_truncate = sd_truncate,
+.bdrv_truncate= sd_truncate,
-.bdrv_co_readv = sd_co_readv,
-.bdrv_co_writev = sd_co_writev,
-.bdrv_co_flush_to_disk = sd_co_flush_to_disk,
-.bdrv_co_pdiscard = sd_co_pdi
Re: [Qemu-block] [PATCH v3 5/7] block/curl: check error return of curl_global_init()
On Tue, Nov 07, 2017 at 05:27:22PM -0500, Jeff Cody wrote:
If curl_global_init() fails, per the documentation no other curl
functions may be called, so make sure to check the return value.
Also, some minor changes to the initialization latch variable 'inited':
- Make it static in the file, for clarity
- Change the name for clarity
- Make it a bool
Signed-off-by: Jeff Cody
Reviewed-by: Eric Blake
Reviewed-by: Darren Kenny
---
block/curl.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/block/curl.c b/block/curl.c
index 2a244e2..00a9879 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -89,6 +89,8 @@ static CURLMcode __curl_multi_socket_action(CURLM
*multi_handle,
struct BDRVCURLState;
+static bool libcurl_initialized;
+
typedef struct CURLAIOCB {
Coroutine *co;
QEMUIOVector *qiov;
@@ -686,14 +688,23 @@ static int curl_open(BlockDriverState *bs, QDict
*options, int flags,
double d;
const char *secretid;
const char *protocol_delimiter;
+int ret;
-static int inited = 0;
if (flags & BDRV_O_RDWR) {
error_setg(errp, "curl block device does not support writes");
return -EROFS;
}
+if (!libcurl_initialized) {
+ret = curl_global_init(CURL_GLOBAL_ALL);
+if (ret) {
+error_setg(errp, "libcurl initialization failed with %d", ret);
+return -EIO;
+}
+libcurl_initialized = true;
+}
+
qemu_mutex_init(&s->mutex);
opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
qemu_opts_absorb_qdict(opts, options, &local_err);
@@ -772,11 +783,6 @@ static int curl_open(BlockDriverState *bs, QDict *options,
int flags,
}
}
-if (!inited) {
-curl_global_init(CURL_GLOBAL_ALL);
-inited = 1;
-}
-
DPRINTF("CURL: Opening %s\n", file);
QSIMPLEQ_INIT(&s->free_state_waitq);
s->aio_context = bdrv_get_aio_context(bs);
--
2.9.5
Re: [Qemu-block] [PATCH v3 2/7] block/ssh: make compliant with coding guidelines
On Tue, Nov 07, 2017 at 05:27:19PM -0500, Jeff Cody wrote:
Signed-off-by: Jeff Cody
Reviewed-by: Eric Blake
Reviewed-by: Darren Kenny
---
block/ssh.c | 32 ++--
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/block/ssh.c b/block/ssh.c
index de81ec8..39cacc1 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -241,7 +241,7 @@ static int parse_uri(const char *filename, QDict *options,
Error **errp)
goto err;
}
-if(uri->user && strcmp(uri->user, "") != 0) {
+if (uri->user && strcmp(uri->user, "") != 0) {
qdict_put_str(options, "user", uri->user);
}
@@ -268,7 +268,7 @@ static int parse_uri(const char *filename, QDict *options,
Error **errp)
err:
if (uri) {
- uri_free(uri);
+uri_free(uri);
}
return -EINVAL;
}
@@ -342,7 +342,7 @@ static int check_host_key_knownhosts(BDRVSSHState *s,
libssh2_knownhost_readfile(knh, knh_file, LIBSSH2_KNOWNHOST_FILE_OPENSSH);
r = libssh2_knownhost_checkp(knh, host, port, hostkey, len,
- LIBSSH2_KNOWNHOST_TYPE_PLAIN|
+ LIBSSH2_KNOWNHOST_TYPE_PLAIN |
LIBSSH2_KNOWNHOST_KEYENC_RAW,
&found);
switch (r) {
@@ -405,15 +405,18 @@ static int compare_fingerprint(const unsigned char
*fingerprint, size_t len,
unsigned c;
while (len > 0) {
-while (*host_key_check == ':')
+while (*host_key_check == ':') {
host_key_check++;
+}
if (!qemu_isxdigit(host_key_check[0]) ||
-!qemu_isxdigit(host_key_check[1]))
+!qemu_isxdigit(host_key_check[1])) {
return 1;
+}
c = hex2decimal(host_key_check[0]) * 16 +
hex2decimal(host_key_check[1]);
-if (c - *fingerprint != 0)
+if (c - *fingerprint != 0) {
return c - *fingerprint;
+}
fingerprint++;
len--;
host_key_check += 2;
@@ -433,8 +436,8 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,
return -EINVAL;
}
-if(compare_fingerprint((unsigned char *) fingerprint, fingerprint_len,
- hash) != 0) {
+if (compare_fingerprint((unsigned char *) fingerprint, fingerprint_len,
+hash) != 0) {
error_setg(errp, "remote host key does not match host_key_check '%s'",
hash);
return -EPERM;
@@ -507,7 +510,7 @@ static int authenticate(BDRVSSHState *s, const char *user,
Error **errp)
goto out;
}
-for(;;) {
+for (;;) {
r = libssh2_agent_get_identity(agent, &identity, prev_identity);
if (r == 1) { /* end of list */
break;
@@ -860,8 +863,8 @@ static int ssh_create(const char *filename, QemuOpts *opts,
Error **errp)
}
r = connect_to_ssh(&s, uri_options,
- LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE|
- LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC,
+ LIBSSH2_FXF_READ | LIBSSH2_FXF_WRITE |
+ LIBSSH2_FXF_CREAT | LIBSSH2_FXF_TRUNC,
0644, errp);
if (r < 0) {
ret = r;
@@ -869,7 +872,7 @@ static int ssh_create(const char *filename, QemuOpts *opts,
Error **errp)
}
if (total_size > 0) {
-libssh2_sftp_seek64(s.sftp_handle, total_size-1);
+libssh2_sftp_seek64(s.sftp_handle, total_size - 1);
r2 = libssh2_sftp_write(s.sftp_handle, c, 1);
if (r2 < 0) {
sftp_error_setg(errp, &s, "truncate failed");
@@ -1108,7 +,7 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState
*bs,
* works for me.
*/
if (r == 0) {
-ssh_seek(s, offset + written, SSH_SEEK_WRITE|SSH_SEEK_FORCE);
+ssh_seek(s, offset + written, SSH_SEEK_WRITE | SSH_SEEK_FORCE);
co_yield(s, bs);
goto again;
}
@@ -1122,8 +1125,9 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState
*bs,
end_of_vec = i->iov_base + i->iov_len;
}
-if (offset + written > s->attrs.filesize)
+if (offset + written > s->attrs.filesize) {
s->attrs.filesize = offset + written;
+}
}
return 0;
--
2.9.5
Re: [Qemu-block] [Qemu-devel] [PATCH] block: Deprecate bdrv_set_read_only() and users
On Wed, Nov 08, 2017 at 11:44:01AM +0100, Paolo Bonzini wrote: > On 07/11/2017 18:39, Daniel P. Berrange wrote: > > On Tue, Nov 07, 2017 at 06:26:38PM +0100, Kevin Wolf wrote: > >> bdrv_set_read_only() is used by some block drivers to override the > >> read-only option given by the user. This is not how read-only images > >> generally work in QEMU: Instead of second guessing what the user really > >> meant (which currently includes making an image read-only even if the > >> user didn't only use the default, but explicitly said read-only=off), we > >> should error out if we can't provide what the user requested. > >> > >> This adds deprecation warnings to all callers of bdrv_set_read_only() so > >> that the behaviour can be corrected after the usual deprecation period. > > > > All deprecations should be listed in "Deprecated features" appendix > > in qemu-doc.texi. This probably fits in the 'system emulator command > > line arguments' section, even though its talking about the need for > > the user to add something extra, rather than deleting something they > > currently use. > > I am not sure this counts as deprecation, but it should go in the > release notes as "future incompatible changes", and that section > probably should go in qemu-doc.texi itself. Yeah, adding a "Incompatible changes" appendix to the qemu-doc.texi would be useful, listing the planned change, and when it is actually made. That way apps adding support for a feature have an indication of any incompatiblities they might need to care about. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-block] [PATCH v3 3/7] block/sheepdog: remove spurious NULL check
On Tue, Nov 07, 2017 at 05:27:20PM -0500, Jeff Cody wrote:
'tag' is already checked in the lines immediately preceding this check,
and set to non-NULL if NULL. No need to check again, it hasn't changed.
Signed-off-by: Jeff Cody
Reviewed-by: Eric Blake
Reviewed-by: Darren Kenny
---
block/sheepdog.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 696a714..459d93a 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1632,7 +1632,7 @@ static int sd_open(BlockDriverState *bs, QDict *options,
int flags,
if (!tag) {
tag = "";
}
-if (tag && strlen(tag) >= SD_MAX_VDI_TAG_LEN) {
+if (strlen(tag) >= SD_MAX_VDI_TAG_LEN) {
error_setg(errp, "value of parameter 'tag' is too long");
ret = -EINVAL;
goto err_no_fd;
--
2.9.5
Re: [Qemu-block] [PATCH v3 7/7] block/curl: code cleanup to comply with coding style
Hi Jeff,
While I'm relatively new to this community, I do have some comments
about the styling in this file.
I don't see anything in the CODING_STYLE file that tells me I'm
wrong here, but it's certainly possible...
More inline.
On Tue, Nov 07, 2017 at 05:27:24PM -0500, Jeff Cody wrote:
This addresses non-functional changes to help curl.c better comply
with the coding styles (comments, indentation, brackets, etc.).
One minor code change is the combination of two if statements into
a single if statement.
Signed-off-by: Jeff Cody
Reviewed-by: Eric Blake
---
block/curl.c | 100 +++
1 file changed, 52 insertions(+), 48 deletions(-)
diff --git a/block/curl.c b/block/curl.c
index 35cf417..7a6dd44 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -32,8 +32,10 @@
#include
#include "qemu/cutils.h"
-// #define DEBUG_CURL
-// #define DEBUG_VERBOSE
+/*
+ #define DEBUG_CURL
+ #define DEBUG_VERBOSE
+*/
Is it not more common to use the style:
/*
* text
*/
This is visible in almost every file at the top, where the Copyright
and License is.
#ifdef DEBUG_CURL
#define DEBUG_CURL_PRINT 1
@@ -76,15 +78,15 @@ static CURLMcode __curl_multi_socket_action(CURLM
*multi_handle,
#define CURL_TIMEOUT_DEFAULT 5
#define CURL_TIMEOUT_MAX 1
-#define CURL_BLOCK_OPT_URL "url"
-#define CURL_BLOCK_OPT_READAHEAD "readahead"
-#define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
-#define CURL_BLOCK_OPT_TIMEOUT "timeout"
-#define CURL_BLOCK_OPT_COOKIE"cookie"
-#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret"
-#define CURL_BLOCK_OPT_USERNAME "username"
-#define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret"
-#define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"
+#define CURL_BLOCK_OPT_URL "url"
+#define CURL_BLOCK_OPT_READAHEAD "readahead"
+#define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
+#define CURL_BLOCK_OPT_TIMEOUT "timeout"
+#define CURL_BLOCK_OPT_COOKIE"cookie"
+#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret"
+#define CURL_BLOCK_OPT_USERNAME "username"
+#define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret"
+#define CURL_BLOCK_OPT_PROXY_USERNAME"proxy-username"
#define CURL_BLOCK_OPT_PROXY_PASSWORD_SECRET "proxy-password-secret"
struct BDRVCURLState;
@@ -110,8 +112,7 @@ typedef struct CURLSocket {
QLIST_ENTRY(CURLSocket) next;
} CURLSocket;
-typedef struct CURLState
-{
+typedef struct CURLState {
struct BDRVCURLState *s;
CURLAIOCB *acb[CURL_NUM_ACB];
CURL *curl;
@@ -196,22 +197,22 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int
action,
DPRINTF("CURL (AIO): Sock action %d on fd %d\n", action, (int)fd);
switch (action) {
-case CURL_POLL_IN:
-aio_set_fd_handler(s->aio_context, fd, false,
- curl_multi_read, NULL, NULL, state);
-break;
-case CURL_POLL_OUT:
-aio_set_fd_handler(s->aio_context, fd, false,
- NULL, curl_multi_do, NULL, state);
-break;
-case CURL_POLL_INOUT:
-aio_set_fd_handler(s->aio_context, fd, false,
- curl_multi_read, curl_multi_do, NULL, state);
-break;
-case CURL_POLL_REMOVE:
-aio_set_fd_handler(s->aio_context, fd, false,
- NULL, NULL, NULL, NULL);
-break;
+case CURL_POLL_IN:
+aio_set_fd_handler(s->aio_context, fd, false,
+ curl_multi_read, NULL, NULL, state);
+break;
+case CURL_POLL_OUT:
+aio_set_fd_handler(s->aio_context, fd, false,
+ NULL, curl_multi_do, NULL, state);
+break;
+case CURL_POLL_INOUT:
+aio_set_fd_handler(s->aio_context, fd, false,
+ curl_multi_read, curl_multi_do, NULL, state);
+break;
+case CURL_POLL_REMOVE:
+aio_set_fd_handler(s->aio_context, fd, false,
+ NULL, NULL, NULL, NULL);
+break;
}
return 0;
@@ -235,7 +236,7 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t
nmemb, void *opaque)
/* Called from curl_multi_do_locked, with s->mutex held. */
static size_t curl_read_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
{
-CURLState *s = ((CURLState*)opaque);
+CURLState *s = opaque;
Not sure about this one - while it may not be strictly necessary to
do the casting, from a readability point of view it is helpful to
see the cast - possibly with the outer brackets removed:
CURLState *s = (CURLState*)opaque;
size_t realsize = size * nmemb;
int i;
@@ -253,11 +254,12 @@ static size_t curl_read_cb(void *ptr, size_t size, size_t
nmemb, void *opaque)
memcpy(s->orig_buf + s->buf_off, ptr, realsize);
s->buf_off += realsize;
-for(i=0; iacb[i];
-if (!acb)
+if (!acb) {
Re: [Qemu-block] [Qemu-devel] [PATCH] block: Deprecate bdrv_set_read_only() and users
On 07/11/2017 18:39, Daniel P. Berrange wrote:
> On Tue, Nov 07, 2017 at 06:26:38PM +0100, Kevin Wolf wrote:
>> bdrv_set_read_only() is used by some block drivers to override the
>> read-only option given by the user. This is not how read-only images
>> generally work in QEMU: Instead of second guessing what the user really
>> meant (which currently includes making an image read-only even if the
>> user didn't only use the default, but explicitly said read-only=off), we
>> should error out if we can't provide what the user requested.
>>
>> This adds deprecation warnings to all callers of bdrv_set_read_only() so
>> that the behaviour can be corrected after the usual deprecation period.
>
> All deprecations should be listed in "Deprecated features" appendix
> in qemu-doc.texi. This probably fits in the 'system emulator command
> line arguments' section, even though its talking about the need for
> the user to add something extra, rather than deleting something they
> currently use.
I am not sure this counts as deprecation, but it should go in the
release notes as "future incompatible changes", and that section
probably should go in qemu-doc.texi itself.
Paolo
>
>>
>> Signed-off-by: Kevin Wolf
>> ---
>> block.c | 5 +
>> block/bochs.c | 13 ++---
>> block/cloop.c | 13 ++---
>> block/dmg.c | 12 +---
>> block/rbd.c | 14 ++
>> block/vvfat.c | 6 +-
>> 6 files changed, 49 insertions(+), 14 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index f6415547fe..0ed0c27140 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -261,6 +261,11 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool
>> read_only,
>> return 0;
>> }
>>
>> +/* TODO Remove (deprecated since 2.11)
>> + * Block drivers are not supposed to automatically change bs->read_only.
>> + * Instead, they should just check whether they can provide what the user
>> + * explicitly requested and error out if read-write is requested, but they
>> can
>> + * only provide read-only access. */
>> int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>> {
>> int ret = 0;
>> diff --git a/block/bochs.c b/block/bochs.c
>> index a759b6eff0..50c630047b 100644
>> --- a/block/bochs.c
>> +++ b/block/bochs.c
>> @@ -28,6 +28,7 @@
>> #include "block/block_int.h"
>> #include "qemu/module.h"
>> #include "qemu/bswap.h"
>> +#include "qemu/error-report.h"
>>
>> /**/
>>
>> @@ -110,9 +111,15 @@ static int bochs_open(BlockDriverState *bs, QDict
>> *options, int flags,
>> return -EINVAL;
>> }
>>
>> -ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
>> -if (ret < 0) {
>> -return ret;
>> +if (!bdrv_is_read_only(bs)) {
>> +error_report("Opening bochs images without an explicit read-only=on
>> "
>> + "option is deprecated. Future versions will refuse to "
>> + "open the image instead of automatically marking the "
>> + "image read-only.");
>> +ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
>> +if (ret < 0) {
>> +return ret;
>> +}
>> }
>>
>> ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
>> diff --git a/block/cloop.c b/block/cloop.c
>> index d6597fcf78..2be68987bd 100644
>> --- a/block/cloop.c
>> +++ b/block/cloop.c
>> @@ -23,6 +23,7 @@
>> */
>> #include "qemu/osdep.h"
>> #include "qapi/error.h"
>> +#include "qemu/error-report.h"
>> #include "qemu-common.h"
>> #include "block/block_int.h"
>> #include "qemu/module.h"
>> @@ -72,9 +73,15 @@ static int cloop_open(BlockDriverState *bs, QDict
>> *options, int flags,
>> return -EINVAL;
>> }
>>
>> -ret = bdrv_set_read_only(bs, true, errp);
>> -if (ret < 0) {
>> -return ret;
>> +if (!bdrv_is_read_only(bs)) {
>> +error_report("Opening cloop images without an explicit read-only=on
>> "
>> + "option is deprecated. Future versions will refuse to "
>> + "open the image instead of automatically marking the "
>> + "image read-only.");
>> +ret = bdrv_set_read_only(bs, true, errp);
>> +if (ret < 0) {
>> +return ret;
>> +}
>> }
>>
>> /* read header */
>> diff --git a/block/dmg.c b/block/dmg.c
>> index 6c0711f563..c9b3c519c4 100644
>> --- a/block/dmg.c
>> +++ b/block/dmg.c
>> @@ -419,9 +419,15 @@ static int dmg_open(BlockDriverState *bs, QDict
>> *options, int flags,
>> return -EINVAL;
>> }
>>
>> -ret = bdrv_set_read_only(bs, true, errp);
>> -if (ret < 0) {
>> -return ret;
>> +if (!bdrv_is_read_only(bs)) {
>> +error_report("Opening dmg images without an explicit read-only=on "
>> + "option is deprecated. Future versions will refuse to "
>> + "open the im
Re: [Qemu-block] [PATCH] block: Deprecate bdrv_set_read_only() and users
Am 07.11.2017 um 21:29 hat Eric Blake geschrieben:
> On 11/07/2017 11:26 AM, Kevin Wolf wrote:
> > bdrv_set_read_only() is used by some block drivers to override the
> > read-only option given by the user. This is not how read-only images
> > generally work in QEMU: Instead of second guessing what the user really
> > meant (which currently includes making an image read-only even if the
> > user didn't only use the default, but explicitly said read-only=off), we
> > should error out if we can't provide what the user requested.
> >
> > This adds deprecation warnings to all callers of bdrv_set_read_only() so
> > that the behaviour can be corrected after the usual deprecation period.
> >
> > Signed-off-by: Kevin Wolf
> > ---
> > block.c | 5 +
> > block/bochs.c | 13 ++---
> > block/cloop.c | 13 ++---
> > block/dmg.c | 12 +---
> > block/rbd.c | 14 ++
> > block/vvfat.c | 6 +-
> > 6 files changed, 49 insertions(+), 14 deletions(-)
>
> Dan pointed out the missing documentation, but for the code itself, the
> approach looks sane (especially since it was my attempt to make it worse
> by extending the idiom to NBD that triggered you to write this patch).
>
> Other documentation: In qapi/block-core.json, @BlockdevOptions, we
> probably ought to mention under @read-only that some block drivers
> require the use of an explicit read-only.
Well, they don't only need an explicitly set option, but the important
point is that they don't work with the default value. But I can add
something to this effect.
> > +++ b/block/vvfat.c
> > @@ -1259,7 +1259,11 @@ static int vvfat_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > "Unable to set VVFAT to 'rw' when drive is
> > read-only");
> > goto fail;
> > }
> > -} else {
> > +} else if (!bdrv_is_read_only(bs)) {
> > +error_report("Opening non-rw vvfat images without an explicit "
> > + "read-only=on option is deprecated. Future versions "
> > + "will refuse to open the image instead of "
> > + "automatically marking the image read-only.");
> > /* read only is the default for safety */
> > ret = bdrv_set_read_only(bs, true, &local_err);
>
> Is this also a good time to deprecate vvfat's duplication of rw vs.
> read-only, and consolidate that into a single option? No other device
> defaults to read-only, so the deprecation period is a good point to warn
> that a future version may default to read-write without an explicit
> read-only. I guess vvfat is the only driver with a device-specific QAPI
> change (for 'rw') that might be impacted if you make that additional change.
I would love to get rid of the duplication, but there's a reason why
vvfat defaults to read-only. I think we're relatively confident that a
read-only vvfat can be safely implemented (and hopefully is), but write
support is really a clever hack that may or may not work reliably
depending on how crazy the guest OS goes.
So if we removed the 'rw' option, would we want 'read-only' to default
to true for vvfat? I'm not sure if we want to go there, it would mean
making the default value of some base BlockdevOptions depend on the
driver.
On the other hand, I'm not sure how useful 'read-only' even is apart
from the protocol layer... Should it have been driver-specific? But it's
too late for that anyway.
Kevin
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v2 4/7] qcow2: Don't open images with header.refcount_table_clusters == 0
On Tue 07 Nov 2017 05:43:49 PM CET, Kevin Wolf wrote: >> +echo >> +echo "=== Testing zero refcount table size ===" >> +echo >> +_make_test_img 64M >> +poke_file "$TEST_IMG" "56""\x00\x00\x00\x00" >> +$QEMU_IO -c "write 0 64k" "$TEST_IMG" 2>&1 | _filter_testdir | >> _filter_imgfmt > > In the commit message, you claim that the image can be repaired. Would > it be worth actually testing the repair here? Good idea. Max already merged this series in his branch, so I'll just send a follow-up patch with this. Berto
