Re: [Qemu-devel] [PATCH 1/6] libqblock APIs

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

     Thank u for the careful reviewing of my codes, I will write down
the typo errors you mentioned on a note.

> On 09/03/2012 03:18 AM, Wenchao Xia wrote:
>>    This patch contains the major APIs in the library.
>> Important APIs:
>>    1 QBroker. These structure was used to retrieve errors, every thread must
>> create one first, Later maybe thread related staff could be added into it.
>>    2 QBlockState. It stands for an block image object.
>>    3 QBlockInfoImageStatic. Now it is not folded with location and format.
>>    4 ABI was kept with reserved members.
>>
>> structure, because it would cause caller more codes to find one member.
>
> Commit message snafu?
>
  a wrong paste, sorry.

>> +/**
>> + * libqblock_init: Initialize the library
>> + */
>> +void libqblock_init(void);
>
> Is this function safe to call more than once?  Even tighter, is it safe
> to call this function simultaneously from multiple threads?
>
   No, it should be only called once, any other thread should not call
it again, will document it. About the multiple thread user case, qemu
block layer can't support that now, will fix that later.

>> +
>> +/**
>> + * qb_broker_new: allocate a new broker
>> + *
>> + * Broker is used to pass operation to libqblock, and got feed back from it.
>
> s/got feed back/get feedback/
>
>> + *
>> + * Returns 0 on success, negative value on fail.
>
> Is there any particular interpretation to this negative value, such as
> negative errno constant, or always -1?
>
   Yes, negative values are defined with macros in the header file.

>> +
>> +/**
>> + * qb_state_new: allocate a new QBloctState struct
>
> s/Bloct/Block/
>
>> + *
>> + * Following qblock action were based on this struct
>
> Didn't read well.  Did you mean:
>
> Subsequent qblock actions will use this struct
>
   Yes.

>> + *
>> + * Returns 0 if succeed, negative value on fail.
>
> Again, is there any particular meaning to which negative value is returned?
>
>> +
>> +/**
>> + * qb_open: open a block object.
>> + *
>> + * return 0 on success, negative on fail.
>> + *
>> + * @broker: operation broker.
>> + * @qbs: pointer to struct QBlockState.
>> + * @loc: location options for open, how to find the image.
>> + * @fmt: format options, how to extract the data, only valid member now is
>> +     fmt->fmt_type, set NULL if you want auto discovery the format.
>
> set to NULL if you want to auto-discover the format
>
> Maybe also add a warning about the inherent security risks of attempting
> format auto-discovery (any raw image must NOT be probed, as the raw
> image can emulate any other format and cause qemu to chase down chains
> where it should not).
>
   it seems qemu-img could find out that an image is raw correctly by
probing, do you mean give a warning saying that this image is probably
some formats that qemu do not supported, such as virtual box's image?

>> + * @flag: behavior control flags.
>
> What flags are currently defined?
>
   It is the flags defined in the header file, such as LIBQBLOCK_O_RDWR,
will document it.

>> + */
>> +int qb_open(struct QBroker *broker,
>> +            struct QBlockState *qbs,
>> +            struct QBlockOptionLoc *loc,
>> +            struct QBlockOptionFormat *fmt,
>> +            int flag);
>> +
>> +/**
>> + * qb_close: close a block object.
>> + *
>> + * qb_flush is automaticlly done inside.
>
> s/automaticlly/automatically/
>
>> +/**
>> + * qb_create: create a block image or object.
>> + *
>> + * Note: Create operation would not open the image automatically.
>> + *
>> + * return negative on fail, 0 on success.
>> + *
>> + * @broker: operation broker.
>> + * @qbs: pointer to struct QBlockState.
>> + * @loc: location options for open, how to find the image.
>> + * @fmt: format options, how to extract the data.
>> + * @flag: behavior control flags.
>
> Again, what flags are defined?
>
>> +
>> +/* sync access */
>> +/**
>> + * qb_read: block sync read.
>> + *
>> + * return negative on fail, 0 on success.
>
> Shouldn't this instead return the number of successfully read bytes, to
> allow for short reads if offset exceeds end-of-file?  If so, should it
> return ssize_t instead of int?
>
   I had this idea before too, let'me check if qemu block layer can
provide this functionality,

>> + *
>> + * @broker: operation broker.
>> + * @qbs: pointer to struct QBlockState.
>> + * @buf: buffer that receive the content.
>
> s/receive/receives/
>
>> + * @len: length to read.
>
> Is there a magic length for reading the entire file in one go?
>
   no, if so where should I put with the result.

>> + * @offset: offset in the block data.
>> + */
>> +int qb_read(struct QBroker *broker,
>> +            struct QBlockState *qbs,
>> +            const void *buf,
>> +            size_t len,
>> +            off_t offset);
>
> You do realize that size_t and off_t are not necessarily the same width;
> but I'm okay with limiting to size_t reads.
>
>> +/**
>> + * qb_write: block sync write.
>> + *
>> + * return negative on fail, 0 on success.
>
> Again, this should probably return number of successfully written bytes,
> as an ssize_t.
>
>> + *
>> + * @broker: operation broker.
>> + * @qbs: pointer to struct QBlockState.
>> + * @buf: buffer that receive the content.
>
> s/receive/supplies/
>
>> +/* advance image APIs */
>> +/**
>> + * qb_is_allocated: check if following sectors was allocated on the image.
>> + *
>> + * return negative on fail, 0 or 1 on success. 0 means unallocated, 1 means
>> + *allocated.
>
> Formatting is off.
>
   coming later.

>> + *
>> + * @broker: operation broker.
>> + * @qbs: pointer to struct QBlockState.
>> + * @sector_num: start sector number. If 'sector_num' is beyond the end of the
>> + *disk image the return value is 0 and 'pnum' is set to 0.
>> + * @nb_sectors: how many sector would be checked, it is the max value 'pnum'
>> + *should be set to.  If nb_sectors goes beyond the end of the disk image it
>> + *will be clamped.
>> + * @pnum: pointer to receive how many sectors are allocated or unallocated.
>
> This interface requires the user to know how big a sector is.  Would it
> be any more convenient to the user to pass offsets, rather than sector
> numbers; and/or have a function for converting between offsets and
> sector numbers?
>
   OK, I need a function returning how big a sector is in an image.

>> + */
>> +int qb_is_allocated(struct QBroker *broker,
>> +                    struct QBlockState *qbs,
>> +                    int64_t sector_num,
>> +                    int nb_sectors,
>
> Shouldn't nb_sectors be size_t?
>
>> +                    int *pnum);
>
> Exactly how does the *pnum argument work?  This interface looks like it
> isn't fully thought out yet.  Either I want to know if a chunk of
> sectors is allocated (I supply start and length of sectors to check),
> regardless of how many sectors beyond that point are also allocated
> (pnum makes no sense); or I want to know how many sectors are allocated
> from a given point (I supply start, and the function returns length, so
> nb_sectors makes no sense).  Either way, I think you are supplying too
> many parameters for how I envision checking for allocated sectors.
>


-- 
Best Regards

Wenchao Xia