From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Roger_Pau_Monn=E9?= Subject: Re: [PATCH] xl: Return an error if an empty file is passed to cd-insert Date: Mon, 13 May 2013 11:35:45 +0200 Message-ID: <5190B3F1.3060703@citrix.com> References: <1368200604-25882-1-git-send-email-george.dunlap@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1368200604-25882-1-git-send-email-george.dunlap@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: Ian Jackson , Ian Campbell , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 10/05/13 17:43, George Dunlap wrote: > Two changes: > * Stat the file before calling libxl_cdrom_insert() > * Return an error if anything fails (including libxl_cdrom_insert) > > This is in part to work around the fact that the RAW disk type > is used for things that aren't actually files; so we can't call > stat in libxl_device.c:libxl__device_disk_set_backend() because > it may be going over a remote protocol. > > Signed-off-by: George Dunlap > CC: Ian Campbell > CC: Ian Jackson > --- > tools/libxl/xl_cmdimpl.c | 26 +++++++++++++++++++++----- > 1 file changed, 21 insertions(+), 5 deletions(-) > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index c1a969b..e8ce35b 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -2505,25 +2505,42 @@ int main_memset(int argc, char **argv) > return 0; > } > > -static void cd_insert(uint32_t domid, const char *virtdev, char *phys) > +static int cd_insert(uint32_t domid, const char *virtdev, char *phys) > { > libxl_device_disk disk; /* we don't free disk's contents */ > char *buf = NULL; > XLU_Config *config = 0; > + struct stat b; > + int rc = 0; > > > if (asprintf(&buf, "vdev=%s,access=r,devtype=cdrom,target=%s", > virtdev, phys ? phys : "") < 0) { > fprintf(stderr, "out of memory\n"); > - return; > + return 1; ERROR_NOMEM > } > > parse_disk_config(&config, buf, &disk); > > - libxl_cdrom_insert(ctx, domid, &disk, NULL); > + /* ATM the existence of the backing file is not checked for qdisk > + * in libxl_cdrom_insert() because RAW is used for remote > + * protocols as well as plain files. This will ideally be changed > + * for 4.4, but this work-around fixes the problem of "cd-insert" > + * returning success for non-existent files. */ > + if (disk.format != LIBXL_DISK_FORMAT_EMPTY > + && stat(disk.pdev_path, &b)) { > + fprintf(stderr, "Cannot stat file: %s\n", > + disk.pdev_path); You are leaking buf here, I know xl is just about to exit, but it might be best to to define a label below that contains: out: free(buf); return rc; and go to it. > + return 1; ERROR_INVAL > + } > + > + if (libxl_cdrom_insert(ctx, domid, &disk, NULL)) > + rc=1; You could use the return value of libxl_cdrom_insert instead of 1. > > libxl_device_disk_dispose(&disk); > free(buf); > + > + return rc; > } > > int main_cd_eject(int argc, char **argv) > @@ -2539,8 +2556,7 @@ int main_cd_eject(int argc, char **argv) > domid = find_domain(argv[optind]); > virtdev = argv[optind + 1]; > > - cd_insert(domid, virtdev, NULL); > - return 0; > + return cd_insert(domid, virtdev, NULL); > } > > int main_cd_insert(int argc, char **argv) >