Pat Campbell
2008-Jan-30 14:14 UTC
[Xen-devel][PATCH][ioemu] strip tap subtype prefix from image name (restyled)
More readable version
Currently I am not able to mount or boot from an HVM CDROM when it is configured
for ''tap:aio'' instead of ''file''.
disk=[
''tap:aio:/var/lib/xen/images/sles10-sp2-fv/disk0,hda,w'',
''
tap:aio:/home/iso/sles/SLES10.iso,hdc:cdrom,r'', ]
With the attached patch I am able to boot from the CDROM and or mount it.
Patch changes xenstore.c:xenstore_process_event() to strip the tap subtype
prefix from the image name.
Please apply to xen-unstable tip.
Signed-off-by: Pat Campbell <plc@novell.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Christoph Egger
2008-Jan-30 14:38 UTC
Re: [Xen-devel][PATCH][ioemu] strip tap subtype prefix from image name (restyled)
On Wednesday 30 January 2008 15:14:24 Pat Campbell wrote:> More readable version > > Currently I am not able to mount or boot from an HVM CDROM when it is > configured for ''tap:aio'' instead of ''file''. > > disk=[ ''tap:aio:/var/lib/xen/images/sles10-sp2-fv/disk0,hda,w'', '' > tap:aio:/home/iso/sles/SLES10.iso,hdc:cdrom,r'', ] > > With the attached patch I am able to boot from the CDROM and or mount it. > > Patch changes xenstore.c:xenstore_process_event() to strip the tap subtype > prefix from the image name. > > Please apply to xen-unstable tip. > > Signed-off-by: Pat Campbell <plc@novell.com>That looks much better. See inline for comments.> xen-blktab-subtype-strip.patch > diff -r 1c826ea72a80 tools/ioemu/xenstore.c > --- a/tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 > +++ b/tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 > @@ -418,7 +418,7 @@ void xenstore_record_dm_state(char *stat > > void xenstore_process_event(void *opaque) > { > - char **vec, *image = NULL; > + char **vec, *offset, *bpath = NULL, *buf = NULL, *drv = NULL, *image =NULL;> unsigned int len, num, hd_index; > > vec = xs_read_watch(xsh, &num); > @@ -440,8 +440,28 @@ void xenstore_process_event(void *opaque > goto out; > hd_index = vec[XS_WATCH_TOKEN][2] - ''a''; > image = xs_read(xsh, XBT_NULL, vec[XS_WATCH_PATH], &len); > - if (image == NULL || !strcmp(image, bs_table[hd_index]->filename)) > - goto out; /* gone or identical */ > + if (image == NULL) > + goto out; /* gone */ > + > + /* Strip off blktap sub-type prefix */ > + bpath = strdup(vec[XS_WATCH_PATH]); > + if (bpath)I think, you mean if (!bpath) here.> + goto out; > + if ((offset = strrchr(bpath, ''/'')) != NULL) > + *offset = ''\0''; > + if (pasprintf(&buf, "%s/type", bpath) == -1) > + goto out; > + drv = xs_read(xsh, XBT_NULL, buf, &len); > + if (drv) {I think, you mean "if (!drv) goto out;" here.> + if (!strcmp(drv, "tap")) { > + offset = strchr(image, '':''); > + if (offset) > + memmove(image, offset+1, strlen(offset+1)+1 ); > + } > + } > + > + if (!strcmp(image, bs_table[hd_index]->filename)) > + goto out; /* identical */ > > do_eject(0, vec[XS_WATCH_TOKEN]); > bs_table[hd_index]->filename[0] = 0; > @@ -456,6 +476,9 @@ void xenstore_process_event(void *opaque > } > > out: > + free(drv); > + free(buf); > + free(bpath); > free(image); > free(vec);That doesn''t work. If strdup() fails, then drv and buf are NULL here. Analagous counts for the other failures. You need to check for != NULL before calling free().> }-- AMD Saxony, Dresden, Germany Operating System Research Center Legal Information: AMD Saxony Limited Liability Company & Co. KG Sitz (Geschäftsanschrift): Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland Registergericht Dresden: HRA 4896 vertretungsberechtigter Komplementär: AMD Saxony LLC (Sitz Wilmington, Delaware, USA) Geschäftsführer der AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Jan-30 14:45 UTC
Re: [Xen-devel][PATCH][ioemu] strip tap subtype prefix from image name (restyled)
On 30/1/08 14:38, "Christoph Egger" <Christoph.Egger@amd.com> wrote:>> out: >> + free(drv); >> + free(buf); >> + free(bpath); >> free(image); >> free(vec); > > That doesn''t work. If strdup() fails, then drv and buf are NULL here. > Analagous counts for the other failures. > You need to check for != NULL before calling free().free() is defined to accept a NULL argument. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Pat Campbell
2008-Jan-30 16:24 UTC
Re: [Xen-devel][PATCH][ioemu] strip tap subtype prefix from image name (restyled)
Christoph Egger wrote:> On Wednesday 30 January 2008 15:14:24 Pat Campbell wrote: > >> More readable version >> >> Currently I am not able to mount or boot from an HVM CDROM when it is >> configured for ''tap:aio'' instead of ''file''. >> >> disk=[ ''tap:aio:/var/lib/xen/images/sles10-sp2-fv/disk0,hda,w'', '' >> tap:aio:/home/iso/sles/SLES10.iso,hdc:cdrom,r'', ] >> >> With the attached patch I am able to boot from the CDROM and or mount it. >> >> Patch changes xenstore.c:xenstore_process_event() to strip the tap subtype >> prefix from the image name. >> >> Please apply to xen-unstable tip. >> >> Signed-off-by: Pat Campbell <plc@novell.com> >> > > That looks much better. > See inline for comments. > > >> xen-blktab-subtype-strip.patch >> diff -r 1c826ea72a80 tools/ioemu/xenstore.c >> --- a/tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 >> +++ b/tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 >> @@ -418,7 +418,7 @@ void xenstore_record_dm_state(char *stat >> >> void xenstore_process_event(void *opaque) >> { >> - char **vec, *image = NULL; >> + char **vec, *offset, *bpath = NULL, *buf = NULL, *drv = NULL, *image = >> > NULL; > >> unsigned int len, num, hd_index; >> >> vec = xs_read_watch(xsh, &num); >> @@ -440,8 +440,28 @@ void xenstore_process_event(void *opaque >> goto out; >> hd_index = vec[XS_WATCH_TOKEN][2] - ''a''; >> image = xs_read(xsh, XBT_NULL, vec[XS_WATCH_PATH], &len); >> - if (image == NULL || !strcmp(image, bs_table[hd_index]->filename)) >> - goto out; /* gone or identical */ >> + if (image == NULL) >> + goto out; /* gone */ >> + >> + /* Strip off blktap sub-type prefix */ >> + bpath = strdup(vec[XS_WATCH_PATH]); >> + if (bpath) >> > > I think, you mean if (!bpath) here. >You are right, missed that in my recode.> >> + goto out; >> + if ((offset = strrchr(bpath, ''/'')) != NULL) >> + *offset = ''\0''; >> + if (pasprintf(&buf, "%s/type", bpath) == -1) >> + goto out; >> + drv = xs_read(xsh, XBT_NULL, buf, &len); >> + if (drv) { >> > > I think, you mean "if (!drv) goto out;" here. >My thinking was, if we read the type we want to check for tap, otherwise we want to fall thru and do the normal image compare. Failure to read type is not be an error condition. It can be viewed as an error condition I suppose. Will change that to.> >> + if (!strcmp(drv, "tap")) { >> + offset = strchr(image, '':''); >> + if (offset) >> + memmove(image, offset+1, strlen(offset+1)+1 ); >> + } >> + } >> + >> + if (!strcmp(image, bs_table[hd_index]->filename)) >> + goto out; /* identical */ >> >> do_eject(0, vec[XS_WATCH_TOKEN]); >> bs_table[hd_index]->filename[0] = 0; >> @@ -456,6 +476,9 @@ void xenstore_process_event(void *opaque >> } >> >> out: >> + free(drv); >> + free(buf); >> + free(bpath); >> free(image); >> free(vec); >> > > That doesn''t work. If strdup() fails, then drv and buf are NULL here. > Analagous counts for the other failures. > You need to check for != NULL before calling free(). > >It appears this code style relies on free dealing with a NULL. Just trying to fit it :-)>> } >> > > >Thanks for taking the time. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Jan-30 16:31 UTC
Re: [Xen-devel][PATCH][ioemu] strip tap subtype prefix from image name (restyled)
I already checked in an appropriate fix following Christoph''s comments. -- Keir On 30/1/08 16:24, "Pat Campbell" <plc@novell.com> wrote:> Christoph Egger wrote: >> On Wednesday 30 January 2008 15:14:24 Pat Campbell wrote: >> >>> More readable version >>> >>> Currently I am not able to mount or boot from an HVM CDROM when it is >>> configured for ''tap:aio'' instead of ''file''. >>> >>> disk=[ ''tap:aio:/var/lib/xen/images/sles10-sp2-fv/disk0,hda,w'', '' >>> tap:aio:/home/iso/sles/SLES10.iso,hdc:cdrom,r'', ] >>> >>> With the attached patch I am able to boot from the CDROM and or mount it. >>> >>> Patch changes xenstore.c:xenstore_process_event() to strip the tap subtype >>> prefix from the image name. >>> >>> Please apply to xen-unstable tip. >>> >>> Signed-off-by: Pat Campbell <plc@novell.com> >>> >> >> That looks much better. >> See inline for comments. >> >> >>> xen-blktab-subtype-strip.patch >>> diff -r 1c826ea72a80 tools/ioemu/xenstore.c >>> --- a/tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 >>> +++ b/tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 >>> @@ -418,7 +418,7 @@ void xenstore_record_dm_state(char *stat >>> >>> void xenstore_process_event(void *opaque) >>> { >>> - char **vec, *image = NULL; >>> + char **vec, *offset, *bpath = NULL, *buf = NULL, *drv = NULL, *image >>> >> NULL; >> >>> unsigned int len, num, hd_index; >>> >>> vec = xs_read_watch(xsh, &num); >>> @@ -440,8 +440,28 @@ void xenstore_process_event(void *opaque >>> goto out; >>> hd_index = vec[XS_WATCH_TOKEN][2] - ''a''; >>> image = xs_read(xsh, XBT_NULL, vec[XS_WATCH_PATH], &len); >>> - if (image == NULL || !strcmp(image, bs_table[hd_index]->filename)) >>> - goto out; /* gone or identical */ >>> + if (image == NULL) >>> + goto out; /* gone */ >>> + >>> + /* Strip off blktap sub-type prefix */ >>> + bpath = strdup(vec[XS_WATCH_PATH]); >>> + if (bpath) >>> >> >> I think, you mean if (!bpath) here. >> > You are right, missed that in my recode. >> >>> + goto out; >>> + if ((offset = strrchr(bpath, ''/'')) != NULL) >>> + *offset = ''\0''; >>> + if (pasprintf(&buf, "%s/type", bpath) == -1) >>> + goto out; >>> + drv = xs_read(xsh, XBT_NULL, buf, &len); >>> + if (drv) { >>> >> >> I think, you mean "if (!drv) goto out;" here. >> > My thinking was, if we read the type we want to check for tap, otherwise > we want to fall thru and do the normal image compare. Failure to read > type is not be an error condition. > > It can be viewed as an error condition I suppose. Will change that to. >> >>> + if (!strcmp(drv, "tap")) { >>> + offset = strchr(image, '':''); >>> + if (offset) >>> + memmove(image, offset+1, strlen(offset+1)+1 ); >>> + } >>> + } >>> + >>> + if (!strcmp(image, bs_table[hd_index]->filename)) >>> + goto out; /* identical */ >>> >>> do_eject(0, vec[XS_WATCH_TOKEN]); >>> bs_table[hd_index]->filename[0] = 0; >>> @@ -456,6 +476,9 @@ void xenstore_process_event(void *opaque >>> } >>> >>> out: >>> + free(drv); >>> + free(buf); >>> + free(bpath); >>> free(image); >>> free(vec); >>> >> >> That doesn''t work. If strdup() fails, then drv and buf are NULL here. >> Analagous counts for the other failures. >> You need to check for != NULL before calling free(). >> >> > It appears this code style relies on free dealing with a NULL. Just > trying to fit it :-) >>> } >>> >> >> >> > Thanks for taking the time. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger
2008-Jan-31 08:36 UTC
Re: [Xen-devel][PATCH][ioemu] strip tap subtype prefix from image name (restyled)
On Wednesday 30 January 2008 15:45:03 Keir Fraser wrote:> On 30/1/08 14:38, "Christoph Egger" <Christoph.Egger@amd.com> wrote: > >> out: > >> + free(drv); > >> + free(buf); > >> + free(bpath); > >> free(image); > >> free(vec); > > > > That doesn''t work. If strdup() fails, then drv and buf are NULL here. > > Analagous counts for the other failures. > > You need to check for != NULL before calling free(). > > free() is defined to accept a NULL argument. > > -- KeirThat''s right. But that is the way to get in double-free() security holes. Christoph -- AMD Saxony, Dresden, Germany Operating System Research Center Legal Information: AMD Saxony Limited Liability Company & Co. KG Sitz (Geschäftsanschrift): Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland Registergericht Dresden: HRA 4896 vertretungsberechtigter Komplementär: AMD Saxony LLC (Sitz Wilmington, Delaware, USA) Geschäftsführer der AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel