On Tue, 18 Nov 2025 at 14:58, Cédric Le Goater <[email protected]> wrote:
>
> On 11/17/25 16:56, John Levon wrote:
> > This function was unnecessarily difficult to understand due to the
> > separate handling of request and reply messages. Use common code for
> > both where we can.
>
> It's still difficult to read :) Could we have feedback from Thanos or Mark ?
FWIW, when I suggested a restructure of this (because it popped
up in a false positive Coverity warning because Coverity couldn't
figure out the logic in it) the shape of code I had in mind
was something like
static int vfio_user_recv_one(VFIOUserProxy *proxy, Error **errp)
{
/* common prep, header read, etc code here */
switch (hdr.flags & VFIO_USER_TYPE) {
case VFIO_USER_REQUEST:
return vfio_user_handle_request(...stuff...);
break;
case VFIO_USER_REPLY:
return vfio_user_handle_request(...stuff...);
break;
default:
error_setg(errp, "unknown message type");
goto fatal;
}
}
and then use a shared helper function for the
"read rest of message" logic which is the only other
part the request and reply codepaths seem to share
once they diverge.
In particular this means that the error-exit logic
doesn't have to have "if (isreply)" checks in it,
because the error-exit path in _handle_reply only
deals with tidying up the reply path, and similarly
for the path in _handle_request. This is the kind of
"codepaths diverge and then rejoin and then diverge
again" flow that Coverity (and I think human readers)
tends to have trouble with.
There are some fiddly loose ends here so it's not quite
as neat as the above pseudocode sketch (e.g. maybe the
handle_* functions need to return a value and
vfio_user_recv_one does some cleanup on failure too).
But anyway, that was the thing I had in my head.
thanks
-- PMM
