[sheepdog] [PATCH] dog: don't free value if it is passed via command line

[Hitoshi Mitake]

At Wed, 20 Aug 2014 09:51:20 +0800,
Ruoyu wrote:
> 
> 
> On 2014年08月18日 10:26, Hitoshi Mitake wrote:
> > At Fri, 15 Aug 2014 10:26:42 +0800,
> > Ruoyu wrote:
> >>
> >> On 2014年08月15日 09:32, Hitoshi Mitake wrote:
> >>> vdi_setattr() has two way of obtaining value of attr:
> >>> 1. command line parameter
> >>> 2. read from stdin
> >>>
> >>> In a case of 1, the value shouldn't be freed because it is not in heap
> >>> area. But current dog does it. This patch removes this invalid free.
> >> In case 1, what about value = strdup(argv[optind++]) to alloc the space 
> >> in stack area?
> >> Then, free will fit for both situations.
> >> Because I think the new variable value_allocated is confused for developers.
> > I want to avoid using strdup() in this case. Because strdup() can fail
> > and we need to add another error handling code. It would add another
> > complexity.
> How about adding a new function xstrdup with error handling bundled,
> similar to xmalloc?

Sounds nice. I'll do it in v2.

Thanks,
Hitoshi

> >
> > How about enhancing comment for the new variable (value_allocated)?
> I hope the code is self-explained as far as possible.
> >
> > Thanks,
> > Hitoshi
> >
> >>> Reported-by: Ruoyu <liangry at ucweb.com>
> >>> Signed-off-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
> >>> ---
> >>>   dog/vdi.c | 4 +++-
> >>>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/dog/vdi.c b/dog/vdi.c
> >>> index 84715b3..1d8cc6e 100644
> >>> --- a/dog/vdi.c
> >>> +++ b/dog/vdi.c
> >>> @@ -1278,6 +1278,7 @@ static int vdi_setattr(int argc, char **argv)
> >>>   	const char *vdiname = argv[optind++], *key;
> >>>   	char *value = NULL;
> >>>   	uint64_t offset;
> >>> +	bool value_alloced = false;
> >>>   
> >>>   	key = argv[optind++];
> >>>   	if (!key) {
> >>> @@ -1289,6 +1290,7 @@ static int vdi_setattr(int argc, char **argv)
> >>>   	value = argv[optind++];
> >>>   	if (!value && !vdi_cmd_data.delete) {
> >>>   		value = xmalloc(SD_MAX_VDI_ATTR_VALUE_LEN);
> >>> +		value_alloced = true;
> >>>   
> >>>   		offset = 0;
> >>>   reread:
> >>> @@ -1333,7 +1335,7 @@ reread:
> >>>   	}
> >>>   
> >>>   out:
> >>> -	if (value)
> >>> +	if (value_alloced)
> >>>   		free(value);
> >>>   
> >>>   	return ret;
> >>
> >> -- 
> >> sheepdog mailing list
> >> sheepdog at lists.wpkg.org
> >> http://lists.wpkg.org/mailman/listinfo/sheepdog
> 
> 
> -- 
> sheepdog mailing list
> sheepdog at lists.wpkg.org
> http://lists.wpkg.org/mailman/listinfo/sheepdog