search

xen-troops / displ_be

GNU General Public License v2.0
2 stars 8 forks source link

class CtrlRingBuffer: input parameters #118

Closed iusyk closed 3 years ago

iusyk commented 4 years ago

Description

There are 2 issues with the ctor of class CtrlRingBuffer

  1. The input parameters are the pointers, but they are not checked for a nullptr, what brings the risk of unexpected crash
  2. Parameter 'connector' must be removed The reason of this is because connector can be used just when it is initialized, but the ctor takes always not initialized input parameter, see the code auto width = stoi(resolution.substr(0, pos)); auto height = stoi(resolution.substr(++pos)); auto connector = mDisplay->createConnector(getDomId(), id, width, height); CtrlRingBufferPtr ctrlRingBuffer( new CtrlRingBuffer(mDisplay, connector, bufferStorage, eventRingBuffer, getDomId(), port, ref)); There is not any sense to create the not initialized connector from mDisplay, and put both parameters in the ctor of CtrlRingBuffer. This behavior just increase the risk of the exceptions (what can be avoided) and crashes(pointers are not checked for the nullptr), besides - all code to work with connector must be wrapped in try{}catch{} block or it is necessary to , because connector is not initialized to check the connector state.

Solution

  1. Remove input parameter 'connector'
  2. Add the pointers validation in ctor of CtrlRingBuffer
  3. Create the conenctor just when it is necessary, in our case this is DisplayCommandHandler::setConfig(const xendispl_req& req, xendispl_resp& rsp) Because, CtrlRingBuffer has the member mCommandHandler,
  4. Remove input parameter 'connector' from the ctor declaration of DisplayCommandHandler
  5. Instead of if([connector->]isInitialilised) add if(connector) where required. (and totaly remove from connector implementaiton)
iusyk commented 4 years ago

fix https://github.com/xen-troops/displ_be/pull/132 fixed partly, please, read the explanation in pull request.

iusyk commented 3 years ago

fix #132 is merged