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

[Stefan Hajnoczi]

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"? :)

> > Kevin: Does the block layer do short reads/writes?
> 
> No, it doesn't.

Great, then we can use int return values where 0 means success and
-errno is used for errors.  No need to return the number of bytes from
the read/write functions.

> 
> >>>> +/**
> >>>> + * 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 guess it's nice to keep the structs QAPI compatible in case we want to
convert to QAPI later.

Stefan