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

[Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>]

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

 -- Keir

On 30/1/08 16:24, "Pat Campbell" <plc@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>     
>> 
>> 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@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel