[lvm-devel] [PATCH 5/8] Add lvm_vg_get_property() generic vg property function.

[Petr Rockai]

Hi,

Dave Wysochanski <dwysocha at redhat.com> writes:
>> A is_integer might be useful here, for symmetry. In the client code, it
>> might not be obvious that !p.is_string means that it is an integer. It
>> would also improve API extensibility for the future, may we ever run
>> into a property that's neither string nor integer (although it won't
>> help with ABI).
>> 
>
> Ok, I've implemented this - easy.  But...
> Now that I think about it, rather than a bitfield and 'is_string' and
> 'is_integer", should we have a 'type' field and leave this an unsigned
> (for example, #define LVM_PROPERTY_TYPE_NUM 1 and #define
> LVM_PROPERTY_TYPE STR 2) or at least should we add padding to this
> struct?  The struct might warrant a bit more thought, given how much
> we've changed it recently, and given the ABI implications.

The advantage of the bitfield is that it is much more convenient to use
(and the resulting code more concise and clearer). It also does not
impose any ABI problems as long as you keep the items in the right
order. What can be done to ensure forward compatibility is this:

uint32_t foo:1;
uint32_t bar:1;
uint32_t _padding:30;

And when you need a new flag, just reduce the padding by one and add
your new flag between the existing flags and the padding.

> As the code is, the client does not need to check lvm_errno() unless it
> cares what went wrong.  In essence, v.is_valid "should =" !lvm_errno()
> today, though there's no explicit assignment.  However, if we explicitly
> assign v.is_valid = !lvm_errno(), then this assumes all error paths will
> properly set lvm_errno().  Otherwise, we may end up telling the user the
> value is valid when it may not be.  Now this case is clearly a bug but
> keeping them separate avoids the potential bug.  It's fairly clear right
> now that all error paths do properly set lvm_errno(), so this assignment
> would be safe today.
Ok.

Yours,
   Petr.