Re: [RFC Patch v4 9/9] update libxl__device_disk_from_xs_be() to support blktap device

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

On 09/22/2014 11:08 PM, Ian Campbell wrote:
> On Mon, 2014-09-22 at 13:59 +0800, Wen Congyang wrote:
>> If the disk backend is blktap device, we store "format:pdev_path"
>> in tapdisk-params, and store "phy" in type. So use tapdisk-params
>> to set libxl_device_disk instead of params and type.
> 
> This seems to be the same format as params -- can you not just
> dynamically select the key name and share the parsing code?

OK. I will try it.

> 
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
>> ---
>>  tools/libxl/libxl.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index aa9345b..8c241aa 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -2468,6 +2468,45 @@ static int libxl__device_disk_from_xs_be(libxl__gc *gc,
>>          goto cleanup;
>>      }
>>  
>> +    disk->format = LIBXL_DISK_FORMAT_UNKNOWN;
>> +
>> +    /* "tapdisk-params" is only for tapdisk */
>> +    tmp = xs_read(ctx->xsh, XBT_NULL,
>> +                  libxl__sprintf(gc, "%s/tapdisk-params", be_path), &len);
> 
> libxl__xs_read will gc the result, which will make the error handling
> easier (you strdup tmp so I think the reasons for using xs_read don't
> apply). Also libxl__sprintf can be shortended with GCSPRINTF.

Hmm, I agree with it, and I think we should also the similar codes.

> 
>> +        if (disk->format != LIBXL_DISK_FORMAT_VHD &&
>> +            disk->format != LIBXL_DISK_FORMAT_RAW) {
>> +            LOG(ERROR, "unsupported tapdisk format: %s\n", tmp);
> 
> Hrm, if we get here then is there any reason to reject this? obviously
> something wrote it etc.

Hmm, we can get here only when the xenstore node is corrupted.

> 
> This function is supposed to read the current state and return it, I'm
> not sure it should be rejecting what it finds TBH, and it would simplify
> things not to do so.

The xenstore node is corrupted, what should we do? Ignore this disk or return
error?

> 
>> +            free(tmp);
>> +            goto cleanup;
>> +        }
>> +        free(tmp);
>> +
>> +        /*
>> +         * The backend is tapdisk, so we store tapdev in params, and
>> +         * phy in type(see device_disk_add())
>> +         */
>> +        disk->backend = LIBXL_DISK_BACKEND_TAP;
>> +
>> +        goto skip_type;
> 
> I think you want to put some all all of the following into an else
> clause, not simulate that affect with a goto.

OK

> 
>> @@ -2520,8 +2560,6 @@ static int libxl__device_disk_from_xs_be(libxl__gc *gc,
>>      }
>>      disk->is_cdrom = !strcmp(tmp, "cdrom");
>>  
>> -    disk->format = LIBXL_DISK_FORMAT_UNKNOWN;
> 
> Why does this need to move? Also I think UNKNOWN is the fault from
> libxl_device_disk_init, so it's probably unnecessary.

Yes.

Thanks
Wen Congyang

> 
> Ian.
> 
> .
>