* [PATCH][ioemu] strip tap subtype prefix from image name (restyled)
@ 2008-01-30 14:14 Pat Campbell
2008-01-30 14:38 ` Christoph Egger
0 siblings, 1 reply; 6+ messages in thread
From: Pat Campbell @ 2008-01-30 14:14 UTC (permalink / raw)
To: xen-devel@lists.xensource.com
[-- Attachment #1: Type: text/plain, Size: 523 bytes --]
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>
[-- Attachment #2: xen-blktab-subtype-strip.patch --]
[-- Type: text/x-patch, Size: 1682 bytes --]
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)
+ 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) {
+ 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);
}
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH][ioemu] strip tap subtype prefix from image name (restyled)
2008-01-30 14:14 [PATCH][ioemu] strip tap subtype prefix from image name (restyled) Pat Campbell
@ 2008-01-30 14:38 ` Christoph Egger
2008-01-30 14:45 ` Keir Fraser
2008-01-30 16:24 ` Pat Campbell
0 siblings, 2 replies; 6+ messages in thread
From: Christoph Egger @ 2008-01-30 14:38 UTC (permalink / raw)
To: xen-devel; +Cc: Pat Campbell
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH][ioemu] strip tap subtype prefix from image name (restyled)
2008-01-30 14:38 ` Christoph Egger
@ 2008-01-30 14:45 ` Keir Fraser
2008-01-31 8:36 ` Christoph Egger
2008-01-30 16:24 ` Pat Campbell
1 sibling, 1 reply; 6+ messages in thread
From: Keir Fraser @ 2008-01-30 14:45 UTC (permalink / raw)
To: Christoph Egger, xen-devel; +Cc: Pat Campbell
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][ioemu] strip tap subtype prefix from image name (restyled)
2008-01-30 14:45 ` Keir Fraser
@ 2008-01-31 8:36 ` Christoph Egger
0 siblings, 0 replies; 6+ messages in thread
From: Christoph Egger @ 2008-01-31 8:36 UTC (permalink / raw)
To: Keir Fraser; +Cc: xen-devel, Pat Campbell
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.
>
> -- Keir
That'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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][ioemu] strip tap subtype prefix from image name (restyled)
2008-01-30 14:38 ` Christoph Egger
2008-01-30 14:45 ` Keir Fraser
@ 2008-01-30 16:24 ` Pat Campbell
2008-01-30 16:31 ` Keir Fraser
1 sibling, 1 reply; 6+ messages in thread
From: Pat Campbell @ 2008-01-30 16:24 UTC (permalink / raw)
To: Christoph Egger; +Cc: xen-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH][ioemu] strip tap subtype prefix from image name (restyled)
2008-01-30 16:24 ` Pat Campbell
@ 2008-01-30 16:31 ` Keir Fraser
0 siblings, 0 replies; 6+ messages in thread
From: Keir Fraser @ 2008-01-30 16:31 UTC (permalink / raw)
To: Pat Campbell, Christoph Egger; +Cc: xen-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-01-31 8:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-01-30 14:14 [PATCH][ioemu] strip tap subtype prefix from image name (restyled) Pat Campbell
2008-01-30 14:38 ` Christoph Egger
2008-01-30 14:45 ` Keir Fraser
2008-01-31 8:36 ` Christoph Egger
2008-01-30 16:24 ` Pat Campbell
2008-01-30 16:31 ` Keir Fraser
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.