On 09/10/2017 19:27, Vladimir Sementsov-Ogievskiy wrote:
+ int ret = nbd_co_do_receive_one_chunk(s, handle, only_structured,
+ &request_ret, qiov, payload);
+
+ if (ret < 0) {
+ s->quit = true;
+ } else {
+ /* For assert at loop start in nbd_read_reply_entry */
+ if (reply) {
+ *reply = s->reply;
+ }
reply is always non-NULL, right?
+ s->reply.handle = 0;
+ ret = request_ret;
+ }
...
+static int nbd_co_receive_return_code(NBDClientSession *s, uint64_t handle)
+{
+ int ret = 0;
+ int i = HANDLE_TO_INDEX(s, handle);
+ NBDReply reply;
+ bool only_structured = false;
+
+ do {
+ int rc = nbd_co_receive_one_chunk(s, handle, only_structured,
+ NULL, &reply, NULL);
Is rc > 0 propagated correctly?
+ if (rc < 0 || nbd_reply_is_simple(&reply)) {
+ if (ret == 0) {
+ ret = rc;
+ }
+ only_structured = true;
+ continue;
+ }
+ } while (!s->quit && nbd_reply_is_structured(&s->reply) &&
+ !(s->reply.structured.flags & NBD_SREP_FLAG_DONE));
+
+ s->requests[i].coroutine = NULL;
+
+ qemu_co_mutex_lock(&s->send_mutex);
+ s->in_flight--;
+ qemu_co_queue_next(&s->free_sema);
+ qemu_co_mutex_unlock(&s->send_mutex);
The code after the loop is duplicated.
An idea could be to wrap nbd_co_receive_one_chunk with an iterator like
typedef struct NBDReplyChunkIter {
int ret;
bool done, only_structured;
} NBDReplyChunkIter;
#define NBD_FOREACH_REPLY_CHUNK(s, handle, iter, qiov, reply, payload, \
structured) \
for (iter = (NBDReplyChunkIter) { .only_structured = structured }; \
nbd_reply_chunk_iter_receive(s, &iter, qiov, &reply, handle, \
payload);;)
bool coroutine_fn nbd_reply_chunk_iter_receive(...)
{
if (s->quit) {
if (iter->ret == 0) {
iter->ret = -EIO;
}
goto break_loop;
}
/* Handle the end of the previous iteration. */
if (iter->done) {
goto break_loop;
}
rc = nbd_co_receive_one_chunk(s, handle, iter->only_structured,
qiov, reply, payload);
if (rc < 0) {
assert(s->quit);
if (iter->ret == 0) {
iter->ret = rc;
}
goto break_loop;
}
/* No structured reply? Do not execute the body of
* NBD_FOREACH_REPLY_CHUNK.
*/
if (nbd_reply_is_simple(&s->reply) || s->quit) {
goto break_loop;
}
iter->only_structured = true;
if (s->reply.structured.flags & NBD_SREP_FLAG_DONE) {
iter->ret = rc;
iter->done = true;
/* Execute the loop body, but this iteration is the last. */
return true;
}
/* An error reply must have NBD_SREP_FLAG_DONE set, otherwise it is
* a protocol error (FIXME: is this true? but if not, how do you
* cope with multiple error replies?)
*/
if (rc > 0) {
s->quit = true;
iter->ret = -EIO;
goto break_loop;
}
return true;
break_loop:
iter->done = true;
s->requests[HANDLE_TO_INDEX(s, handle)].coroutine = NULL;
qemu_co_mutex_lock(&s->send_mutex);
s->in_flight--;
qemu_co_queue_next(&s->free_sema);
qemu_co_mutex_unlock(&s->send_mutex);
return false;
}
so this loop could be written like
FOR_EACH_REPLY_CHUNK(s, handle, iter, NULL, &reply, NULL) {
if (reply.structured.type != NBD_SREP_TYPE_NONE) {
/* not allowed reply type */
s->quit = true;
break;
}
}
return iter.ret;
and likewise for nbd_co_receive_cmdread_reply. The iterator is a bit
more complex, but it abstracts all the protocol handling and avoids lots
of duplicated code.
Paolo
+ return ret;
+}