Re: [Qemu-devel] [PATCH] vmdk: Allow vmdk_create to work with protocol

[Fam Zheng <famz@redhat.com>]

On 2013年12月19日 21:12, Stefan Hajnoczi wrote:
> On Tue, Dec 17, 2013 at 08:00:00PM +0800, Fam Zheng wrote:
>> @@ -1511,48 +1521,55 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
>>       header.check_bytes[3] = 0xa;
>>
>>       /* write all the data */
>> -    ret = qemu_write_full(fd, &magic, sizeof(magic));
>> -    if (ret != sizeof(magic)) {
>> -        ret = -errno;
>> +    ret = bdrv_pwrite(bs, 0, &magic, sizeof(magic));
>> +    if (ret < 0) {
>> +        error_set(errp, QERR_IO_ERROR);
>>           goto exit;
>>       }
>> -    ret = qemu_write_full(fd, &header, sizeof(header));
>> +    ret = bdrv_pwrite(bs, sizeof(magic), &header, sizeof(header));
>>       if (ret != sizeof(header)) {
>> +        error_set(errp, QERR_IO_ERROR);
>>           ret = -errno;
>
> This line should be deleted.
>
> Also, I noticed you changed ret != sizeof(magic) to ret < 0 for the
> magic number bdrv_pwrite() but did not change the condition for the
> header write.  Please keep the error handling condition consistent.
>

OK.

>>           goto exit;
>>       }
>>
>> -    ret = ftruncate(fd, le64_to_cpu(header.grain_offset) << 9);
>> +    ret = bdrv_truncate(bs, (le64_to_cpu(header.grain_offset)) << 9);
>
> Why add parentheses around le64_to_cpu()?
>

Unintended. Will remove.

>>       if (ret < 0) {
>> -        ret = -errno;
>> -        goto exit;
>> +        error_setg(errp, "Could not truncate file");
>
> goto exit?
>

Yes, thanks.

>>       }
>>
>>       /* write grain directory */
>> -    lseek(fd, le64_to_cpu(header.rgd_offset) << 9, SEEK_SET);
>> -    for (i = 0, tmp = le64_to_cpu(header.rgd_offset) + gd_size;
>> +    gd_buf_size = gd_sectors * BDRV_SECTOR_SIZE * sizeof(*gd_buf);
>> +    gd_buf = g_malloc0(gd_buf_size);
>> +    for (i = 0, tmp = le64_to_cpu(header.rgd_offset) + gd_sectors;
>>            i < gt_count; i++, tmp += gt_size) {
>> -        ret = qemu_write_full(fd, &tmp, sizeof(tmp));
>> -        if (ret != sizeof(tmp)) {
>> -            ret = -errno;
>> -            goto exit;
>> -        }
>
> Was this old code not endian-safe?  It appears to be writing native
> endian values.  The new code is different.
>

Yes, a bonus bug fix. I'll add note in the commit message.

>> @@ -1771,33 +1791,34 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>>                              total_size / (int64_t)(63 * number_heads * 512),
>>                              number_heads,
>>                              adapter_type);
>> -    if (split || flat) {
>> -        fd = qemu_open(filename,
>> -                       O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
>> -                       0644);
>> +    desc_len = strlen(desc);
>> +    /* the descriptor offset = 0x200 */
>> +    if (!split && !flat) {
>> +        desc_offset = 0x200;
>>       } else {
>> -        fd = qemu_open(filename,
>> -                       O_WRONLY | O_BINARY | O_LARGEFILE,
>> -                       0644);
>> +        ret = bdrv_create_file(filename, options, &local_err);
>
> Missing error handling if bdrv_create_file() fails.
>

OK.

>>       }
>> -    if (fd < 0) {
>> -        ret = -errno;
>> +    ret = bdrv_file_open(&new_bs, filename, NULL, BDRV_O_RDWR, &local_err);
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, -ret, "Could not write description");
>>           goto exit;
>>       }
>> -    /* the descriptor offset = 0x200 */
>> -    if (!split && !flat && 0x200 != lseek(fd, 0x200, SEEK_SET)) {
>> -        ret = -errno;
>> -        goto close_exit;
>> +    ret = bdrv_pwrite(new_bs, desc_offset, desc, desc_len);
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, -ret, "Could not write description");
>
> goto close_exit?
>

OK.

>>       }
>> -    ret = qemu_write_full(fd, desc, strlen(desc));
>> -    if (ret != strlen(desc)) {
>> -        ret = -errno;
>> -        goto close_exit;
>> +    /* bdrv_pwrite write padding zeros to align to sector, we don't need that
>> +     * for description file */
>> +    if (desc_offset == 0) {
>> +        ret = bdrv_truncate(new_bs, desc_offset + desc_len);
>
> We know desc_offset == 0, so desc_offset (0) + desc_len is really just
> desc_len.
>

OK.

Thanks for the review!

Fam