George Dunlap
2013-May-10 15:43 UTC
[PATCH] xl: Return an error if an empty file is passed to cd-insert
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 <george.dunlap@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
---
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;
}
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);
+ return 1;
+ }
+
+ if (libxl_cdrom_insert(ctx, domid, &disk, NULL))
+ rc=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)
--
1.7.9.5
George Dunlap
2013-May-10 15:57 UTC
Re: [PATCH] xl: Return an error if an empty file is passed to cd-insert
On Fri, May 10, 2013 at 4:43 PM, George Dunlap <george.dunlap@eu.citrix.com> wrote:> Two changes: > * Stat the file before calling libxl_cdrom_insert() > * Return an error if anything fails (including libxl_cdrom_insert)Hmm -- not sure how "empty file" got into that description -- it should say "non-existent file"... -George> > 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 <george.dunlap@eu.citrix.com> > CC: Ian Campbell <ian.campbell@citrix.com> > CC: Ian Jackson <ian.jackson@citrix.com> > --- > 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; > } > > 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); > + return 1; > + } > + > + if (libxl_cdrom_insert(ctx, domid, &disk, NULL)) > + rc=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) > -- > 1.7.9.5 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Roger Pau Monné
2013-May-13 09:35 UTC
Re: [PATCH] xl: Return an error if an empty file is passed to cd-insert
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 <george.dunlap@eu.citrix.com> > CC: Ian Campbell <ian.campbell@citrix.com> > CC: Ian Jackson <ian.jackson@citrix.com> > --- > 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) >
Ian Campbell
2013-May-13 09:45 UTC
Re: [PATCH] xl: Return an error if an empty file is passed to cd-insert
On Mon, 2013-05-13 at 10:35 +0100, Roger Pau Monne 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 <george.dunlap@eu.citrix.com> > > CC: Ian Campbell <ian.campbell@citrix.com> > > CC: Ian Jackson <ian.jackson@citrix.com> > > --- > > 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_NOMEMxl seems pretty inconsistent in its use of libxl error codes, sometimes it uses +ERROR_FOO, other times -ERROR_FOO and other times it uses its own open coded numbers. So I wouldn''t say this is wrong as such but so long as the final xl exit code is 0 on success and non-zero otherwise I think it is up to George.> > > } > > > > 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,I prefer to have xl clear up properly whenever possible. Not because I care particularly about xl (which isn''t generally long lived) but because it makes it possible to use valgrind+xl to check for leaks in libxl, and I care about that because other toolstacks which use libxl can be long lived and therefore care about leaks. Having xl not contain false positive leaks helps here. Ian.
Roger Pau Monné
2013-May-13 10:03 UTC
Re: [PATCH] xl: Return an error if an empty file is passed to cd-insert
On 13/05/13 11:45, Ian Campbell wrote:> On Mon, 2013-05-13 at 10:35 +0100, Roger Pau Monne 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 <george.dunlap@eu.citrix.com> >>> CC: Ian Campbell <ian.campbell@citrix.com> >>> CC: Ian Jackson <ian.jackson@citrix.com> >>> --- >>> 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 > > xl seems pretty inconsistent in its use of libxl error codes, sometimes > it uses +ERROR_FOO, other times -ERROR_FOO and other times it uses its > own open coded numbers. > > So I wouldn''t say this is wrong as such but so long as the final xl exit > code is 0 on success and non-zero otherwise I think it is up to George.Yes I agree, xl is full of return 1, -1 and things like that, but anyway I think it was worth mentioning.>> >>> } >>> >>> 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, > > I prefer to have xl clear up properly whenever possible. Not because I > care particularly about xl (which isn''t generally long lived) but > because it makes it possible to use valgrind+xl to check for leaks in > libxl, and I care about that because other toolstacks which use libxl > can be long lived and therefore care about leaks. Having xl not contain > false positive leaks helps here. > > Ian. >
George Dunlap
2013-May-13 10:31 UTC
Re: [PATCH] xl: Return an error if an empty file is passed to cd-insert
On 13/05/13 10:35, Roger Pau Monné 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 <george.dunlap@eu.citrix.com> >> CC: Ian Campbell <ian.campbell@citrix.com> >> CC: Ian Jackson <ian.jackson@citrix.com> >> --- >> 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_NOMEMIf 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 != 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=1; > 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
George Dunlap
2013-May-14 10:07 UTC
[PATCH] xl: Return an error if an empty file is passed to cd-insert
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.
v2:
- Avoid leaking buf
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
---
tools/libxl/xl_cmdimpl.c | 29 ++++++++++++++++++++++++-----
1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c1a969b..2ef6875 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2505,25 +2505,45 @@ 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;
+ rc = 1;
+ goto out;
}
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);
+ rc = 1;
+ goto out;
+ }
+
+ if (libxl_cdrom_insert(ctx, domid, &disk, NULL))
+ rc=1;
+out:
libxl_device_disk_dispose(&disk);
free(buf);
+
+ return rc;
}
int main_cd_eject(int argc, char **argv)
@@ -2539,8 +2559,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)
--
1.7.9.5
Ian Campbell
2013-May-30 09:00 UTC
Re: [PATCH] xl: Return an error if an empty file is passed to cd-insert
On Tue, 2013-05-14 at 11:07 +0100, 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. > > v2: > - Avoid leaking buf > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>Acked + applied, thanks.> + /* 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. */Is this on your 4.4 list? Ian.
George Dunlap
2013-May-30 09:28 UTC
Re: [PATCH] xl: Return an error if an empty file is passed to cd-insert
On Thu, May 30, 2013 at 10:00 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:> On Tue, 2013-05-14 at 11:07 +0100, 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. >> >> v2: >> - Avoid leaking buf >> >> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com> > > Acked + applied, thanks. > >> + /* 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. */ > > Is this on your 4.4 list?Yes: * libxl: Don''t use RAW format for "URL"-based qdisks (e.g., rbd:rbd/foo.img) We talked about this at the hackathon as well. -George