[PATCH] iotests/testrunner.py: refactor test_field_width
A lot of Optional[] types doesn't make code beautiful.
test_field_width defaults to 8, but that is never used in the code.
More over, if we want some default behavior for single call of
test_run(), it should just print the whole test name, not limiting or
expanding its width, so 8 is bad default.
So, just drop the default as unused for now.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
This is a follow-up for "[PATCH 0/3] iotests: multiprocessing!!" and
based on Hanna's block-next branch.
Based-on: <2021120313.2780098-1-vsement...@virtuozzo.com>
tests/qemu-iotests/testrunner.py | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 0feaa396d0..15788f919e 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -174,19 +174,17 @@ def __enter__(self) -> 'TestRunner':
def __exit__(self, exc_type: Any, exc_value: Any, traceback: Any) -> None:
self._stack.close()
-def test_print_one_line(self, test: str, starttime: str,
+def test_print_one_line(self, test: str,
+test_field_width: int,
+starttime: str,
endtime: Optional[str] = None, status: str = '...',
lasttime: Optional[float] = None,
thistime: Optional[float] = None,
description: str = '',
-test_field_width: Optional[int] = None,
end: str = '\n') -> None:
""" Print short test info before/after test run """
test = os.path.basename(test)
-if test_field_width is None:
-test_field_width = 8
-
if self.makecheck and status != '...':
if status and status != 'pass':
status = f' [{status}]'
@@ -328,7 +326,7 @@ def do_run_test(self, test: str, mp: bool) -> TestResult:
casenotrun=casenotrun)
def run_test(self, test: str,
- test_field_width: Optional[int] = None,
+ test_field_width: int,
mp: bool = False) -> TestResult:
"""
Run one test and print short status
@@ -347,20 +345,21 @@ def run_test(self, test: str,
if not self.makecheck:
self.test_print_one_line(test=test,
+ test_field_width=test_field_width,
status = 'started' if mp else '...',
starttime=start,
lasttime=last_el,
- end = '\n' if mp else '\r',
- test_field_width=test_field_width)
+ end = '\n' if mp else '\r')
res = self.do_run_test(test, mp)
end = datetime.datetime.now().strftime('%H:%M:%S')
-self.test_print_one_line(test=test, status=res.status,
+self.test_print_one_line(test=test,
+ test_field_width=test_field_width,
+ status=res.status,
starttime=start, endtime=end,
lasttime=last_el, thistime=res.elapsed,
- description=res.description,
- test_field_width=test_field_width)
+ description=res.description)
if res.casenotrun:
print(res.casenotrun)
--
2.31.1
Re: [PATCH 2/3] iotests/testrunner.py: move updating last_elapsed to run_tests
10.12.2021 17:47, Vladimir Sementsov-Ogievskiy wrote:
10.12.2021 17:25, Kevin Wolf wrote:
Am 06.12.2021 um 18:59 hat John Snow geschrieben:
On Fri, Dec 3, 2021 at 7:22 AM Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> wrote:
We are going to use do_run_test() in multiprocessing environment, where
we'll not be able to change original runner object.
Happily, the only thing we change is that last_elapsed and it's simple
to do it in run_tests() instead. All other accesses to self in
do_runt_test() and in run_test() are read-only.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/testrunner.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/testrunner.py
b/tests/qemu-iotests/testrunner.py
index fa842252d3..a9f2feb58c 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -287,7 +287,6 @@ def do_run_test(self, test: str) -> TestResult:
diff=diff, casenotrun=casenotrun)
else:
f_bad.unlink()
- self.last_elapsed.update(test, elapsed)
return TestResult(status='pass', elapsed=elapsed,
casenotrun=casenotrun)
@@ -353,6 +352,9 @@ def run_tests(self, tests: List[str]) -> bool:
print('\n'.join(res.diff))
elif res.status == 'not run':
notrun.append(name)
+ elif res.status == 'pass':
+ assert res.elapsed is not None
+ self.last_elapsed.update(t, res.elapsed)
sys.stdout.flush()
if res.interrupted:
--
2.31.1
(I continue to be annoyed by the "None" problem in mypy, but I suppose it
really can't be helped. Nothing for you to change with this patch or
series. I just wish we didn't need so many assertions ...)
I'm inclined to say it's a None problem in our code, not in mypy.
Essentially it comes from the fact that we're abusing a string
(res.status) and None values to distinguish different types of results
that have a different set of valid attributes.
Of course, Python already provides a language feature to distinguish
different types of results that have a different set of attributes and
that wouldn't run into this problem: subclasses.
Agree, you are right. I'll look if it is simple to refactor.
No it's not simple)
Actually, it means making TestResult more smart, and moving (most of) logic of test result
result parsing (checking different files, etc) to TestResult.. But we'll still want to update
last_elapsed.. Adding a method "TestResult.update_runnner(runner)", which will be
no-op in base TestResult and will be actually realized only in TestResult subclass that have
.elapsed to call runner.update_last_elapsed()? And we'll have a hierarhy like TestResultBase
-> TestResultWithElapsed -> (TestResultFail, TestResultPass).. At least, that's what I
imagine, and I don't like it :)
--
Best regards,
Vladimir
Re: [PATCH] spec: Add NBD_OPT_EXTENDED_HEADERS
04.12.2021 02:14, Eric Blake wrote: Add a new negotiation feature where the client and server agree to use larger packet headers on every packet sent during transmission phase. This has two purposes: first, it makes it possible to perform operations like trim, write zeroes, and block status on more than 2^32 bytes in a single command; this in turn requires that some structured replies from the server also be extended to match. The wording chosen here is careful to permit a server to use either flavor in its reply (that is, a request less than 32-bits can trigger an extended reply, and conversely a request larger than 32-bits can trigger a compact reply). About this.. Isn't it too permissive? I think that actually having to very similar ways to do the same thing is usually a bad design. I think we don't want someone implement the logic, which tries to send 32bit commands/replies for small requests and 64bit command/replies for larger ones? Moreover, you don't allow doing it for commands. So, for symmetry, it may be good to be strict with replies too: in 64bit mode only 64bit replies. Now we of course have to support old 32bit commands and new 64bit commands. But, may be, we'll want to deprecate 32bit commands at some moment? I'm not sure we can deprecate them in protocol, but we can deprecate them in Qemu at least. And several years later we'll drop old code, keeping only support for 64bit commands. Less code paths, less similar structures, simpler code, I think it worth it. -- Best regards, Vladimir
Re: [PATCH] spec: Add NBD_OPT_EXTENDED_HEADERS
07.12.2021 12:08, Vladimir Sementsov-Ogievskiy wrote: 07.12.2021 02:00, Eric Blake wrote: On Mon, Dec 06, 2021 at 02:40:45PM +0300, Vladimir Sementsov-Ogievskiy wrote: [..] +S: 64 bits, padding (MUST be zero) Hmm. Extra 8 bytes to be power-of-2. Does 32 bytes really perform better than 24 bytes? Consider: struct header[100]; if sizeof(header[0]) is a power of 2 <= the cache line size (and the compiler prefers to start arrays aligned to the cache line) then we are guaranteed that all array members each reside in a single cache line. But if it is not a power of 2, some of the array members straddle two cache lines. Will there be code that wants to create an array of headers? Perhaps so, because that is a logical way (along with scatter/gather to combine the header with variable-sized payloads) of tracking the headers for multiple commands issued in parallel. Do I have actual performance numbers? No. But there's plenty of google hits for why sizing structs to a power of 2 is a good idea. I have a thought: If client stores headers in separate, nothing prevents make this padding in RAM. So you can define header struct with padding. But what a reason to make the padding in the stream? You can have and array of good-aligned structures, but fill only part of header structure reading from the socket. Note, that you can read only one header in one read() call anyway, as you have to analyze, does it have payload or not. So, if we want to improve performance by padding the structures in RAM, it's not a reason for padding them in the wire, keeping in mind that we can't read more then one structure at once. -- Best regards, Vladimir
Re: [PATCH v5 09/31] block: introduce assert_bdrv_graph_writable
On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote:
We want to be sure that the functions that write the child and
parent list of a bs are under BQL and drain.
BQL prevents from concurrent writings from the GS API, while
drains protect from I/O.
TODO: drains are missing in some functions using this assert.
Therefore a proper assertion will fail. Because adding drains
requires additional discussions, they will be added in future
series.
Signed-off-by: Emanuele Giuseppe Esposito
---
include/block/block_int-global-state.h | 10 +-
block.c| 4
block/io.c | 11 +++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/include/block/block_int-global-state.h
b/include/block/block_int-global-state.h
index a1b7d0579d..fa96e8b449 100644
--- a/include/block/block_int-global-state.h
+++ b/include/block/block_int-global-state.h
@@ -312,4 +312,12 @@ void bdrv_remove_aio_context_notifier(BlockDriverState *bs,
*/
void bdrv_drain_all_end_quiesce(BlockDriverState *bs);
-#endif /* BLOCK_INT_GLOBAL_STATE*/
This looks like it should be squashed into patch 7, sorry I missed this
in v4...
(Rest of this patch looks good to me, for the record – and while I’m at
it, for patches I didn’t reply to so far, I planned to send an R-b
later. But then there’s things like patches 2/3 looking good to me, but
it turned out in my review for patch 4 that bdrv_lock_medium() is used
in an I/O path, so I can’t really send an R-b now anymore...)
Hanna
+/**
+ * Make sure that the function is running under both drain and BQL.
+ * The latter protects from concurrent writings
+ * from the GS API, while the former prevents concurrent reads
+ * from I/O.
+ */
+void assert_bdrv_graph_writable(BlockDriverState *bs);
+
+#endif /* BLOCK_INT_GLOBAL_STATE */
diff --git a/block.c b/block.c
index 198ec636ff..522a273140 100644
--- a/block.c
+++ b/block.c
@@ -1416,6 +1416,7 @@ static void bdrv_child_cb_attach(BdrvChild *child)
{
BlockDriverState *bs = child->opaque;
+assert_bdrv_graph_writable(bs);
QLIST_INSERT_HEAD(>children, child, next);
if (child->role & BDRV_CHILD_COW) {
@@ -1435,6 +1436,7 @@ static void bdrv_child_cb_detach(BdrvChild *child)
bdrv_unapply_subtree_drain(child, bs);
+assert_bdrv_graph_writable(bs);
QLIST_REMOVE(child, next);
}
@@ -2818,6 +2820,7 @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
if (child->klass->detach) {
child->klass->detach(child);
}
+assert_bdrv_graph_writable(old_bs);
QLIST_REMOVE(child, next_parent);
}
@@ -2827,6 +2830,7 @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
}
if (new_bs) {
+assert_bdrv_graph_writable(new_bs);
QLIST_INSERT_HEAD(_bs->parents, child, next_parent);
/*
diff --git a/block/io.c b/block/io.c
index cb095deeec..3be08cad29 100644
--- a/block/io.c
+++ b/block/io.c
@@ -734,6 +734,17 @@ void bdrv_drain_all(void)
bdrv_drain_all_end();
}
+void assert_bdrv_graph_writable(BlockDriverState *bs)
+{
+/*
+ * TODO: this function is incomplete. Because the users of this
+ * assert lack the necessary drains, check only for BQL.
+ * Once the necessary drains are added,
+ * assert also for qatomic_read(>quiesce_counter) > 0
+ */
+assert(qemu_in_main_thread());
+}
+
/**
* Remove an active request from the tracked requests list
*
Re: [PATCH 3/3] iotests: check: multiprocessing support
Am 10.12.2021 um 15:46 hat Vladimir Sementsov-Ogievskiy geschrieben: > 10.12.2021 17:36, Kevin Wolf wrote: > > Am 03.12.2021 um 13:22 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > Add -j parameter, to run tests in several jobs simultaneously. > > > For realization - simply utilize multiprocessing.Pool class. > > > > > > Notes: > > > > > > 1. Of course, tests can't run simultaneously in same TEST_DIR. So, > > > use subdirectories TEST_DIR/testname/ and SOCK_DIR/testname/ > > > instead of simply TEST_DIR and SOCK_DIR > > > > > > 2. multiprocessing.Pool.starmap function doesn't support passing > > > context managers, so we can't simply pass "self". Happily, we need > > > self only for read-only access, and it just works if it is defined > > > in global space. So, add a temporary link TestRunner.shared_self > > > during run_tests(). > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > > > > Just wondering, is it worth even supporting the mp=false case or can we > > simplify the code a bit by always going through multiprocessing and > > using nice directory names even if only one process is spawned? > > > > Maybe John's observation that directory names get longer might be a > > reason not to do that by default. Any other reasons you're aware of? > > I just wanted to keep the behavior without a new option unchanged, to > not deal with possible CI failures on "make check": who know what > multiprocessing brings together with performance. So basically just err on the side of caution. Makes sense. Kevin
Re: [PATCH 0/2] block-backend: Retain permissions after migration
Am 25.11.2021 um 14:53 hat Hanna Reitz geschrieben: > Hi, > > Peng Liang has reported an issue regarding migration of raw images here: > https://lists.nongnu.org/archive/html/qemu-block/2021-11/msg00673.html > > It turns out that after migrating, all permissions are shared when they > weren’t before. The cause of the problem is that we deliberately delay > restricting the shared permissions until migration is really done (until > the runstate is no longer INMIGRATE) and first share all permissions; > but this causes us to lose the original shared permission mask and > overwrites it with BLK_PERM_ALL, so once we do try to restrict the > shared permissions, we only again share them all. > > Fix this by saving the set of shared permissions through the first > blk_perm_set() call that shares all; and add a regression test. > > > I don’t believe we have to fix this in 6.2, because I think this bug has > existed for four years now. (I.e. it isn’t critical, and it’s no > regression.) Feels a bit like a hack, but I guess as long as it works... :-) Reviewed-by: Kevin Wolf
Re: [PATCH v5 06/31] block/block-backend.c: assertions for block-backend
On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote: All the global state (GS) API functions will check that qemu_in_main_thread() returns true. If not, it means that the safety of BQL cannot be guaranteed, and they need to be moved to I/O. Signed-off-by: Emanuele Giuseppe Esposito --- block/block-backend.c | 83 ++ softmmu/qdev-monitor.c | 2 + 2 files changed, 85 insertions(+) So given my thoughts on patch 5, I believe that blk_set_perm() and blk_get_perm() should get assertions here. Hanna
Re: [PATCH 3/3] iotests: check: multiprocessing support
10.12.2021 17:36, Kevin Wolf wrote: Am 03.12.2021 um 13:22 hat Vladimir Sementsov-Ogievskiy geschrieben: Add -j parameter, to run tests in several jobs simultaneously. For realization - simply utilize multiprocessing.Pool class. Notes: 1. Of course, tests can't run simultaneously in same TEST_DIR. So, use subdirectories TEST_DIR/testname/ and SOCK_DIR/testname/ instead of simply TEST_DIR and SOCK_DIR 2. multiprocessing.Pool.starmap function doesn't support passing context managers, so we can't simply pass "self". Happily, we need self only for read-only access, and it just works if it is defined in global space. So, add a temporary link TestRunner.shared_self during run_tests(). Signed-off-by: Vladimir Sementsov-Ogievskiy Just wondering, is it worth even supporting the mp=false case or can we simplify the code a bit by always going through multiprocessing and using nice directory names even if only one process is spawned? Maybe John's observation that directory names get longer might be a reason not to do that by default. Any other reasons you're aware of? I just wanted to keep the behavior without a new option unchanged, to not deal with possible CI failures on "make check": who know what multiprocessing brings together with performance. -- Best regards, Vladimir
Re: [PATCH 2/3] iotests/testrunner.py: move updating last_elapsed to run_tests
10.12.2021 17:25, Kevin Wolf wrote:
Am 06.12.2021 um 18:59 hat John Snow geschrieben:
On Fri, Dec 3, 2021 at 7:22 AM Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> wrote:
We are going to use do_run_test() in multiprocessing environment, where
we'll not be able to change original runner object.
Happily, the only thing we change is that last_elapsed and it's simple
to do it in run_tests() instead. All other accesses to self in
do_runt_test() and in run_test() are read-only.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/testrunner.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/testrunner.py
b/tests/qemu-iotests/testrunner.py
index fa842252d3..a9f2feb58c 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -287,7 +287,6 @@ def do_run_test(self, test: str) -> TestResult:
diff=diff, casenotrun=casenotrun)
else:
f_bad.unlink()
-self.last_elapsed.update(test, elapsed)
return TestResult(status='pass', elapsed=elapsed,
casenotrun=casenotrun)
@@ -353,6 +352,9 @@ def run_tests(self, tests: List[str]) -> bool:
print('\n'.join(res.diff))
elif res.status == 'not run':
notrun.append(name)
+elif res.status == 'pass':
+assert res.elapsed is not None
+self.last_elapsed.update(t, res.elapsed)
sys.stdout.flush()
if res.interrupted:
--
2.31.1
(I continue to be annoyed by the "None" problem in mypy, but I suppose it
really can't be helped. Nothing for you to change with this patch or
series. I just wish we didn't need so many assertions ...)
I'm inclined to say it's a None problem in our code, not in mypy.
Essentially it comes from the fact that we're abusing a string
(res.status) and None values to distinguish different types of results
that have a different set of valid attributes.
Of course, Python already provides a language feature to distinguish
different types of results that have a different set of attributes and
that wouldn't run into this problem: subclasses.
Agree, you are right. I'll look if it is simple to refactor.
--
Best regards,
Vladimir
Re: [PATCH 1/3] iotests/testrunner.py: add doc string for run_test()
10.12.2021 17:12, Kevin Wolf wrote:
Am 03.12.2021 um 13:22 hat Vladimir Sementsov-Ogievskiy geschrieben:
We are going to modify these methods and will add more documentation in
further commit. As a preparation add basic documentation.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/testrunner.py | 13 +
1 file changed, 13 insertions(+)
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 0e29c2fddd..fa842252d3 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -220,6 +220,12 @@ def find_reference(self, test: str) -> str:
return f'{test}.out'
def do_run_test(self, test: str) -> TestResult:
+"""
+Run one test
+
+:param test: test file path
+"""
+
f_test = Path(test)
f_bad = Path(f_test.name + '.out.bad')
f_notrun = Path(f_test.name + '.notrun')
@@ -287,6 +293,13 @@ def do_run_test(self, test: str) -> TestResult:
def run_test(self, test: str,
test_field_width: Optional[int] = None) -> TestResult:
+"""
+Run one test and print short status
+
+:param test: test file path
+:param test_field_width: width for first field of status format
+"""
+
last_el = self.last_elapsed.get(test)
start = datetime.datetime.now().strftime('%H:%M:%S')
test_field_width is Optional[int], so the documentation should specify
what happens when you pass None.
Seems it doesn't change the behaviour, but just picks a default value of
8. 'test_field_width: int = 8' might have been the more obvious solution
for this in the first place.
Kevin
Agree, I'll make a follow-up patch for it.
--
Best regards,
Vladimir
Re: [PATCH v5 05/31] block-backend: special comments for blk_set/get_perm due to fuse
On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote:
Fuse logic can be classified as I/O, so there is no BQL held
during its execution. And yet, it uses blk_{get/set}_perm
functions, that are classified as BQL and clearly require
the BQL lock. Since there is no easy solution for this,
add a couple of TODOs and FIXME in the relevant sections of the
code.
Well, the problem goes deeper, because we still consider them GS
functions. So it’s fine for the permission function
raw_handle_perm_lock() to call bdrv_get_flags(), which is a GS function,
and then you still get:
qemu-storage-daemon: ../block.c:6195: bdrv_get_flags: Assertion
`qemu_in_main_thread()' failed.
It looks like in this case making bdrv_get_flags() not a GS function
would be feasible and might solve the problem, but I guess we should
instead think about adding something like
if (!exp->growable && !qemu_in_main_thread()) {
/* Changing permissions like below only works in the main thread */
return -EPERM;
}
to fuse_do_truncate().
Ideally, to make up for this, we should probably take the RESIZE
permission in fuse_export_create() for writable exports in iothreads.
Signed-off-by: Emanuele Giuseppe Esposito
---
block/block-backend.c | 10 ++
block/export/fuse.c | 16 ++--
2 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 1f0bda578e..7a4b50a8f0 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -888,6 +888,11 @@ int blk_set_perm(BlockBackend *blk, uint64_t perm,
uint64_t shared_perm,
Error **errp)
{
int ret;
+/*
+ * FIXME: blk_{get/set}_perm should be always called under
+ * BQL, but it is not the case right now (see block/export/fuse.c)
+ */
+/* assert(qemu_in_main_thread()); */
if (blk->root && !blk->disable_perm) {
ret = bdrv_child_try_set_perm(blk->root, perm, shared_perm, errp);
@@ -904,6 +909,11 @@ int blk_set_perm(BlockBackend *blk, uint64_t perm,
uint64_t shared_perm,
void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm)
{
+/*
+ * FIXME: blk_{get/set}_perm should be always called under
+ * BQL, but it is not the case right now (see block/export/fuse.c)
+ */
+/* assert(qemu_in_main_thread()); */
*perm = blk->perm;
*shared_perm = blk->shared_perm;
}
diff --git a/block/export/fuse.c b/block/export/fuse.c
index 823c126d23..7ceb8d783b 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -89,7 +89,10 @@ static int fuse_export_create(BlockExport *blk_exp,
/* For growable exports, take the RESIZE permission */
if (args->growable) {
uint64_t blk_perm, blk_shared_perm;
-
+/*
+ * FIXME: blk_{get/set}_perm should not be here, as permissions
+ * should be modified only under BQL and here we are not!
+ */
I believe we are, BlockExportDriver.create() is called by blk_exp_add(),
which looks very much like it runs in the main thread (calling
bdrv_lookup_bs(), bdrv_try_set_aio_context(), blk_new(), and whatnot).
Hanna
blk_get_perm(exp->common.blk, _perm, _shared_perm);
ret = blk_set_perm(exp->common.blk, blk_perm | BLK_PERM_RESIZE,
@@ -400,6 +403,11 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t
size,
/* Growable exports have a permanent RESIZE permission */
if (!exp->growable) {
+
+/*
+ * FIXME: blk_{get/set}_perm should not be here, as permissions
+ * should be modified only under BQL and here we are not!
+ */
blk_get_perm(exp->common.blk, _perm, _shared_perm);
ret = blk_set_perm(exp->common.blk, blk_perm | BLK_PERM_RESIZE,
@@ -413,7 +421,11 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t
size,
truncate_flags, NULL);
if (!exp->growable) {
-/* Must succeed, because we are only giving up the RESIZE permission */
+/*
+ * Must succeed, because we are only giving up the RESIZE permission.
+ * FIXME: blk_{get/set}_perm should not be here, as permissions
+ * should be modified only under BQL and here we are not!
+ */
blk_set_perm(exp->common.blk, blk_perm, blk_shared_perm,
_abort);
}
Re: [PATCH 3/3] iotests: check: multiprocessing support
Am 03.12.2021 um 13:22 hat Vladimir Sementsov-Ogievskiy geschrieben: > Add -j parameter, to run tests in several jobs simultaneously. > For realization - simply utilize multiprocessing.Pool class. > > Notes: > > 1. Of course, tests can't run simultaneously in same TEST_DIR. So, >use subdirectories TEST_DIR/testname/ and SOCK_DIR/testname/ >instead of simply TEST_DIR and SOCK_DIR > > 2. multiprocessing.Pool.starmap function doesn't support passing >context managers, so we can't simply pass "self". Happily, we need >self only for read-only access, and it just works if it is defined >in global space. So, add a temporary link TestRunner.shared_self >during run_tests(). > > Signed-off-by: Vladimir Sementsov-Ogievskiy Just wondering, is it worth even supporting the mp=false case or can we simplify the code a bit by always going through multiprocessing and using nice directory names even if only one process is spawned? Maybe John's observation that directory names get longer might be a reason not to do that by default. Any other reasons you're aware of? Kevin
Re: [PATCH 2/3] iotests/testrunner.py: move updating last_elapsed to run_tests
Am 06.12.2021 um 18:59 hat John Snow geschrieben:
> On Fri, Dec 3, 2021 at 7:22 AM Vladimir Sementsov-Ogievskiy <
> vsement...@virtuozzo.com> wrote:
>
> > We are going to use do_run_test() in multiprocessing environment, where
> > we'll not be able to change original runner object.
> >
> > Happily, the only thing we change is that last_elapsed and it's simple
> > to do it in run_tests() instead. All other accesses to self in
> > do_runt_test() and in run_test() are read-only.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy
> > ---
> > tests/qemu-iotests/testrunner.py | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/qemu-iotests/testrunner.py
> > b/tests/qemu-iotests/testrunner.py
> > index fa842252d3..a9f2feb58c 100644
> > --- a/tests/qemu-iotests/testrunner.py
> > +++ b/tests/qemu-iotests/testrunner.py
> > @@ -287,7 +287,6 @@ def do_run_test(self, test: str) -> TestResult:
> >diff=diff, casenotrun=casenotrun)
> > else:
> > f_bad.unlink()
> > -self.last_elapsed.update(test, elapsed)
> > return TestResult(status='pass', elapsed=elapsed,
> >casenotrun=casenotrun)
> >
> > @@ -353,6 +352,9 @@ def run_tests(self, tests: List[str]) -> bool:
> > print('\n'.join(res.diff))
> > elif res.status == 'not run':
> > notrun.append(name)
> > +elif res.status == 'pass':
> > +assert res.elapsed is not None
> > +self.last_elapsed.update(t, res.elapsed)
> >
> > sys.stdout.flush()
> > if res.interrupted:
> > --
> > 2.31.1
> >
> >
> (I continue to be annoyed by the "None" problem in mypy, but I suppose it
> really can't be helped. Nothing for you to change with this patch or
> series. I just wish we didn't need so many assertions ...)
I'm inclined to say it's a None problem in our code, not in mypy.
Essentially it comes from the fact that we're abusing a string
(res.status) and None values to distinguish different types of results
that have a different set of valid attributes.
Of course, Python already provides a language feature to distinguish
different types of results that have a different set of attributes and
that wouldn't run into this problem: subclasses.
Kevin
Re: [PATCH v5 04/31] include/sysemu/block-backend: split header into I/O and global state (GS) API
On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote: Similarly to the previous patches, split block-backend.h in block-backend-io.h and block-backend-global-state.h In addition, remove "block/block.h" include as it seems it is not necessary anymore, together with "qemu/iov.h" block-backend-common.h contains the structures shared between the two headers, and the functions that can't be categorized as I/O or global state. Assertions are added in the next patch. Signed-off-by: Emanuele Giuseppe Esposito --- include/sysemu/block-backend-common.h | 84 ++ include/sysemu/block-backend-global-state.h | 121 + include/sysemu/block-backend-io.h | 139 ++ include/sysemu/block-backend.h | 269 +--- block/block-backend.c | 9 +- 5 files changed, 353 insertions(+), 269 deletions(-) create mode 100644 include/sysemu/block-backend-common.h create mode 100644 include/sysemu/block-backend-global-state.h create mode 100644 include/sysemu/block-backend-io.h [...] diff --git a/include/sysemu/block-backend-global-state.h b/include/sysemu/block-backend-global-state.h new file mode 100644 index 00..77009bf7a2 --- /dev/null +++ b/include/sysemu/block-backend-global-state.h [...] +int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf); I don’t quite follow what makes blk_ioctl() a GS function. (It’s possible that I forgot something about this from v4, though, and can’t find it anymore...) I suppose it can be said that in the context of the block layer, ioctl functions are generally used outside of I/O threads as something that kind of affects the global data state, but... bdrv_co_ioctl() is in block-io.h, and internally we definitely do use ioctl in I/O threads (various uses in file-posix.c, e.g. to zero out areas). I also don’t understand why blk_ioctl() is GS, and blk_aio_ioctl() is I/O. And if you have a virtio-scsi device in an iothread, and then create a scsi-generic device on it, will bdrv_co_ioctl() not run in the I/O thread still...? Hm, testing (on patch 6), it appears that not – I think that’s because it mostly uses blk_aio_ioctl(), apart from *_realize() and scsi_SG_IO_FROM_DEV() (whatever that is; and this makes me a bit cautious, too). Still, I’m not quite sure how it’s more global state than e.g. writing zeroes or, well, blk_aio_ioctl(). However, testing brought some other problems to light: $ ls /dev/sg? /dev/sg0 $ sudo modprobe scsi_debug max_luns=1 num_tgts=1 add_host=1 $ ls /dev/sg? /dev/sg0 /dev/sg1 $ ./qemu-system-x86_64 \ -object iothread,id=foo \ -device virtio-scsi,iothread=foo \ -blockdev host_device,filename=/dev/sg1,node-name=bar \ -device scsi-generic,drive=bar qemu-system-x86_64: ../block/block-backend.c:2038: blk_set_guest_block_size: Assertion `qemu_in_main_thread()' failed. [1] 121353 IOT instruction (core dumped) ./qemu-system-x86_64 -object iothread,id=foo -device virtio-scsi,iothread=foo And: $ ./qemu-system-x86_64 \ -object iothread,id=foo \ -device virtio-scsi,iothread=foo \ -blockdev null-co,read-zeroes=true,node-name=bar \ -device scsi-hd,drive=bar \ -cdrom ~/tmp/arch.iso \ -enable-kvm \ -m 1g qemu-system-x86_64: ../block/block-backend.c:1918: blk_enable_write_cache: Assertion `qemu_in_main_thread()' failed. [1] 127203 IOT instruction (core dumped) ./qemu-system-x86_64 -object iothread,id=foo -device virtio-scsi,iothread=foo If I boot from a CD under virtio-scsi (in an iothread), I also get an error for blk_lock_medium(), and if I eject such a CD, for blk_eject() and blk_get_attached_dev_id() (called from blk_eject()). It appears like scsi_disk_emulate_command() is always run in the BB’s AioContext, and so everything it (indirectly) calls cannot assume to be in the main thread. As for blk_set_guest_block_size(), that one’s called from scsi-generic’s scsi_read_complete(). Hanna
[PATCH 1/2] scsi/scsi_bus: use host_status as parameter for scsi_sense_from_host_status()
The scsi_sense_from_host_status() always returns GOOD since
req->host_status is 255 (-1) at this time. Change req->host_status to
host_status so that scsi_sense_from_host_status() will be able to return
the expected value.
Fixes: f3126d65b393("scsi: move host_status handling into SCSI drivers")
Cc: Joe Jin
Signed-off-by: Dongli Zhang
---
hw/scsi/scsi-bus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 77325d8cc7..d46650bd8c 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1465,7 +1465,7 @@ void scsi_req_complete_failed(SCSIRequest *req, int
host_status)
assert(req->ops != _unit_attention);
if (!req->bus->info->fail) {
-status = scsi_sense_from_host_status(req->host_status, );
+status = scsi_sense_from_host_status(host_status, );
if (status == CHECK_CONDITION) {
scsi_req_build_sense(req, sense);
}
--
2.17.1
[PATCH 2/2] scsi/utils: pass host_status = SCSI_HOST_ERROR to guest kernel
For scsi_req_complete_failed() and when the req->bus->info->fail() is implemented, the virtio-scsi passes SCSI_HOST_ERROR to the guest kernel as VIRTIO_SCSI_S_FAILURE, while the pvscsi passes SCSI_HOST_ERROR to guest kernel as BTSTAT_HASOFTWARE. However, the scsi_req_complete_failed()->scsi_sense_from_host_status() always returns GOOD for SCSI_HOST_ERROR, when the req->bus->info->fail() is not implemented (e.g., megasas). As a result, the sense is not passed to the guest kernel. The SCSI_HOST_ERROR is reproduced on purpose by below QEMU command line: -device megasas,id=vscsi0,bus=pci.0,addr=0x4 \ -drive file=/dev/sdc,format=raw,if=none,id=drive01 \ -device scsi-block,bus=vscsi0.0,channel=0,scsi-id=0,lun=0,drive=drive01 \ ... and when we remove the /dev/sdc from the host with: "echo 1 > /sys/block/sdc/device/delete" This patch passes sense_code_IO_ERROR to the guest kernel for host_status = SCSI_HOST_ERROR. (This issue is detected by running a testing code from Rui Loura). Cc: Joe Jin Cc: Adnan Misherfi Cc: Rui Loura Signed-off-by: Dongli Zhang --- scsi/utils.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scsi/utils.c b/scsi/utils.c index 357b036671..086a1fea66 100644 --- a/scsi/utils.c +++ b/scsi/utils.c @@ -638,6 +638,9 @@ int scsi_sense_from_host_status(uint8_t host_status, case SCSI_HOST_ABORTED: *sense = SENSE_CODE(COMMAND_ABORTED); return CHECK_CONDITION; +case SCSI_HOST_ERROR: +*sense = SENSE_CODE(IO_ERROR); +return CHECK_CONDITION; case SCSI_HOST_RESET: *sense = SENSE_CODE(RESET); return CHECK_CONDITION; -- 2.17.1
[PATCH 0/2] scsi: to fix issue on passing host_status to the guest kernel
This patchset fixes the issue on passing 'host_status' to the guest kernel. The 1st patch fixes the erroneous usage of req->host_status. The 2nd patch is to pass the SCSI_HOST_ERROR to the guest kernel when the req->bus->info->fail() is not implemented. I do not add 'Fixes:' because I am not sure if to not pass SCSI_HOST_ERROR was on purpose, especially for security reason. Thank you very much! Dongli Zhang
Re: [PATCH 1/3] iotests/testrunner.py: add doc string for run_test()
Am 03.12.2021 um 13:22 hat Vladimir Sementsov-Ogievskiy geschrieben:
> We are going to modify these methods and will add more documentation in
> further commit. As a preparation add basic documentation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> tests/qemu-iotests/testrunner.py | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/tests/qemu-iotests/testrunner.py
> b/tests/qemu-iotests/testrunner.py
> index 0e29c2fddd..fa842252d3 100644
> --- a/tests/qemu-iotests/testrunner.py
> +++ b/tests/qemu-iotests/testrunner.py
> @@ -220,6 +220,12 @@ def find_reference(self, test: str) -> str:
> return f'{test}.out'
>
> def do_run_test(self, test: str) -> TestResult:
> +"""
> +Run one test
> +
> +:param test: test file path
> +"""
> +
> f_test = Path(test)
> f_bad = Path(f_test.name + '.out.bad')
> f_notrun = Path(f_test.name + '.notrun')
> @@ -287,6 +293,13 @@ def do_run_test(self, test: str) -> TestResult:
>
> def run_test(self, test: str,
> test_field_width: Optional[int] = None) -> TestResult:
> +"""
> +Run one test and print short status
> +
> +:param test: test file path
> +:param test_field_width: width for first field of status format
> +"""
> +
> last_el = self.last_elapsed.get(test)
> start = datetime.datetime.now().strftime('%H:%M:%S')
test_field_width is Optional[int], so the documentation should specify
what happens when you pass None.
Seems it doesn't change the behaviour, but just picks a default value of
8. 'test_field_width: int = 8' might have been the more obvious solution
for this in the first place.
Kevin
Re: [RFC] block-backend: prevent dangling BDS pointer in blk_drain()
Am 09.12.2021 um 15:23 hat Stefan Hajnoczi geschrieben: > The BlockBackend root child can change during bdrv_drained_begin() when > aio_poll() is invoked. In fact the BlockDriverState can reach refcnt 0 > and blk_drain() is left with a dangling BDS pointer. > > One example is scsi_device_purge_requests(), which calls blk_drain() to > wait for in-flight requests to cancel. If the backup blockjob is active, > then the BlockBackend root child is a temporary filter BDS owned by the > blockjob. The blockjob can complete during bdrv_drained_begin() and the > last reference to the BDS is released when the temporary filter node is > removed. This results in a use-after-free when blk_drain() calls > bdrv_drained_end(bs) on the dangling pointer. > > The general problem is that a function and its callers must not assume > that bs is still valid across aio_poll(). Explicitly hold a reference to > bs in blk_drain() to avoid the dangling pointer. > > Signed-off-by: Stefan Hajnoczi > --- > I found that BDS nodes are sometimes deleted with bs->quiesce_counter > > 0 (at least when running "make check"), so it is currently not possible > to put the bdrv_ref/unref() calls in bdrv_do_drained_begin() and > bdrv_do_drained_end() because they will be unbalanced. That would have > been a more general solution than only fixing blk_drain(). They are not supposed to end up unbalanced because detaching a child calls bdrv_unapply_subtree_drain(). In fact, I think test-bdrv-drain tests a few scenarios like this. Do have more details about the case that failed for you? Kevin
Re: [PATCH v4 0/3] hw/block/fdc: Fix CVE-2021-20196
Am 24.11.2021 um 17:15 hat Philippe Mathieu-Daudé geschrieben: > Since v3: > - Preliminary extract blk_create_empty_drive() > - qtest checks qtest_check_clang_sanitizer() enabled > - qtest uses null-co:// driver instead of file > > Philippe Mathieu-Daudé (3): > hw/block/fdc: Extract blk_create_empty_drive() > hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196 > tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 If I may ask a meta question: No doubt that this is a bug and it's good that we fixed it, but why was it assigned a CVE? Any guest can legitimately shut down and we don't consider that a denial of service. This bug was essentially just another undocumented way for the guest kernel to shut down, as unprivileged users in the guest can't normally access the I/O ports of the floppy controller. I don't think we generally consider guests killing themselves a security problem as long as it requires kernel or root privileges in the guest. Kevin
Re: [PATCH 2/2] iotests/149: Skip on unsupported ciphers
Am 17.11.2021 um 16:05 hat Hanna Reitz geschrieben:
> On 17.11.21 16:01, Hanna Reitz wrote:
> > Whenever qemu-img or qemu-io report that some cipher is unsupported,
> > skip the whole test, because that is probably because qemu has been
> > configured with the gnutls crypto backend.
> >
> > We could taylor the algorithm list to what gnutls supports, but this is
> > a test that is run rather rarely anyway (because it requires
> > password-less sudo), and so it seems better and easier to skip it. When
> > this test is intentionally run to check LUKS compatibility, it seems
> > better not to limit the algorithms but keep the list extensive.
> >
> > Signed-off-by: Hanna Reitz
> > ---
> > tests/qemu-iotests/149 | 23 ++-
> > 1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
> > index 328fd05a4c..adcef86e88 100755
> > --- a/tests/qemu-iotests/149
> > +++ b/tests/qemu-iotests/149
> > @@ -230,6 +230,18 @@ def create_image(config, size_mb):
> > fn.truncate(size_mb * 1024 * 1024)
> > +def check_cipher_support(output):
> > +"""Check the output of qemu-img or qemu-io for mention of the
> > respective
> > +cipher algorithm being unsupported, and if so, skip this test.
> > +(Returns `output` for convenience.)"""
> > +
> > +if 'Unsupported cipher algorithm' in output:
> > +iotests.notrun('Unsupported cipher algorithm '
> > + f'{config.cipher}-{config.keylen}-{config.mode}; '
>
> Oops. Just when I sent this I realized that during refactoring (putting
> this code into its own function) I forgot to pass `config` as a parameter.
>
> Didn’t notice that because... It seems to work just fine despite `config`
> not being defined here? Python will forever remain a black box for me...
This is an old thread by now, but I think that it works is just because
it's defined as a global variable ('for config in configs') before
calling this function.
Kevin
Re: [PATCH 3/4] Move CONFIG_XFS handling to meson.build
On 12/10/21 09:46, Thomas Huth wrote: platform_test_xfs_fd() is only used to decide whether to invoke XFS_IOC_DIOINFO; but failures of XFS_IOC_DIOINFO are ignored anyway, so we can get rid of is_xfs in BDRVRawState, too. After staring at the code for a while, I wonder why we're not simply using fstat() here instead to get the st_blksize value... wouldn't that be better anyway since it also works with other file system types? The value that XFS_IOC_DIOINFO returns is the logical sector size of the underlying device; it should be 512 or 4096, but more likely 512. It can be smaller than st_blksize, because often it will be if it is 512 but the st_blksize is usually 4096. If it is wrong, QEMU will do unnecessary read/modify/write operations for disk writes that are not 4K-aligned. Paolo
Re: [PATCH 3/4] Move CONFIG_XFS handling to meson.build
On 10/12/2021 09.39, Paolo Bonzini wrote:
On 12/10/21 08:53, Thomas Huth wrote:
On 02/11/2021 12.34, Paolo Bonzini wrote:
On 28/10/21 20:59, Thomas Huth wrote:
Checking for xfsctl() can be done more easily in meson.build. Also,
this is not a "real" feature like the other features that we provide
with the "--enable-xxx" and "--disable-xxx" switches for the
configure script, since this does not influence lots of code (it's
only about one call to xfsctl() in file-posix.c), so people don't
gain much with the ability to disable this with "--disable-xfsctl".
Let's rather treat this like the other cc.has_function() checks in
meson.build, i.e. don't add a new option for this in meson_options.txt.
Signed-off-by: Thomas Huth
I think we should just use ioctl and copy the relevant definitions from
Linux:
struct dioattr {
u32 d_mem; /* data buffer memory alignment */
u32 d_miniosz; /* min xfer size */
u32 d_maxiosz; /* max xfer size */
};
#define XFS_IOC_DIOINFO _IOR ('X', 30, struct dioattr)
I've now had a closer look at this idea, but it's getting messy: We'd
additionally also need the platform_test_xfs_fd() function that is called
from file-posix.c ...
platform_test_xfs_fd() is only used to decide whether to invoke
XFS_IOC_DIOINFO; but failures of XFS_IOC_DIOINFO are ignored anyway, so we
can get rid of is_xfs in BDRVRawState, too.
After staring at the code for a while, I wonder why we're not simply using
fstat() here instead to get the st_blksize value... wouldn't that be better
anyway since it also works with other file system types?
Thomas
Re: [PATCH 3/4] Move CONFIG_XFS handling to meson.build
On 12/10/21 08:53, Thomas Huth wrote:
On 02/11/2021 12.34, Paolo Bonzini wrote:
On 28/10/21 20:59, Thomas Huth wrote:
Checking for xfsctl() can be done more easily in meson.build. Also,
this is not a "real" feature like the other features that we provide
with the "--enable-xxx" and "--disable-xxx" switches for the
configure script, since this does not influence lots of code (it's
only about one call to xfsctl() in file-posix.c), so people don't
gain much with the ability to disable this with "--disable-xfsctl".
Let's rather treat this like the other cc.has_function() checks in
meson.build, i.e. don't add a new option for this in meson_options.txt.
Signed-off-by: Thomas Huth
I think we should just use ioctl and copy the relevant definitions
from Linux:
struct dioattr {
u32 d_mem; /* data buffer memory alignment */
u32 d_miniosz; /* min xfer size */
u32 d_maxiosz; /* max xfer size */
};
#define XFS_IOC_DIOINFO _IOR ('X', 30, struct dioattr)
I've now had a closer look at this idea, but it's getting messy: We'd
additionally also need the platform_test_xfs_fd() function that is
called from file-posix.c ...
platform_test_xfs_fd() is only used to decide whether to invoke
XFS_IOC_DIOINFO; but failures of XFS_IOC_DIOINFO are ignored anyway, so
we can get rid of is_xfs in BDRVRawState, too.
Paolo
Re: [Libguestfs] [libnbd PATCH 00/13] libnbd patches for NBD_OPT_EXTENDED_HEADERS
On 12/04/21 00:17, Eric Blake wrote: > Available here: > https://repo.or.cz/libnbd/ericb.git/shortlog/refs/tags/exthdr-v1 > > I also want to do followup patches to teach 'nbdinfo --map' and > 'nbdcopy' to utilize 64-bit extents. > > Eric Blake (13): > golang: Simplify nbd_block_status callback array copy > block_status: Refactor array storage > protocol: Add definitions for extended headers > protocol: Prepare to send 64-bit requests > protocol: Prepare to receive 64-bit replies > protocol: Accept 64-bit holes during pread > generator: Add struct nbd_extent in prep for 64-bit extents > block_status: Track 64-bit extents internally > block_status: Accept 64-bit extents during block status > api: Add [aio_]nbd_block_status_64 > api: Add three functions for controlling extended headers > generator: Actually request extended headers > interop: Add test of 64-bit block status > > lib/internal.h| 31 ++- > lib/nbd-protocol.h| 61 - > generator/API.ml | 237 -- > generator/API.mli | 3 +- > generator/C.ml| 24 +- > generator/GoLang.ml | 35 ++- > generator/Makefile.am | 3 +- > generator/OCaml.ml| 20 +- > generator/Python.ml | 29 ++- > generator/state_machine.ml| 52 +++- > generator/states-issue-command.c | 31 ++- > .../states-newstyle-opt-extended-headers.c| 90 +++ > generator/states-newstyle-opt-starttls.c | 10 +- > generator/states-reply-structured.c | 220 > generator/states-reply.c | 31 ++- > lib/handle.c | 27 +- > lib/rw.c | 105 +++- > python/t/110-defaults.py | 3 +- > python/t/120-set-non-defaults.py | 4 +- > python/t/465-block-status-64.py | 56 + > ocaml/helpers.c | 22 +- > ocaml/nbd-c.h | 3 +- > ocaml/tests/Makefile.am | 5 +- > ocaml/tests/test_110_defaults.ml | 4 +- > ocaml/tests/test_120_set_non_defaults.ml | 5 +- > ocaml/tests/test_465_block_status_64.ml | 58 + > tests/meta-base-allocation.c | 111 +++- > interop/Makefile.am | 6 + > interop/large-status.c| 186 ++ > interop/large-status.sh | 49 > .gitignore| 1 + > golang/Makefile.am| 3 +- > golang/handle.go | 6 + > golang/libnbd_110_defaults_test.go| 8 + > golang/libnbd_120_set_non_defaults_test.go| 12 + > golang/libnbd_465_block_status_64_test.go | 119 + > 36 files changed, 1511 insertions(+), 159 deletions(-) > create mode 100644 generator/states-newstyle-opt-extended-headers.c > create mode 100644 python/t/465-block-status-64.py > create mode 100644 ocaml/tests/test_465_block_status_64.ml > create mode 100644 interop/large-status.c > create mode 100755 interop/large-status.sh > create mode 100644 golang/libnbd_465_block_status_64_test.go > I figured I should slowly / gradually review this series, and as a *pre-requisite* for it, first apply the spec patch, and then read through the spec with something like $ git show --color -U1000 In other words, read the whole spec, just highlight the new additions. Now, I see Vladimir has made several comments on the spec patch; will those comments necessitate a respin of the libnbd series? If so, how intrusive are the changes going to be? I'm hesitant to start my review if significant changes are already foreseen. Thanks, Laszlo
