Re: [Qemu-devel] [PATCH V4 2/5] libqblock type defines

[Wenchao Xia]

于 2012-9-27 18:16, Paolo Bonzini 写道:
> Il 27/09/2012 11:52, Wenchao Xia ha scritto:
> > > Please use QBO_ instead of QB_ throughout.  Also write COMPAT instead of
> > > CPT, and remove CPT_NONE since 0.10 is the default:
> >    __NONE is used to indicate whether this property is set or get, so
> > it is actually have 3 status than just 2: Not set, V010, V110. It
> > is the same reason that I use __NONE in bool values, especially __NONE
> > could represent it is not got in information retrieving.
>
> Yes, that I guessed.  I thought in many cases we can anticipate that the
> default is not going to change, but perhaps it's better to be safe
> (which is what you did).
>
> Please do change FALSE/TRUE to OFF/METADATA for preallocation enums, and
> please remove MONOLITHIC from QBO_FMT_VMDK_SUBFMT_MONOLITHIC_NONE.
  OK, these should be changed.

> > Maybe I should rename them to __NOTSET.
>
> Perhaps "_DEFAULT" is even bette?
>
> Paolo
  Yep, _DEFAULT seems good to me.

> > > > +typedef struct QBlockStaticInfoAddr {
> > > > +    uint64_t *virt_size;
> > > > +    QBlockProtInfo *backing_loc;
> > > > +    bool *encrypt;
> > > > +} QBlockStaticInfoAddr;
> > >
> > > Why the indirection?
> >    It helps user to get these important members, otherwise
> > user will need
> > Switch (fmt) {
> >    case RAW:
> >      ....
> >    case QCOW2:
> >      ...
> > }
> > for every attribute. The indirection address will let user directly
> > access the members.
>
> Ah, ok, now I understand.  Interesting.  An alternative could be to add
> generic accessors like these:
>
> uint64_t qblock_get_virt_size(QBlockFmtInfo *fmt);
> QBlockProtInfo *qblock_get_backing_loc(QBlockFmtInfo *fmt);
> bool qblock_get_encrypt(QBlockFmtInfo *fmt);
>
> etc. that include the switch statement.
  I think it is reasonable use generic accessors instead of embbed
structure, maybe an additional helper function should be added as:

QBlockStaticInfoAddr *qblock_get_static_info_addr(QBlockStaticInfo
 *info)

  In this way only one switch statement is needed in source code,
and only one new API is added.

> Paolo


--
Best Regards

Wenchao Xia