[MPlayer-dev-eng] [PATCH] modify_reg.c

[Reimar Döffinger]

Hello,
On Sun, Aug 05, 2007 at 08:08:05PM +0200, Carl Eugen Hoyos wrote:
[...]
> +#define mp_msg(a, b, c, d, e)
> +#include "get_path.c"

This is quite brittle, at least use the mp_msg stub that e.g.
codec-cfg.c uses.

> +    tmp_val = regs[i].value;
> +    tmp_name = regs[i].name;
> +    for(;i < reg_size -1; i++)
> +        memcpy(&regs[i], &regs[i+1], sizeof(struct reg_value));
> +    reg_size--;
> +    free(tmp_val);
> +    free(tmp_name);

What is the point of tmp_val and tmp_name, why not free them right away?
And why the for-loop, why not memmove the whole block in one step?
The same is done in hier_remove_key as well.

> +  strncpy(key, tmpkey + 1, strlen(tmpkey)-1);
> +  key[strlen(tmpkey)-1] = 0;

Too bad av_strlcpy is not available... But maybe it's worth
copy-and-pasting it?

> +  static struct option Long_Options[] = {
> +    {"registry", 1, 0, 'r'},
> +    {"list", 0, 0, 'l'},
> +    {"key", 1, 0, 'k'},
> +    {"value", 1, 0, 'v'},
> +    {"type", 1, 0, 't'},
> +    {"del", 0, 0, 'd'},
> +  };
> +  while(1) {
> +    c = getopt_long(argc, argv, "r:lk:v:t:id", Long_Options, &Option_Index);

It is probably not worth changing anything, but getopt_long IIRC can be
quite a mess portability-wise.


> +      if(type == REG_DWORD) {
> +        len = sizeof(DWORD);
> +	v = strtoul(value, NULL, 0);

Tab

Greetings,
Reimar Döffinger