[CRIU] [PATCH 1/8] util: Add read_string helper

[Cyrill Gorcunov]

On Thu, May 10, 2012 at 11:04:52AM +0400, Andrew Vagin wrote:
> On Sat, May 05, 2012 at 07:20:17PM +0400, Cyrill Gorcunov wrote:
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>
> > ---
> >  include/util.h |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> > 
> 
> > diff --git a/include/util.h b/include/util.h
> > index 5172a88..62e3877 100644
> > --- a/include/util.h
> > +++ b/include/util.h
> > @@ -132,6 +132,13 @@ static inline int read_img_buf(int fd, void *ptr, int size)
> >  #define memzero_p(p)		memset(p, 0, sizeof(*p))
> >  #define memzero(p, size)	memset(p, 0, size)
> >  
> > +static inline int read_string(int fd, char *ptr, int size)
> > +{
> > +	int ret = read(fd, ptr, size);
> > +	ptr[ret > 0 ? ret - 1 : 0] = '\0';

> I don't like it, the last symbol may be non-zero, but we set it to zero,
> it may hide a problem.

This is done by design. Sure we can return -ENOMEM in case if string
is trimmed, still it must be guaranted that returned result _is_
as C-string as function implies.

> 	if (ret < 0)
> 		return ret;
> 	else if (ret == size) {
> 		if (ptr[ret - 1] != '\0')
> 			return -ENOMEM;
> 		return ret;
> 	} else
> 		ptr[ret] = '\0';
> 
> Why can you not use my decision for similar cases?

I don't like the idea of introducing global variables, this
kills ability to use functions in parallel execution. Another
misdesign I think is that instead of implementing safe-string-read
in a function itself we introduce 2 requirements

 - a _speacially_ configured buffer must be passed
 - a reader must take into account that buffer has zero byte
   at the end

this looks like overdone for me.

> I think we should have one method for one type of problems.
> I agree with both of them, so you should chage it everywhere.

	Cyrill