Re: [PATCH] commands: add 'findstr' to get string from file

[Alexander Aring <alex.aring@gmail.com>]

On Tue, May 20, 2014 at 10:29:20PM +0200, Christoph Fritz wrote:
> Hi Alexander
> 
> On Tue, 2014-05-20 at 20:08 +0200, Alexander Aring wrote:
> > On Tue, May 20, 2014 at 07:27:55PM +0200, Christoph Fritz wrote:
> 
> > > +config CMD_FINDSTR
> > > +	tristate
> > > +	default n
> > not needed.
> 
> Ok
> 
> > > +	while ((opt = getopt(argc, argv, "o:l:t:")) > 0) {
> > > +		switch (opt) {
> > > +		case 'o':
> > > +			offset = simple_strtoul(optarg, NULL, 0);
> > > +			break;
> > > +		case 'l':
> > > +			len = simple_strtoul(optarg, NULL, 0);
> > > +			break;
> > > +		case 't':
> > > +			t = optarg;
> > > +			break;
> > > +		}
> > 
> > switch case without default branch can occur compiler warnings.
> > 
> > You should return COMMAND_ERROR_USAGE in the default branch if non valid
> > parameter is given.
> 
> Ok
> 
> > > +	fd = open(argv[optind+1], O_RDONLY);
> > > +	if (fd < 0)
> > > +		return COMMAND_ERROR_USAGE;
> > > +
> > > +	while (1) {
> > > +		r = read(fd, mem_rw_buf, RW_BUF_SIZE);
> > > +		if (r < 0) {
> > > +			ret = -EIO;
> > > +			goto out;
> > > +		}
> > > +		if (!r)
> > > +			break;
> > > +
> > > +		v_idx = 0;
> > > +
> > > +		while (r) {
> > > +			if (v[v_idx] == s[s_idx])
> > > +				s_idx++;
> > > +			else
> > > +				s_idx = 0;
> > > +
> > > +			idx++;
> > > +			v_idx++;
> > > +
> > > +			if (s_idx == strlen(s)) {	/* found */
> > > +				loff_t sz;
> > > +				loff_t hit = idx - strlen(s);
> > > +
> > > +				if (lseek(fd, hit + offset, SEEK_SET) < 0) {
> > > +					ret = -EIO;
> > > +					goto out;
> > > +				}
> > > +
> > > +				if (!len)
> > > +					len = strlen(s);
> > > +				sz = min_t(loff_t, len, RW_BUF_SIZE - 1);
> > > +				r = read(fd, mem_rw_buf, sz);
> > > +				if (r != sz) {
> > > +					ret = -EIO;
> > > +					goto out;
> > > +				}
> > > +
> > > +				v[sz] = '\0';
> > > +
> > > +				if (t)
> > > +					setenv(t, v);
> > > +				else
> > > +					printf("%s\n", v);
> > > +
> > > +				ret = 0;
> > > +				goto out;
> > > +			}
> > > +			r--;
> > > +		}
> > 
> > Why not use read_file and the awesome string function strstr and then
> > write the file back.
> 
> read_file() reads the whole file which can get pretty slow over an i2c
> link accessing an eeprom which mostly holds the configuration-string at
> the beginning.
> 
> Why would I want to write the file back?
> 

Oh, thought you do that to manipulate the MAC configuration...
Yea, this could be slow and of course on big files like /dev/mem but
this would be more a pebcak related problem like somebody will do a
"edit /dev/mem", which should not work because edit use read_file.

Are you sure that the command also works if the searched string is
overlapped in two areas of RW_BUF_SIZE? I don't see that in the current
implementation.

You also assume here that the content is ascii based. Maybe you should
write that in the command description.

This handling would be much easier with a proper eeprom
configurationlayout with a static offset for each setting.

- Alex

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox