On Sun, May 27, 2018 at 1:17 PM, Ján Tomko <jtomko@xxxxxxxxxx> wrote:
> On Sat, May 26, 2018 at 11:00:27PM +0200, Fabiano Fidêncio wrote:
>>
>> From: Fabiano Fidêncio <fidencio@xxxxxxxxxx>
>>
>> There are still some places using virConfGetValue() and then checking
>> the specific type of the pointers and so on.
>>
>> Those place are not going to be changed as:
>> - Directly using virConfGetValue*() would trigger virReportError() on
>> their current code
>
>
> Is that a problem in xenParseCPUFeatures?
It would, at least, generate one more log, which would be misleading
whoever ends up debugging some issue on that codepath later on.
>
>> - Expanding virConfGetValue*() to support strings as other types (as
>> boolean or long) doesn't seem to be the safest path to take.
>>
>> Signed-off-by: Fabiano Fidêncio <fabiano@xxxxxxxxxxxx>
>> ---
>> src/xenconfig/xen_common.c | 637
>> ++++++++++++++++++++++-----------------------
>> 1 file changed, 307 insertions(+), 330 deletions(-)
>>
>> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
>> index a2b0708ee3..2e8e95f720 100644
>> --- a/src/xenconfig/xen_common.c
>> +++ b/src/xenconfig/xen_common.c
>> @@ -145,31 +145,18 @@ xenConfigCopyStringInternal(virConfPtr conf,
>> char **value,
>> int allowMissing)
>> {
>> - virConfValuePtr val;
>> + int result;
>
>
> int rc;
>
Noted!
>
>>
>> *value = NULL;
>> - if (!(val = virConfGetValue(conf, name))) {
>> - if (allowMissing)
>> - return 0;
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("config value %s was missing"), name);
>> - return -1;
>> - }
>>
>> - if (val->type != VIR_CONF_STRING) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("config value %s was not a string"), name);
>> - return -1;
>> - }
>> - if (!val->str) {
>> - if (allowMissing)
>> - return 0;
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("config value %s was missing"), name);
>> - return -1;
>> - }
>> + result = virConfGetValueString(conf, name, value);
>> + if (result == 1 && *value != NULL)
>> + return 0;
>> +
>> + if (allowMissing)
>> + return 0;
>>
>> - return VIR_STRDUP(*value, val->str);
>> + return -1;
>> }
>>
>>
>> @@ -193,7 +180,8 @@ xenConfigCopyStringOpt(virConfPtr conf, const char
>> *name, char **value)
>> static int
>> xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid)
>> {
>> - virConfValuePtr val;
>> + char *string = NULL;
>> + int result;
>
>
> int ret = -1;
>
Noted!
>
>>
>> if (!uuid || !name || !conf) {
>> virReportError(VIR_ERR_INVALID_ARG, "%s",
>> @@ -201,35 +189,35 @@ xenConfigGetUUID(virConfPtr conf, const char *name,
>> unsigned char *uuid)
>> return -1;
>> }
>>
>> - if (!(val = virConfGetValue(conf, name))) {
>> - if (virUUIDGenerate(uuid) < 0) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - "%s", _("Failed to generate UUID"));
>> - return -1;
>> - } else {
>> - return 0;
>> - }
>> - }
>> -
>> - if (val->type != VIR_CONF_STRING) {
>> - virReportError(VIR_ERR_CONF_SYNTAX,
>> - _("config value %s not a string"), name);
>> + if (virConfGetValueString(conf, name, &string) < 0)
>> return -1;
>> - }
>>
>> - if (!val->str) {
>> + if (!string) {
>> virReportError(VIR_ERR_CONF_SYNTAX,
>> _("%s can't be empty"), name);
>> return -1;
>> }
>>
>> - if (virUUIDParse(val->str, uuid) < 0) {
>> + if (virUUIDGenerate(uuid) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + "%s", _("Failed to generate UUID"));
>
>
>> + result = -1;
>
>
> Redundant if you initialize it to -1;
>
>> + goto cleanup;
>> + }
>> +
>> + if (virUUIDParse(string, uuid) < 0) {
>> virReportError(VIR_ERR_CONF_SYNTAX,
>> - _("%s not parseable"), val->str);
>> - return -1;
>> + _("%s not parseable"), string);
>
>
>> + result = -1;
>
>
> Dtto.
>
>> + goto cleanup;
>> }
>>
>> - return 0;
>> + result = 1;
>
>
> ret = 0;
>
>> +
>> + cleanup:
>> + VIR_FREE(string);
>
>
>> +
>
>
> Dropping this line could improve readability - it's already visually
> separated by the cleanup label and }
>
>> + return result;
>> }
>>
>>
>> @@ -392,93 +372,92 @@ xenParseEventsActions(virConfPtr conf,
>> virDomainDefPtr def)
>> static int
>> xenParsePCI(virConfPtr conf, virDomainDefPtr def)
>> {
>> - virConfValuePtr list = virConfGetValue(conf, "pci");
>> virDomainHostdevDefPtr hostdev = NULL;
>> + char **pcis = NULL, **entries;
>>
>
> Another function that would benefit from being split.
>
> [...]
>
>>
>> + virStringListFree(pcis);
>> return 0;
>> +
>> + cleanup:
>
>
> Use the ret variable to join these cleanup paths into one.
>
>> + virStringListFree(pcis);
>> + return -1;
>> }
>>
>>
>
> Jano
Thanks for the review!
--
Fabiano Fidêncio
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
