On Wed, Nov 26, 2014 at 11:32 PM, Stefan Hajnoczi <stefa...@gmail.com>
wrote:
> On Thu, Nov 06, 2014 at 10:43:50PM +0800, Xiaodong Gong wrote:
> > + } else if (platform == PLATFORM_W2RU) {
> > + /* Must be UTF16-LE to ASCII */
> > + char *out, *optr;
> > + int j;
> > +
> > + optr = out = (char *) g_malloc(data_length + 1);
> > + if (out == NULL) {
> > + ret = -1;
> > + return ret;
> > + }
> > +
> > + for (j = 0; j < data_length + 1; j++) {
> > + out[j] = bs->backing_file[2 * j];
> > + }
> > + out[data_length + 1] = '\0';
> > +
> > + while (*optr != '\0') {
> > + if (*optr == '\\') {
> > + *optr = '/';
> > + }
> > + optr++;
> > + }
> > +
> > + strncpy(bs->backing_file, out, data_length + 1);
> > +
> > + g_free(out);
> > + out = NULL;
> > +
> > + done = true;
> > + }
>
> Please convert from UTF-16 LE to the local file system character set:
> https://developer.gnome.org/glib/stable/glib-Character-Set-Conversion.html
>
> Also, using ->backing_file[] when the data is UTF-16 LE encoded is not
> ideal since it halves the maximum size of the string! It would be
> better to read into a temporary buffer that is 2 *
> sizeof(backing_file[]) big before writing into ->backing_file[].
>
Okay, I'll use iconv() to convert UTF-16LE and UTF-8 to ASCII. why g_iconv
in
glib ? And the data_length is the total length of location, 2 *
sizeof(backing_file[]) is no need.
> > @@ -286,6 +411,37 @@ static int vpc_open(BlockDriverState *bs, QDict
> *options, int flags,
> > s->free_data_block_offset =
> > (s->bat_offset + (s->max_table_entries * 4) + 511) & ~511;
> >
> > + /* Read tdbatmap header by offset */
> > + if (footer->version >= VHD_VERSION(1, 2)) {
>
> Missing be32_to_cpu(footer->version)
>
> Okay
> +static int vpc_write(BlockDriverState *bs, int64_t sector_num,
> > + const uint8_t *buf, int nb_sectors)
> > +{
> > + BDRVVPCState *s = bs->opaque;
> > + VHDFooter *footer = (VHDFooter *) s->footer_buf;
> > + int64_t sectors_per_block = s->block_size >> BDRV_SECTOR_BITS;
> > + int64_t offset, sectors;
> > + bool diff = true;
> > + int ret = 0;
> > +
> > + switch (be32_to_cpu(footer->type)) {
> > + case VHD_FIXED:
> > + return bdrv_write(bs->file, sector_num, buf, nb_sectors);
> > + case VHD_DYNAMIC:
> > + case VHD_DIFF:
> > + if (be32_to_cpu(footer->type) == VHD_DYNAMIC) {
> > + diff = false;
> > }
>
> This can be done with a fall-through case instead of checking
> footer->type again.
> case VHD_DYNAMIC:
> diff = false;
> /* fall-through */
> case VHD_DIFF:
>
>
Okay, this is more clear
> > + ret = bdrv_pwrite(bs->file, offset, buf,
> > + sectors * BDRV_SECTOR_SIZE);
> > + if (ret != sectors * BDRV_SECTOR_SIZE) {
> > + return -1;
> > + }
> >
> > - nb_sectors -= sectors;
> > - sector_num += sectors;
> > - buf += sectors * BDRV_SECTOR_SIZE;
> > - }
> > + if (diff) {
> > + ret = write_bitmap(bs, sector_num, sectors);
> > + if (ret < 0) {
> > + return -1;
> > + }
> > + }
> >
> > - return 0;
> > + nb_sectors -= sectors;
> > + sector_num += sectors;
> > + buf += sectors * BDRV_SECTOR_SIZE;
> > + }
> > + break;
> > + default:
> > + return -1;
> > + }
> > + return ret;
>
> In the VHD_DYNAMIC case we must *not* return the number of bytes from
> bdrv_pwrite(). Should this be return 0?
>
It is 0 originally, I' ll change it to back. But ret of Fixed format is
not 0 and
> 0 has no side effect.
