On 07/14/2014 03:09 AM, Michal Privoznik wrote:
> On 11.07.2014 18:59, Eric Blake wrote:
>> On 07/11/2014 03:32 AM, Michal Privoznik wrote:
>>> There's this trend in libvirt of using enum types wherever possible.
>>> Now that I'm at virSecurityLabelDef let's rework @type item of the
>>> structure so we don't have to typecast it elsewhere.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>>> ---
>>> src/conf/domain_conf.c | 6 ++++--
>>> src/security/security_dac.c | 2 +-
>>> src/util/virseclabel.h | 2 +-
>>> 3 files changed, 6 insertions(+), 4 deletions(-)
>>
>> I'm not quite as sure about this one. This solves the issue of how to
>> detect errors when parsing strings to enum, but required the use of an
>> intermediate variable which in turn made the patch a net gain in lines
>> of code. If someone forgets to use the intermediate variable for
>> parsing, this backfires. On the other hand, parsing string to enum
>> should be done in just one location, and that's the location touched by
>> this patch. I'm 50-50 on whether to take this, so I'd like someone else
>> to chime in with an opinion.
>
> I hear you. This patch is just a refactor. It does not add anything useful nor solve any issue. It's okay if dropped. But the more I think about our vir*TypeFromString() the more I feel we should do something about it. How about making it follow our typical function return pattern:
>
> int func() {
>
> virMyFavourite x;
> const char *string;
>
> if (virMyFavouriteTypeFromString(string, &x) < 0) {
> virReportError("unknown value: %s", string);
> goto error;
> }
Yeah, making the API failsafe by separating error return from value
setting is what we've done elsewhere (such as virStrToLong_*). It's a
bigger, more invasive change, but may be worth it. And if we do it, we
can then start sticking enum values inside non-public structs.
>
> That is, we need this diff:
>
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 95d1ff9..40075e9 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -407,15 +407,20 @@ virParseVersionString(const char *str, unsigned long *version,
>
> int virEnumFromString(const char *const*types,
> unsigned int ntypes,
> - const char *type)
> + const char *type,
> + int *val)
> } \
> - int name ## TypeFromString(const char *type) { \
> + int name ## TypeFromString(const char *type, name *val) { \
> return virEnumFromString(name ## TypeList, \
> ARRAY_CARDINALITY(name ## TypeList), \
> - type); \
> + type, (int *) val); \
> }
>
> # define VIR_ENUM_DECL(name) \
>
> And then a tons of follow up patches. Or even make virEnumString() report the error (that could save a lot of virReportError() calls).
Or even make the error reporting an optional bool parameter.
It's also a huge patch; I wonder if we can come up with an alternative
name (VIR_ENUM_DECL2 expands to name ## FromString, leaving
VIR_ENUM_DECL expanding to name ## TypeFromString - also nice that it's
a shorter name since it's usage will be longer), where we can then focus
on just a few conversions per patch, until all patches are using the new
name and finally delete the old name, rather than redefining
VIR_ENUM_DECL up front and having to do all the refactoring in a single
patch.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list