Re: [Qemu-devel] [PATCH V19 07/10] libqblock: libqblock API design and type defines

[Wenchao Xia]

> > + */
> > +#define QBLOCK_PROT_OPTIONS_UNION_SIZE (512/sizeof(QBlockReserved))
> > +typedef struct QBlockLocationInfo {
> > +    QBlockProtocol prot_type;
> > +    union {
> > +        QBlockProtocolOptionsFile o_file;
> > +        QBlockReserved union_reserved[QBLOCK_PROT_OPTIONS_UNION_SIZE];
> > +    };
> > +    QBlockReserved reserved[LIBQBLOCK_RESERVED_SIZE];
>
> Do you really mean to reserve trailing bytes after what you have already
> reserved as part of the union?  Theoretically, the reserved space in the
> union seems like it would be enough.
  I think it is necessary to reserve space for every layer in the public
structure, to make sure ABI is OK.

> > +
> > +typedef struct QBlockFormatOptionsQCOW2 {
> > +    QBlockLocationInfo backing_loc;
> > +    QBlockFormat backing_fmt;
> > +    bool encrypt;
> > +    uint64_t cluster_size; /* unit is bytes */
> > +    QBlockFormatOptionsQCOW2CompatLv cpt_lv;
> > +    QBlockFormatOptionsQCOW2PreAlloc pre_mode;
> > +    QBlockReserved reserved[LIBQBLOCK_RESERVED_SIZE];
> > +} QBlockFormatOptionsQCOW2;
>
> Side note: Libvirt is leaning towards presenting compat-level 1.1 to the
> user via the name 'qcow3', than it is to try and teach the users about
> the difference between qcow2 (compat 1.0) and qcow2v3 (compat 1.1) all
> under the name qcow2.  Even though the qcow2 code is able to handle both
> compatibility levels under the hood in qemu source code, it seems less
> confusing if the user interface separates things with a 'qcow3' naming
> convention.  I don't know how much that should impact your design, though.
 thanks for tipping that. I am not sure whether qcow2V3 equals qcow3, I
hope keep it as option of qcow2 for this version, and add QCOW3 if it is
finally decided as a new version later, just to make things simpler.

> > +
> > +#define QBLOCK_FMT_OPTIONS_UNION_SIZE (QBLOCK_PROT_OPTIONS_UNION_SIZE*2)
>
> Spaces around binary operators.
OK.

> > +/**
> > + * QBlockStaticInfo: information about the block image.
> > + *
> > + * @loc: location information.
> > + * @fmt: format information.
> > + * @sector_size: how many bytes in a sector, it is 512 usually.
>
> I'd drop ', it is 512 usually', since in the future, we might want to
> default to 4096 in new images.
  OK.

> > +/**
> > + * qb_create: create a block image or object.
> > + *
> > + * Note: Create operation would not open the image automatically.
> > + *
> > + * return 0 on success, libqblock negative error value on fail.
> > + *
> > + * @qbi: pointer to QBlockImage, used to get error message.
> > + * @loc: location options for open, how to find the image.
> > + * @fmt: format options, how to extract the data.
> > + * @flag: behavior control flags, LIBQBLOCK_O_XXX's combination, it is used if
> > + *  backing file need to be opened..
>
> s/.././
OK.

> > +
> > +/* advance image APIs */
> > +/**
> > + * qb_check_allocation: check if [start, start+length-1] was allocated on the
> > + *  image.
> > + *
> > + * return 0 on success, libqblock negative error value on fail.
> > + *
> > + * @qbi: pointer to QBlockImage.
> > + * @start: start position, unit is byte.
> > + * @length: length to check, unit is byte, max is (INTMAX-1)*sector_size. If it
>
> INT_MAX, not INTMAX.
>
> Might be worth pointing to the API for determining sector_size, since it
> is not a parameter in this function call.  Furthermore, since all the
> relevant parameters are uint64_t, why is there an INT_MAX limitation in
> the first place?  Even if underlying qemu code has a limitation at 2
> gigabytes per iteration, can't the libqblock interface iterate as many
> times as needed to reach the user's desired 64-bit length?
  make sense.

> > + *   is too big, QB_ERR_INVALID_PARAM will be returned.
> > + * @pstatus: pointer to receive the status, 1 means allocated,
> > + *  0 means unallocated.
> > + * @plength: pointer to receive the length that all have the same status as
> > + *  *pstatus.
> > + *
> > + * Note: after return, start+*plength may have the same status as
> > + *  start+*plength-1.
> > + */
> > +QEMU_DLL_PUBLIC
> > +int qb_check_allocation(QBlockImage *qbi,
> > +                        uint64_t start,
> > +                        uint64_t length,
> > +                        int *pstatus,
> > +                        uint64_t *plength);


--
Best Regards

Wenchao Xia