From: Fabiano Fidêncio <fidencio@redhat.com>
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org>
---
src/vmx/vmx.c | 196 ++++++++++++++++++++++------------------------------------
1 file changed, 73 insertions(+), 123 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index df6a58a474..54542c29a6 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -720,41 +720,36 @@ virVMXConvertToUTF8(const char *encoding, const char *string)
}
-
static int
-virVMXGetConfigString(virConfPtr conf, const char *name, char **string,
- bool optional)
+virVMXGetConfigStringHelper(virConfPtr conf, const char *name, char **string,
+ bool optional)
{
- virConfValuePtr value;
-
+ int result;
*string = NULL;
- value = virConfGetValue(conf, name);
- if (value == NULL) {
- if (optional)
- return 0;
+ result = virConfGetValueString(conf, name, string);
+ if (result == 1 && *string != NULL)
+ return 1;
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Missing essential config entry '%s'"), name);
- return -1;
- }
+ if (optional)
+ return 0;
- if (value->type != VIR_CONF_STRING) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Config entry '%s' must be a string"), name);
- return -1;
- }
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Missing essential config entry '%s'"), name);
+ return -1;
+}
- if (value->str == NULL) {
- if (optional)
- return 0;
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Missing essential config entry '%s'"), name);
+static int
+virVMXGetConfigString(virConfPtr conf, const char *name, char **string,
+ bool optional)
+{
+ *string = NULL;
+
+ if (virVMXGetConfigStringHelper(conf, name, string, optional) < 0)
return -1;
- }
- return VIR_STRDUP(*string, value->str);
+ return 0;
}
@@ -763,43 +758,26 @@ static int
virVMXGetConfigUUID(virConfPtr conf, const char *name, unsigned char *uuid,
bool optional)
{
- virConfValuePtr value;
+ char *string = NULL;
+ int result;
- value = virConfGetValue(conf, name);
+ result = virVMXGetConfigStringHelper(conf, name, &string, optional);
+ if (result <= 0)
+ return result;
- if (value == NULL) {
- if (optional) {
- return 0;
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Missing essential config entry '%s'"), name);
- return -1;
- }
- }
-
- if (value->type != VIR_CONF_STRING) {
+ if (virUUIDParse(string, uuid) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Config entry '%s' must be a string"), name);
- return -1;
+ _("Could not parse UUID from string '%s'"), string);
+ result = -1;
+ goto cleanup;
}
- if (value->str == NULL) {
- if (optional) {
- return 0;
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Missing essential config entry '%s'"), name);
- return -1;
- }
- }
+ result = 0;
- if (virUUIDParse(value->str, uuid) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not parse UUID from string '%s'"), value->str);
- return -1;
- }
+ cleanup:
+ VIR_FREE(string);
- return 0;
+ return result;
}
@@ -808,47 +786,35 @@ static int
virVMXGetConfigLong(virConfPtr conf, const char *name, long long *number,
long long default_, bool optional)
{
- virConfValuePtr value;
+ char *string = NULL;
+ int result;
*number = default_;
- value = virConfGetValue(conf, name);
- if (value == NULL) {
- if (optional) {
- return 0;
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Missing essential config entry '%s'"), name);
- return -1;
- }
- }
+ result = virVMXGetConfigStringHelper(conf, name, &string, optional);
+ if (result <= 0)
+ return result;
- if (value->type == VIR_CONF_STRING) {
- if (value->str == NULL) {
- if (optional) {
- return 0;
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Missing essential config entry '%s'"), name);
- return -1;
- }
- }
+ if (STRCASEEQ(string, "unlimited")) {
+ *number = -1;
+ result = 0;
+ goto cleanup;
+ }
- if (STRCASEEQ(value->str, "unlimited")) {
- *number = -1;
- } else if (virStrToLong_ll(value->str, NULL, 10, number) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Config entry '%s' must represent an integer value"),
- name);
- return -1;
- }
- } else {
+ if (virStrToLong_ll(string, NULL, 10, number) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Config entry '%s' must be a string"), name);
- return -1;
+ _("Config entry '%s' must represent an integer value"),
+ name);
+ result = -1;
+ goto cleanup;
}
- return 0;
+ result = 0;
+
+ cleanup:
+ VIR_FREE(string);
+
+ return result;
}
@@ -857,49 +823,33 @@ static int
virVMXGetConfigBoolean(virConfPtr conf, const char *name, bool *boolean_,
bool default_, bool optional)
{
- virConfValuePtr value;
+ char *string = NULL;
+ int result;
*boolean_ = default_;
- value = virConfGetValue(conf, name);
- if (value == NULL) {
- if (optional) {
- return 0;
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Missing essential config entry '%s'"), name);
- return -1;
- }
- }
+ result = virVMXGetConfigStringHelper(conf, name, &string, optional);
+ if (result <= 0)
+ return result;
- if (value->type == VIR_CONF_STRING) {
- if (value->str == NULL) {
- if (optional) {
- return 0;
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Missing essential config entry '%s'"), name);
- return -1;
- }
- }
-
- if (STRCASEEQ(value->str, "true")) {
- *boolean_ = 1;
- } else if (STRCASEEQ(value->str, "false")) {
- *boolean_ = 0;
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Config entry '%s' must represent a boolean value "
- "(true|false)"), name);
- return -1;
- }
+ if (STRCASEEQ(string, "true")) {
+ *boolean_ = 1;
+ } else if (STRCASEEQ(string, "false")) {
+ *boolean_ = 0;
} else {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Config entry '%s' must be a string"), name);
- return -1;
+ _("Config entry '%s' must represent a boolean value "
+ "(true|false)"), name);
+ result = -1;
+ goto cleanup;
}
- return 0;
+ result = 0;
+
+ cleanup:
+ VIR_FREE(string);
+
+ return result;
}
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listOn Sat, May 26, 2018 at 11:00:26PM +0200, Fabiano Fidêncio wrote:
>From: Fabiano Fidêncio <fidencio@redhat.com>
>
>Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org>
>---
> src/vmx/vmx.c | 196 ++++++++++++++++++++++------------------------------------
> 1 file changed, 73 insertions(+), 123 deletions(-)
>
>diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
>index df6a58a474..54542c29a6 100644
>--- a/src/vmx/vmx.c
>+++ b/src/vmx/vmx.c
>@@ -808,47 +786,35 @@ static int
> virVMXGetConfigLong(virConfPtr conf, const char *name, long long *number,
> long long default_, bool optional)
> {
>- virConfValuePtr value;
>+ char *string = NULL;
>+ int result;
Usually we use 'ret' as the name of the variable that will eventually be
used to return from the function and 'rc' to store the value returned by
the function we call (where they return more values than 0/-1):
int ret = -1;
int rc;
>
> *number = default_;
>- value = virConfGetValue(conf, name);
>
>- if (value == NULL) {
>- if (optional) {
>- return 0;
>- } else {
>- virReportError(VIR_ERR_INTERNAL_ERROR,
>- _("Missing essential config entry '%s'"), name);
>- return -1;
>- }
>- }
>+ result = virVMXGetConfigStringHelper(conf, name, &string, optional);
>+ if (result <= 0)
>+ return result;
This would become:
rc = GetStringHelper();
if (rc <= 0)
return rc;
>
>- if (value->type == VIR_CONF_STRING) {
>- if (value->str == NULL) {
>- if (optional) {
>- return 0;
>- } else {
>- virReportError(VIR_ERR_INTERNAL_ERROR,
>- _("Missing essential config entry '%s'"), name);
>- return -1;
>- }
>- }
>+ if (STRCASEEQ(string, "unlimited")) {
>+ *number = -1;
>+ result = 0;
>+ goto cleanup;
>+ }
>
>- if (STRCASEEQ(value->str, "unlimited")) {
>- *number = -1;
>- } else if (virStrToLong_ll(value->str, NULL, 10, number) < 0) {
>- virReportError(VIR_ERR_INTERNAL_ERROR,
>- _("Config entry '%s' must represent an integer value"),
>- name);
>- return -1;
>- }
>- } else {
if you leave this 'else' here, there's no need to touch 'ret' or goto
cleanup above.
>+ if (virStrToLong_ll(string, NULL, 10, number) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
>- _("Config entry '%s' must be a string"), name);
>- return -1;
>+ _("Config entry '%s' must represent an integer value"),
>+ name);
>+ result = -1;
This adjustment would also be unnecessary.
>+ goto cleanup;
> }
>
>- return 0;
>+ result = 0;
>+
(NB: while many of the functions in this file use 'result' instead of
'ret', I'd suggest to slowly start replacing the old name instead of
striving for consistency. Most of them are the Parse functions that
already return a device definition via a pointer argument and the
0/-1 they return is redundant, because it can be replaced by a NULL
check on the pointer)
Jano
Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Sun, May 27, 2018 at 1:02 PM, Ján Tomko <jtomko@redhat.com> wrote:
> On Sat, May 26, 2018 at 11:00:26PM +0200, Fabiano Fidêncio wrote:
>>
>> From: Fabiano Fidêncio <fidencio@redhat.com>
>>
>> Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org>
>> ---
>> src/vmx/vmx.c | 196
>> ++++++++++++++++++++++------------------------------------
>> 1 file changed, 73 insertions(+), 123 deletions(-)
>>
>> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
>> index df6a58a474..54542c29a6 100644
>> --- a/src/vmx/vmx.c
>> +++ b/src/vmx/vmx.c
>> @@ -808,47 +786,35 @@ static int
>> virVMXGetConfigLong(virConfPtr conf, const char *name, long long *number,
>> long long default_, bool optional)
>> {
>> - virConfValuePtr value;
>> + char *string = NULL;
>> + int result;
>
>
> Usually we use 'ret' as the name of the variable that will eventually be
> used to return from the function and 'rc' to store the value returned by
> the function we call (where they return more values than 0/-1):
>
> int ret = -1;
> int rc;
>
Right!
>>
>> *number = default_;
>> - value = virConfGetValue(conf, name);
>>
>> - if (value == NULL) {
>> - if (optional) {
>> - return 0;
>> - } else {
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("Missing essential config entry '%s'"),
>> name);
>> - return -1;
>> - }
>> - }
>> + result = virVMXGetConfigStringHelper(conf, name, &string, optional);
>> + if (result <= 0)
>> + return result;
>
>
> This would become:
> rc = GetStringHelper();
> if (rc <= 0)
> return rc;
>
Okay.
>>
>> - if (value->type == VIR_CONF_STRING) {
>> - if (value->str == NULL) {
>> - if (optional) {
>> - return 0;
>> - } else {
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("Missing essential config entry '%s'"),
>> name);
>> - return -1;
>> - }
>> - }
>> + if (STRCASEEQ(string, "unlimited")) {
>> + *number = -1;
>> + result = 0;
>> + goto cleanup;
>> + }
>>
>> - if (STRCASEEQ(value->str, "unlimited")) {
>> - *number = -1;
>> - } else if (virStrToLong_ll(value->str, NULL, 10, number) < 0) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("Config entry '%s' must represent an integer
>> value"),
>> - name);
>> - return -1;
>> - }
>> - } else {
>
>
> if you leave this 'else' here, there's no need to touch 'ret' or goto
> cleanup above.
Okay, I'll keep it as it is.
>
>> + if (virStrToLong_ll(string, NULL, 10, number) < 0) {
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("Config entry '%s' must be a string"), name);
>> - return -1;
>> + _("Config entry '%s' must represent an integer value"),
>> + name);
>
>
>> + result = -1;
>
>
> This adjustment would also be unnecessary.
>
>> + goto cleanup;
>> }
>>
>> - return 0;
>> + result = 0;
>> +
>
>
> (NB: while many of the functions in this file use 'result' instead of
> 'ret', I'd suggest to slowly start replacing the old name instead of
> striving for consistency. Most of them are the Parse functions that
> already return a device definition via a pointer argument and the
> 0/-1 they return is redundant, because it can be replaced by a NULL
> check on the pointer)
Right!
>
> Jano
>
> Jano
Thanks for the review!
--
Fabiano Fidêncio
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.