On Sun, May 27, 2018 at 1:02 PM, Ján Tomko <jtomko@xxxxxxxxxx> wrote:
> On Sat, May 26, 2018 at 11:00:26PM +0200, Fabiano Fidêncio wrote:
>>
>> From: Fabiano Fidêncio <fidencio@xxxxxxxxxx>
>>
>> Signed-off-by: Fabiano Fidêncio <fabiano@xxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
