Re: [PATCH][ioemu] strip tap subtype prefix from image name (restyled)

[Pat Campbell <plc@novell.com>]

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.