[pacman-dev] [PATCH v3 2/4] pacman/upgrade: Refactor memory management

[Andrew Gregory]

On 03/11/14 at 07:29pm, Sören Brinkmann wrote:
> Refactor the upgrade routine to use an array that can be allocated in
> one step instead of an alpm_list that is gradually extended in loops.
> 
> Signed-off-by: Sören Brinkmann <soeren.brinkmann at gmail.com>
> ---

Looks good; I just have a few small style nitpicks.

>  src/pacman/upgrade.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c
> index 11da4dcb5e53..e0cdf564971e 100644
> --- a/src/pacman/upgrade.c
> +++ b/src/pacman/upgrade.c
> @@ -39,23 +39,25 @@
>   */
>  int pacman_upgrade(alpm_list_t *targets)
>  {
> -	int retval = 0;
> -	alpm_list_t *i, *j, *remote = NULL;
> +	int retval = 0, *file_is_remote;
> +	alpm_list_t *i;
> +	unsigned int n, num_targets;
>  
>  	if(targets == NULL) {
>  		pm_printf(ALPM_LOG_ERROR, _("no targets specified (use -h for help)\n"));
>  		return 1;
>  	}
>  
> -	/* Check for URL targets and process them
> -	 */
> -	for(i = targets; i; i = alpm_list_next(i)) {
> -		int *r = malloc(sizeof(int));
> -		if(r == NULL) {
> -			pm_printf(ALPM_LOG_ERROR, _("memory exhausted\n"));
> -			return 1;
> -		}
> +	num_targets = alpm_list_count(targets);
>  
> +	/* Check for URL targets and process them */
> +	file_is_remote = malloc(num_targets * sizeof(*file_is_remote));

Our style guidelines require that sizeof() be used with types, not
values.

> +	if(file_is_remote == NULL) {
> +		pm_printf(ALPM_LOG_ERROR, _("memory exhausted\n"));
> +		return 1;
> +	}
> +
> +	for(i = targets, n = 0; n < num_targets; i = alpm_list_next(i), n++) {

I would prefer to use i as the conditional check here instead of
n since it is such a common idiom throughout our codebase.

>  		if(strstr(i->data, "://")) {
>  			char *str = alpm_fetch_pkgurl(config->handle, i->data);
>  			if(str == NULL) {
> @@ -65,13 +67,11 @@ int pacman_upgrade(alpm_list_t *targets)
>  			} else {
>  				free(i->data);
>  				i->data = str;
> -				*r = 1;
> +				file_is_remote[n] = 1;
>  			}
>  		} else {
> -			*r = 0;
> +			file_is_remote[n] = 0;
>  		}
> -
> -		remote = alpm_list_add(remote, r);
>  	}
>  
>  	if(retval) {
> @@ -85,12 +85,12 @@ int pacman_upgrade(alpm_list_t *targets)
>  
>  	printf(_("loading packages...\n"));
>  	/* add targets to the created transaction */
> -	for(i = targets, j = remote; i; i = alpm_list_next(i), j = alpm_list_next(j)) {
> +	for(i = targets, n = 0; n < num_targets; i = alpm_list_next(i), n++) {

Same as above.

>  		const char *targ = i->data;
>  		alpm_pkg_t *pkg;
>  		alpm_siglevel_t level;
>  
> -		if(*(int *)j->data) {
> +		if(file_is_remote[n]) {
>  			level = alpm_option_get_remote_file_siglevel(config->handle);
>  		} else {
>  			level = alpm_option_get_local_file_siglevel(config->handle);
> @@ -112,7 +112,7 @@ int pacman_upgrade(alpm_list_t *targets)
>  		config->explicit_adds = alpm_list_add(config->explicit_adds, pkg);
>  	}
>  
> -	FREELIST(remote);
> +	free(file_is_remote);
>  
>  	if(retval) {
>  		trans_release();
> -- 
> 1.9.0