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 <[email protected]> > >>> Signed-off-by: Hitoshi Mitake <[email protected]> > >>> --- > >>> 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 > >> [email protected] > >> http://lists.wpkg.org/mailman/listinfo/sheepdog > > > -- > sheepdog mailing list > [email protected] > http://lists.wpkg.org/mailman/listinfo/sheepdog -- sheepdog mailing list [email protected] http://lists.wpkg.org/mailman/listinfo/sheepdog
