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

[Wenchao Xia]

  Adding pad for every public structure seems the easiest way now,
I'll do that in next version.
> Il 05/02/2013 13:39, Stefan Hajnoczi ha scritto:
> > On Tue, Feb 05, 2013 at 10:56:09AM +0100, Paolo Bonzini wrote:
> > > Il 01/02/2013 10:32, Stefan Hajnoczi ha scritto:
> > > > > > What is the relationship between QBlockImage and its context?  Can I
> > > > > > have two contexts A and B like this:
> > > > > >
> > > > > > qb_image_ref(ctx_a, &qbi);
> > > > > > qb_image_unref(ctx_b, &qbi);
> > > > > >
> > > > > > Is this okay?
> > > > >    it should be OK if block layer is thread safe in the future,
> > > > > a thread should own a context. But caller may need to make sure
> > > > > every context should call ref/unref as pairs.
> > > >
> > > > Hmm...I still don't understand the relationship between QBlockImage and
> > > > a context.  Is it safe to use a QBlockImage with context B if it was
> > > > created with context A?  This should be documented.
> > >
> > > No, it shouldn't.
> >
> > I guess you mean "It is not safe to use a QBlockImage with context B if
> > it was created with context A" rather than "No, it shouldn't be
> > documented"? :)
>
> Indeed. :)
  About the qbcontext, there are two choices:
1 discard of qbcontext, adding *error to get error message
for every API. Need to confirm how to make it work
with glib's aio-context.
2 keep qbcontext, every thread use it to store its thread
specific data, such as errors, and it is easy to package
glib's multi-thread contents inside.
  Which way do you like better?


> > > > > > > +/**
> > > > > > > + * qb_formattype2str: translate libqblock format enum type to a string.
> > > > > > > + *
> > > > > > > + * return a pointer to the string, or NULL if type is not supported, and
> > > > > > > + *  returned pointer must NOT be freed.
> > > > > > > + *
> > > > > > > + * @fmt: the format enum type.
> > > > > > > + */
> > > > > > > +QEMU_DLL_PUBLIC
> > > > > > > +const char *qb_formattype2str(QBlockFormat fmt_type);
> > > > > >
> > > > > > Why does the QBlockFormat enum exist?  Is there an issue with using strings?
> > > > >    These fmt/enum code will always exist in an application to handler
> > > > > different format, the choice is whether libqblock handle it for
> > > > > the application. This is more a choice, my idea do it for user. What
> > > > > is your idea?
> > > >
> > > > Always use the strings ("qcow2", "raw", etc).  strcmp() on a 4 or 5-byte
> > > > string is very cheap and eliminates the need to convert between strings
> > > > and enums.  Dropping the enum also means one less thing to update when a
> > > > new image format is added.
> > >
> > > Hmm, I've never seen discriminated records with strings.  When working
> > > on the API with Wenchao, I tried to give it a QAPI flavor.
> >
> > My thinking is that converting back and forth between string and enum is
> > a chore.  If we can reduce boilerplate the library becomes more pleasant
> > to use.
>
> I hope/think in most case the user won't really need to deal with
> format-specific options...
  Exposing the strings to user make libqblock thiner and easier, while
using enum instead of string, resulting a nicer API. The best thing I
can image is let qemu block layer use enum also to eliminate the
converting later, but effort may be not little. However, enum style
stands in the right direction, problem is the implemention, but it can
be hided inside, so I think it is OK.


> > I guess it's nice to keep the structs QAPI compatible in case we want to
> > convert to QAPI later.
>
> Yeah, or at least share idioms.
  Any special requirement in struct define to make it compatiable with
QAPI?


> Paolo


--
Best Regards

Wenchao Xia