Many API in NFC daemon now only return boolean to represent success or fail. It would be better return error code instead of boolean

Hi, I understand the error codes will be the ones defined in Bug 897312. https://bugzilla.mozilla.org/attachment.cgi?id=831587

Hi Yoshi. This is draft version of using error code, could you help check if you think the error code provided in this version is ok or you think nfcd should provide more detail error code. Thanks!

Comment on attachment 8415161 [details] [diff] [review] WIP Patch v1 Review of attachment 8415161 [details] [diff] [review]: ----------------------------------------------------------------- ::: src/NfcGonkMessage.h @@ +55,5 @@ > + NFC_ERROR_INITIALIZE_FAIL = 8, > + NFC_ERROR_DEINITIALIZE_FAIL = 9, > + NFC_ERROR_COULD_NOT_ENABLE_DISCOVERY = 10, > + NFC_ERROR_COULD_NOT_DISABLE_DISCOVERY = 11, > + NFC_ERROR_COULD_NOT_CONNECAT_TAG = 12, CONNECT @@ +57,5 @@ > + NFC_ERROR_COULD_NOT_ENABLE_DISCOVERY = 10, > + NFC_ERROR_COULD_NOT_DISABLE_DISCOVERY = 11, > + NFC_ERROR_COULD_NOT_CONNECAT_TAG = 12, > + NFC_ERROR_COULD_NOT_WRITE_TAG = 13, > + NFC_ERROR_COULD_NOT_MAKE_TAG_READ_ONLY = 14 Bug 897312 has already defined some error code, can we reuse them? ::: src/NfcService.cpp @@ +65,5 @@ > static sem_t thread_sem; > > +#define CHECK_ERR_RTN(sta, err) \ > + if (sta) { \ > + mMsgHandler->processResponse(res, err, NULL); \ Not sure if this is a good macro, 1. We need to define 'res' when calling this macro, and the naming in Mozilla is usually .._ENSURE_... 2. Also usually we need to align \. 3. Usually we need to wrap it with do{} while(0) so caller can add semicolon and can prevent if (foo) CHECK_ERR_RTN else ... proble, ::: src/NfcUtil.h @@ +10,5 @@ > #include "NfcGonkMessage.h" > #include "TagTechnology.h" > > +#define SUCCESSED(result) (result == NFC_SUCCESS) > +#define FAILED(result) (result != NFC_SUCCESS) These two macros doesn't bring much advantage. If you'd like to use it, seems one is enough. SUCCEED(result) ((result) == NFC_SUCCESS)

(In reply to Yoshi Huang[:allstars.chh] from comment #3) > Comment on attachment 8415161 [details] [diff] [review] > WIP Patch v1 > > Review of attachment 8415161 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +57,5 @@ > > + NFC_ERROR_COULD_NOT_ENABLE_DISCOVERY = 10, > > + NFC_ERROR_COULD_NOT_DISABLE_DISCOVERY = 11, > > + NFC_ERROR_COULD_NOT_CONNECAT_TAG = 12, > > + NFC_ERROR_COULD_NOT_WRITE_TAG = 13, > > + NFC_ERROR_COULD_NOT_MAKE_TAG_READ_ONLY = 14 > > Bug 897312 has already defined some error code, can we reuse them? > I will re-use them in next version but still i'll add some more error code because this is not sufficient.

Following Error code is modified: NFC_ERROR_NFC_OFF = ‐18, NFC_ERROR_NFC_DISCOVERY_ON = ‐19, NFC_ERROR_NFC_DISCOVERY_OFF = ‐20, NFC_ERROR_NFC_ENABLE_DISCOVERY = ‐21, NFC_ERROR_NFC_DISABLE_DISCOVERY = ‐22, Most of the error code value in (v1.9) Nfcd/Gonk Protocol Specification is also changed.

Comment on attachment 8417222 [details] NFCD use error code patch v1 see comments on the github

1.Update yoshi's review comment 2.Update following error code definition * NFC_ERROR_NFC_ON -> NFC_ERROR_NFC_ALREADY_ON * NFC_ERROR_NFC_OFF -> NFC_ERROR_NFC_ALREADY_OFF * Add NFC_ERROR_ALREADY_DISCOVERY_ON * Add NFC_ERROR_ALREADY_DISCOVERY_OFF * Add NFC_ERROR_INITIALIZE_FAIL * Add NFC_ERROR_DEINITIALIZE_FAIL

Comment on attachment 8417298 [details] NFCD use error code patch v2 basically ok, but I'd like to make code shorter and review this again.

Update code according to yoshi's suggestion

Comment on attachment 8417855 [details] NFCD use error code patch v3 see github for some nits

Hi Yoshi, I have made some modification for Patch v3 according to suggestion, could you help review again ? Thanks. If you are ok with this, i will create another pull request