[XrdCl] Refactor kXR_status response handling. #1854
Conversation
|
Should this be rebased/merged soon? Is this for 5.5.3 or for 5.6? |
|
@amadio : actually now would be the perfect time to merge it, could you have a look and do a quick review? |
| //---------------------------------------------------------------- | ||
| // The next step is to finalize the read | ||
| //---------------------------------------------------------------- | ||
| readstage = ReadDone; |
There was a problem hiding this comment.
Can ReadMore be required more than once?
There was a problem hiding this comment.
What I wonder is if setting readstage = ReadDone will not prevent ReadMore from being called twice in a row if needed.
| return st; | ||
| } | ||
|
|
||
| if( !st.IsOK() ) |
There was a problem hiding this comment.
Why do you need to test twice for !st.IsOK()? Could these two ifs not be joined into a single one?
There was a problem hiding this comment.
well, the first one is a special case where we have corrupted data, the other one handles all other errors, the only difference is the format of the error message (I wanted to preserve the original log message structure)
There was a problem hiding this comment.
Sure, but you can have an if ( !st.IsOK() ), and inside it have another if (or switch) to treat differently the different kinds of errors. But I guess this is not so performance critical, so it's not the end of the world if we check twice.
| // Do the integrity checks | ||
| //-------------------------------------------------------------------------- | ||
| if( crcval != cse->cseCRC ) | ||
| switch( reqType ) |
There was a problem hiding this comment.
Why a switch with a single case inside? Do you intend this to be expanded with more cases later on?
There was a problem hiding this comment.
yes, exactly! right now kXR_status is used only for 'pgread'/'pgwrite' but in the future it could be used for other requests hence the switch statement.
Move kXR_status response handling to
AsyncMsgReader.