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;
> +}
