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

[Paolo Bonzini]

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. :)

>>>>>> +/**
>>>>>> + * 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...

> 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.

Paolo