strip tap subtype prefix from image name (restyled)

PATCH In review Pat Campbell 3 participants 6 messages Jan 30, 2008 – Jan 31, 2008

lore

Share & tools

lore

Pat Campbell

author

2008-01-30 14:14 UTC #0 lore raw

PATCH 1/1 strip tap subtype prefix from image name (restyled)

Christoph Egger

2008-01-30 14:38 UTC #1 lore raw

Quoted 8 lines · 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', ]

Quoted 8 lines

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 <[email protected]>

That looks much better.
See inline for comments.

tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 void xenstore_record_dm_state(char *stat · line 421

Quoted 10 lines

xen-blktab-subtype-strip.patch
diff -r 1c826ea72a80 tools/ioemu/xenstore.c
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +1 -1
@@ -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;

Quoted 5 lines

tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +0 -0
     unsigned int len, num, hd_index;
 
     vec = xs_read_watch(xsh, &num);
@@ -440,8 +440,28 @@ void xenstore_process_event(void *opaque
         goto out;

tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 void xenstore_process_event(void *opaque · line 448

Quoted 10 lines

tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +6 -2

     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.

tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 void xenstore_process_event(void *opaque · line 455

Quoted 7 lines

tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +7 -0
+        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.

Quoted 13 lines

tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +9 -0

+        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;

tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 void xenstore_process_event(void *opaque · line 483

Quoted 8 lines

tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +3 -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().

Quoted 1 line

tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +0 -0
 }

Signature · 12 lines

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

Keir Fraser

2008-01-30 14:45 UTC #2 lore raw

Quoted 10 lines · Christoph Egger

tools/ioemu/xenstore.c void xenstore_process_event(void *opaque · line 483
Quoted 6 lines
out:
tools/ioemu/xenstore.c +3 -0
+    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

Christoph Egger

2008-01-31 08:36 UTC #3 lore raw

Quoted 8 lines · On Wednesday 30 January 2008 15:45:03 Keir Fraser wrote:

Quoted 7 lines · Christoph Egger
Quoted 6 lines
out:
tools/ioemu/xenstore.c +3 -0
+    free(drv);
+    free(buf);
+    free(bpath);
     free(image);
     free(vec);

Quoted 7 lines

Quoted 3 lines
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

Signature · 12 lines

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

Pat Campbell

author

2008-01-30 16:24 UTC #4 lore raw

In reply to Christoph Egger

Quoted 39 lines · Christoph Egger

Quoted 8 lines · 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', ]
Quoted 9 lines
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 <[email protected]>
That looks much better.
See inline for comments.
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 void xenstore_record_dm_state(char *stat · line 422
Quoted 11 lines
xen-blktab-subtype-strip.patch
diff -r 1c826ea72a80 tools/ioemu/xenstore.c
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +1 -1
@@ -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;
Quoted 2 lines
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +0 -0
     unsigned int len, num, hd_index;
 

Quoted 17 lines

Quoted 4 lines
     vec = xs_read_watch(xsh, &num);
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +0 -0
@@ -440,8 +440,28 @@ void xenstore_process_event(void *opaque
         goto out;
     hd_index = vec[XS_WATCH_TOKEN][2] - 'a';
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 void xenstore_process_event(void *opaque · line 449
Quoted 10 lines
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +6 -2
     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.

Quoted 10 lines

tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 void xenstore_process_event(void *opaque · line 457
Quoted 9 lines
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +7 -0
+        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) {
 

Quoted 2 lines

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.

Quoted 16 lines

Quoted 16 lines
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +9 -0
+        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;
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +0 -0
@@ -456,6 +476,9 @@ void xenstore_process_event(void *opaque
     }
 

Quoted 13 lines

tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 void xenstore_process_event(void *opaque · line 484
Quoted 7 lines
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +3 -0
  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 :-)

Quoted 5 lines

Quoted 2 lines
}

Thanks for taking the time.

Keir Fraser

2008-01-30 16:31 UTC #5 lore raw

I already checked in an appropriate fix following Christoph's comments.

-- Keir

Quoted 117 lines · Pat Campbell

Quoted 39 lines · Christoph Egger
Quoted 8 lines · 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', ]
Quoted 9 lines
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 <[email protected]>
That looks much better.
See inline for comments.
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 void xenstore_record_dm_state(char *stat · line 422
Quoted 11 lines
xen-blktab-subtype-strip.patch
diff -r 1c826ea72a80 tools/ioemu/xenstore.c
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +1 -1
@@ -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;
Quoted 2 lines
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +0 -0
     unsigned int len, num, hd_index;
 
Quoted 17 lines
Quoted 4 lines
     vec = xs_read_watch(xsh, &num);
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +0 -0
@@ -440,8 +440,28 @@ void xenstore_process_event(void *opaque
         goto out;
     hd_index = vec[XS_WATCH_TOKEN][2] - 'a';
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 void xenstore_process_event(void *opaque · line 449
Quoted 10 lines
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +6 -2
     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.
Quoted 10 lines
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 void xenstore_process_event(void *opaque · line 457
Quoted 9 lines
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +7 -0
+        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) {
 
Quoted 2 lines
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.
Quoted 16 lines
Quoted 16 lines
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +9 -0
+        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;
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +0 -0
@@ -456,6 +476,9 @@ void xenstore_process_event(void *opaque
     }
 
Quoted 13 lines
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 void xenstore_process_event(void *opaque · line 484
Quoted 7 lines
tools/ioemu/xenstore.c Wed Jan 30 07:06:12 2008 -0700 renamed from tools/ioemu/xenstore.c Wed Jan 23 15:42:52 2008 +0000 +3 -0
  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 :-)
Quoted 5 lines
Quoted 2 lines
}
Thanks for taking the time.
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel