| Message ID | 1455620983-3109-1-git-send-email-ian.campbell@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 16, 2016 at 11:09:43AM +0000, Ian Campbell wrote: > The underlying code paths in tap_ctl_create attempt to handle both > *devpath == NULL (by allocating) and *devpath != NULL (caller provided > name) and if they allocate tend to write the return immediately before > doing other potentially error generating tasks. All of which makes > handling this at a lower level rather more complicated than handling > it in the error path of libxl__bllktap_devpath. > Extraneous "l" in the function name. > Note that libxl__blktap_devpath initialises devpath to NULL and if the > earlier GCSPRINTF succeeds then the value is returned earlier. > Therefore if we make it to the call to tap_ctl_create then devpath is > still NULL on entry, therefore on the error path devpath is either > still NULL or has been set to a freshly allocated value by > tap_ctl_create. Since free(NULL) is fine it is sufficient to just > free(devpath). > > I also considered adding a non-NULL devnull to the gc, even on > failure, but that would have required a comment to explain the > apparently strange behaviour. > > Compile tested only. > > CID: 1215177 > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Acked-by: Wei Liu <wei.liu2@citrix.com> > --- > tools/libxl/libxl_blktap2.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/libxl/libxl_blktap2.c b/tools/libxl/libxl_blktap2.c > index 4f5e522..5c9a0a2 100644 > --- a/tools/libxl/libxl_blktap2.c > +++ b/tools/libxl/libxl_blktap2.c > @@ -47,6 +47,7 @@ char *libxl__blktap_devpath(libxl__gc *gc, > return devname; > } > > + free(devname); > return NULL; > } > > -- > 2.1.4 >
Wei Liu writes ("Re: [PATCH] tools: libxl: free devpath on failure in libxl__blktap_devpath"):
> On Tue, Feb 16, 2016 at 11:09:43AM +0000, Ian Campbell wrote:
> > The underlying code paths in tap_ctl_create attempt to handle both
> > *devpath == NULL (by allocating) and *devpath != NULL (caller provided
> > name) and if they allocate tend to write the return immediately before
> > doing other potentially error generating tasks. All of which makes
> > handling this at a lower level rather more complicated than handling
> > it in the error path of libxl__bllktap_devpath.
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Extraneous "l" in the function name.
Fixed this.
Thanks,
Ian.
diff --git a/tools/libxl/libxl_blktap2.c b/tools/libxl/libxl_blktap2.c index 4f5e522..5c9a0a2 100644 --- a/tools/libxl/libxl_blktap2.c +++ b/tools/libxl/libxl_blktap2.c @@ -47,6 +47,7 @@ char *libxl__blktap_devpath(libxl__gc *gc, return devname; } + free(devname); return NULL; }
The underlying code paths in tap_ctl_create attempt to handle both *devpath == NULL (by allocating) and *devpath != NULL (caller provided name) and if they allocate tend to write the return immediately before doing other potentially error generating tasks. All of which makes handling this at a lower level rather more complicated than handling it in the error path of libxl__bllktap_devpath. Note that libxl__blktap_devpath initialises devpath to NULL and if the earlier GCSPRINTF succeeds then the value is returned earlier. Therefore if we make it to the call to tap_ctl_create then devpath is still NULL on entry, therefore on the error path devpath is either still NULL or has been set to a freshly allocated value by tap_ctl_create. Since free(NULL) is fine it is sufficient to just free(devpath). I also considered adding a non-NULL devnull to the gc, even on failure, but that would have required a comment to explain the apparently strange behaviour. Compile tested only. CID: 1215177 Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- tools/libxl/libxl_blktap2.c | 1 + 1 file changed, 1 insertion(+)