Re: [RFC Patch v4 8/9] store correct format into tapdisk-params/params

[Wen Congyang <wency@cn.fujitsu.com>]

On 09/22/2014 10:37 PM, Ian Campbell wrote:
> On Mon, 2014-09-22 at 13:59 +0800, Wen Congyang wrote:
> 
> Please prefix the subject "tools: libxl: "

OK

> 
>> If the format is raw, we store aio into tapdisk-params/params.
> 
> I got confused and thought "tapdisl-params/params" was a path. Could you
> spell it out the first time as "into either the tapdisk-params or params
> xenstore node" please.

OK

> 
>> And we cannot use the API libxl_disk_format_from_string()
>> to get the format from the string.
> 
> ... can you spell out why/where this is important please.

The API libxl_device_disk_list() will return all disks. But the
type is wrong if it is a tapdisk device.

> 
>> The API libxl__device_disk_string_of_format() is just
>> for blktap2, which needs to pass aio instead of raw to
>> tapdisk2.
> 
> Should we move it to libxl_blktap2.c then?

I agree with it.

> 
>>  So use libxl_disk_format_to_string() to
>> instead of libxl__device_disk_string_of_format().
>>
>> Also update libxl__device_destroy_tapdisk() due to
>> tapdisk-params changed.
> 
> s/changed/change/.

OK

> 
>> Note: the content of tapdisk-params/params has been changed...
> 
> AFAICT the actual change is that "aio:/path/to/a/thing" might become
> either "raw:/path/to/a/thing" or "empty:/path/to/a/thing", or perhaps
> even "unknown:/path/to/a/thing" (or more likely "aio:"->"empty:").

Yes

> 
> Did you test blktap2 and qemu to be sure that they handle "raw" in
> particular correctly?

Sorry, I don't understand what do you want to test.

> 
>> @@ -67,6 +68,15 @@ int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *params)
>>  
>>      *disk++ = '\0';
>>  
>> +    /* type may be raw */
>> +    rc = libxl_disk_format_from_string(type, &format);
>> +    if (rc < 0) {
>> +        LOG(ERROR, "invalid disk type %s", type);
>> +        return ERROR_FAIL;
> 
> Please propagate rc.

OK

Thanks
Wen Congyang

> 
>> +    }
>> +
>> +    type = libxl__device_disk_string_of_format(format);
>> +
>>      err = tap_ctl_find(type, disk, &tap);
>>      if (err < 0) {
>>          /* returns -errno */
> 
> 
> .
>