[Asterisk-code-review] funcs/func curl: Add the ability for CURL to download and st... (asterisk[master])

[Mark Michelson]

Mark Michelson has posted comments on this change.

Change subject: funcs/func_curl: Add the ability for CURL to download and store files
......................................................................


Patch Set 2:

(2 comments)

https://gerrit.asterisk.org/#/c/1888/2/funcs/func_curl.c
File funcs/func_curl.c:

Line 555: 	/*! \brief If a string is being built, the string buffer */
        : 	struct ast_str *str;
        : 	/*! \brief The max size of \ref str */
        : 	ssize_t len;
        : 	/*! \brief If a file is being retrieved, the file to write to */
        : 	FILE *out_file;
Seems like this is a decent use case for a union since you'll either be writing a string or a file, but never both.


Line 808: 	if (ast_strlen_zero(args.file_type)) {
        : 		args.file_type = "wb";
        : 	}
This is a bit awkward to me that you allow the mode to be specified by the user.

Personally, I think  that you should go one of two routes here:

* Condense this to a "write" or "append" option to choose between.
* Don't even offer a choice and always open in "w" mode.

Personally, I think the desire to append to an existing file will be minimal. Plus, if someone really wants to combine the contents of multiple files together, they can always use `cat` for that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44f4ad823d7d20f04ceaad3698c5c7f653c41b0d
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes