From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH] xl: Return an error if an empty file is passed to cd-insert Date: Mon, 13 May 2013 11:31:24 +0100 Message-ID: <5190C0FC.4010700@eu.citrix.com> References: <1368200604-25882-1-git-send-email-george.dunlap@eu.citrix.com> <5190B3F1.3060703@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <5190B3F1.3060703@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: =?ISO-8859-1?Q?Roger_Pau_Monn=E9?= Cc: Ian Jackson , Ian Campbell , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 13/05/13 10:35, Roger Pau Monn=E9 wrote: > 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 =3D NULL; >> XLU_Config *config =3D 0; >> + struct stat b; >> + int rc =3D 0; >> = >> = >> if (asprintf(&buf, "vdev=3D%s,access=3Dr,devtype=3Dcdrom,target=3D= %s", >> virtdev, phys ? phys : "") < 0) { >> fprintf(stderr, "out of memory\n"); >> - return; >> + return 1; > ERROR_NOMEM If this were a library function I would definitely return -ERROR_NOMEM; = but since this is just a helper function whose value is going to be = returned to the command-line, I think just returning 1 is the best thing = to do. > >> } >> = >> 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 !=3D 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: Oops! Sorry about that... >> + return 1; > ERROR_INVAL > >> + } >> + >> + if (libxl_cdrom_insert(ctx, domid, &disk, NULL)) >> + rc=3D1; > You could use the return value of libxl_cdrom_insert instead of 1. I sort of assumed that libxl_cdrom_insert() was a negative value, and = that I wanted xl to return 1. I'll send a respin w/ the leak fixed. -George