[Asterisk-code-review] cdr/cdr csv.c: Refactor funcion to write on csv file. (asterisk[master])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: cdr/cdr_csv.c: Refactor funcion to write on csv file.
......................................................................


Patch Set 1: Code-Review-1

(2 comments)

https://gerrit.asterisk.org/#/c/319/1//COMMIT_MSG
Commit Message:

Line 7: cdr/cdr_csv.c: Refactor funcion to write on csv file.
      : 
      : Change-Id: Idce707f4c108083252e0aeb948f421d924953e65
While I appreciate the patches you've done for the CDR backends, you are going to need to start writing more complete commit messages. Having a record of what a patch does, its purpose, and why it was done is incredibly important to the maintainability of the Asterisk project. Without good, complete, and descriptive commit messages, maintainers of the Asterisk project would have an incredibly difficult time in determining why changes were made.

We're in this for the long haul. We need your commits to reflect that.

Please starting taking some time in writing what your patch does. There are some great guidelines on how to write a good commit message out there. For starters, please take a look at the Asterisk wiki:

https://wiki.asterisk.org/wiki/display/AST/Commit+Messages

The OpenStack project has also written a fantastic guide on how to write good commit messages:

https://wiki.openstack.org/wiki/GitCommitMessages#Information_in_commit_messages


https://gerrit.asterisk.org/#/c/319/1/cdr/cdr_csv.c
File cdr/cdr_csv.c:

Line 296: 	if (writefile(buf, csvmaster))
        : 		ast_log(LOG_WARNING, "Unable to write CSV record to master '%s' : %s\n", csvmaster, strerror(errno));
        : 
        : 	if (accountlogs && !ast_strlen_zero(cdr->accountcode)) {
        : 		if (writefile_account(buf, cdr->accountcode))
        : 			ast_log(LOG_WARNING, "Unable to write CSV record to account file '%s' : %s\n", cdr->accountcode, strerror(errno));
        : 	}
I don't think this is how I would refactor these functions.

You've taken the following pattern:

lock(mf_lock)
write to main csv, protected by mf_lock
unlock(mf_lock)
lock(acf_lock)
write to accountcode csv, protected by acf_lock
unlock(acf_lock)

And instead changed it to this:

lock(mf_lock)
write to main csv
unlock(mf_lock)
lock(mf_lock)
write to accountcode csv
unlock(mf_lock)

I'm not sure how that's much better than what is here. Ideally, the pattern would be:

lock(mf_lock)
perform all CDR writes
unlock(mf_lock)

That would generally avoid multiple locks/unlocks that this patch introduces.


-- 
To view, visit https://gerrit.asterisk.org/319
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idce707f4c108083252e0aeb948f421d924953e65
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Rodrigo Ramirez Norambuena <decipher.hk at gmail.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Rodrigo Ramirez Norambuena <decipher.hk at gmail.com>
Gerrit-HasComments: Yes