Jason Parker's review
--------------------------------------

trunk/Makefile (Diff revision 1)

135 	

  ASTETCDIR=$(sysconfdir)/asterisk

135 	

  ASTETCDIR=/usr/local/etc/asterisk

    bad, bad, bad

trunk/apps/app_voicemail.c (Diff revision 1)

243 	

	OPT_DTMFEXIT =         (1 << 7),

243 	

	OPT_MESSAGE_URGENT =   (1 << 7),

    Seems a little silly to renumber these for no apparent reason.

trunk/apps/app_voicemail.c (Diff revision 1)

1591 	

	/* Find all .txt files - even if they are not in sequence from 0000 */

1603 	

	/* Find all .txt files - even if they are not in sequence from, int urgent 0000 */

    O.o

trunk/apps/app_voicemail.c (Diff revision 1)

1605 	

	int vmcount = 0, is_urgent;

    No need for this variable.

trunk/apps/app_voicemail.c (Diff revision 1)

3087 	

			snprintf(arguments, sizeof(arguments), "%s %s %s %d %s&", externnotify, context, extension, newvoicemails, flag ? flag : "");

    S_OR(flag, "")

trunk/apps/app_voicemail.c (Diff revision 1)
	

3150 	

	if (ast_test_flag(options, OPT_MESSAGE_URGENT)) {
		ast_copy_string(flag, "URGENT", sizeof(flag));
	} else if (ast_test_flag(options, OPT_MESSAGE_PRIORITY)) {
		ast_copy_string(flag, "PRIORITY", sizeof(flag));
	} else {
		flag[0] = '\0';
	}
	

    flag (why is it called flag?) (why is it a char?) is used in many places, such as format strings.  All of those places would look odd if flag is 0 length.

trunk/apps/app_voicemail.c (Diff revision 1)

3431 	

				category ? category : "");

    S_OR

trunk/apps/app_voicemail.c (Diff revision 1)

4839 	

	char flag[10];

    Wasn't it char [80] earlier?

trunk/include/asterisk/app.h (Diff revision 1)

115 	

/*! \brief Determine number of new/old messages in a mailbox */

115 	

/*! Determine number of urgent/new/old messages in a mailbox */

    removes \brief?

trunk/main/manager.c (Diff revision 1)
	
2053 	

"	UrgMessages: <count>\n"

    No reason not to spell it out
    
trunk/apps/app_voicemail.c (Diff revision 1)
	

244 	

	OPT_MESSAGE_PRIORITY = (1 << 8)

    Is this just not implemented?
-------------------------------------------
Mark Michelson's review:
------------------------------------------------

trunk/apps/app_voicemail.c (Diff revision 1)

2045 	

		fprintf(p, "Subject: New %s message %d in mailbox %s" ENDL, flag, msgnum + 1, mailbox);

    Minor quibble. A non-urgent message will have two spaces between "New" and "Message"

trunk/apps/app_voicemail.c (Diff revision 1)

2542 	

	if (flag != NULL && !strcmp(flag,"URGENT")) {

    Should use !ast_strlen_zero(flag) instead of checking if it   is not NULL

trunk/apps/app_voicemail.c (Diff revision 1)
	

2890 	

static int check_if_urgent(char *dir, char *filename)

    This whole function should be replaced using the ast_config API. This seems like a reinvention of the wheel to me.

trunk/apps/app_voicemail.c (Diff revision 1)	

2892 	

	unsigned char buf[256]; 

    Why unsigned? Every time buf is used in this function it has to be cast to a char *...

trunk/apps/app_voicemail.c (Diff revision 1)
	

2899 	

	ast_debug(1, "About to open file %s\n", path);

    I think either this debug comment should be removed or at least be at a higher level.

trunk/apps/app_voicemail.c (Diff revision 1)
	

2912 	

					ast_debug(1, "** buf is set to %s\n", buf);

    Same with this debug statement as well

trunk/apps/app_voicemail.c (Diff revision 1)

2960 	

		if (!strncasecmp(de->d_name, "msg", 3)) {
			if (shortcircuit) {
				ret = 1;
				break;
			} else if (!strncasecmp(de->d_name + 8, "txt", 3)) {
				if (is_urgent == urgent) {	/* count urgent or non-urgent as specified */
					ret++;
				}
			}
		}

    This is problematic because if shortcirtcuit is set, then you will not accurately report if there are urgent messages. The function will return true as long as there's a message in the INBOX.


trunk/apps/app_voicemail.c (Diff revision 1)

3087 	

			snprintf(arguments, sizeof(arguments), "%s %s %s %d %s&", externnotify, context, extension, newvoicemails, flag ? flag : "");

    This concerns me somewhat since I don't know if this will break whatever it is people use for externnotify.

trunk/apps/app_voicemail.c (Diff revision 1)	

4894 	

	/* Play the word urgent if we are listening to urgent messages */
	if (flag && strncmp(flag,"URGENT",6) == 0) {
		res = wait_file2(chan, vms, "vm-Urgent");	/* "urgent" */
	}

    While it doesn't hurt to just do a string comparison of the first 6 characters, it isn't necessary. Since flag was set using ast_variable_retrieve, you don't have to worry about the trailing newline.

    The same goes for the comparison a few lines down.

trunk/apps/app_voicemail.c (Diff revision 1)

5207 	

	if (box == NEW_FOLDER && urgent == 1) {
		pgm->unseen = 1;
		pgm->seen = 0;
		pgm->flagged = 1;
		pgm->unflagged = 0;
	} else if (box == NEW_FOLDER && urgent == 0) {
		pgm->unseen = 1;
		pgm->seen = 0;
		pgm->flagged = 0;
		pgm->unflagged = 1;
	} else if (box == OLD_FOLDER) {
		pgm->seen = 1;
		pgm->unseen = 0;
	}

    This section of code makes the assumption that there would be no urgent messages in the "old" folder. Also, it assumes that any flagged message is an urgent message. I guess the second assumption is valid for now, but if any further flags are added later, it would require modification.

trunk/apps/app_voicemail.c (Diff revision 1)
	
2718 	

		/* look for urgent messages */
		if (fold == 2) {
                	pgm->flagged = 1;
                	pgm->unflagged = 0;
		}

    This (as well as any other places which use fold == 2 as a sign to look for urgent messages is incorrect. folder_int returns 2 for the "Work" folder.