On Thu, Sep 20, 2018 at 10:17 PM, John Ferlan <jferlan@xxxxxxxxxx> wrote:
I think this all can be further simplified to:
On 09/20/2018 09:28 AM, Fabiano Fidêncio wrote:
> This change actually changes the behaviour of xenConfigGetString() as
> now it returns a newly-allocated string.
>
> Unfortunately, there's not much that can be done in order to avoid that
> and all the callers have to be changed in order to avoid leaking the
> return value.
>
> Also, as a side-effect of the change above, the function now takes a
> "char **" argument instead of a "const char **" one.
>
> Signed-off-by: Fabiano Fidêncio <fidencio@xxxxxxxxxx>
> ---
> src/xenconfig/xen_common.c | 84 ++++++++++++++++++++------------------
> src/xenconfig/xen_common.h | 2 +-
> src/xenconfig/xen_xl.c | 10 +++--
> src/xenconfig/xen_xm.c | 7 ++--
> 4 files changed, 56 insertions(+), 47 deletions(-)
>
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index 587bab2b19..7b3e5c3b44 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -228,26 +228,28 @@ xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid)
> int
> xenConfigGetString(virConfPtr conf,
> const char *name,
> - const char **value,
> + char **value,
> const char *def)
> {
> - virConfValuePtr val;
> + char *string = NULL;
> + int rc;
>
> *value = NULL;
> - if (!(val = virConfGetValue(conf, name))) {
> - *value = def;
> + if ((rc = virConfGetValueString(conf, name, &string)) < 0)
> + return -1;
> +
> + if (rc == 0) {
> + if (VIR_STRDUP(*value, def) < 0)
> + return -1;
> return 0;
> }
>
> - if (val->type != VIR_CONF_STRING) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("config value %s was malformed"), name);
> - return -1;
> + if (!string) {
> + if (VIR_STRDUP(*value, def) < 0)
> + return -1;
> + } else {
> + *value = string;
> }
> - if (!val->str)
> - *value = def;
> - else
> - *value = val->str;
if (rc == 0 || !string) {
if (VIR_STRDUP(*value, def) < 0)
return -1;
} else {
*value = string;
}
return 0;
The only reason I didn't go for it was to have the diff cleaner/smaller.
If you're fine merging the "ifs", please, go for it.
Don't believe the _NULLABLE variant is/was required here considering
> return 0;
> }
>
> @@ -345,32 +347,34 @@ xenParseTimeOffset(virConfPtr conf, virDomainDefPtr def)
> static int
> xenParseEventsActions(virConfPtr conf, virDomainDefPtr def)
> {
> - const char *str = NULL;
> + VIR_AUTOFREE(char *) _on_poweroff_ = NULL;
> + VIR_AUTOFREE(char *) _on_reboot_ = NULL;
> + VIR_AUTOFREE(char *) _on_crash_ = NULL;
>
> - if (xenConfigGetString(conf, "on_poweroff", &str, "destroy") < 0)
> + if (xenConfigGetString(conf, "on_poweroff", &on_poweroff, "destroy") < 0)
> return -1;
>
> - if ((def->_onPoweroff_ = virDomainLifecycleActionTypeFromString(str)) < 0) {
> + if ((def->_onPoweroff_ = virDomainLifecycleActionTypeFromString(on_poweroff)) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("unexpected value %s for on_poweroff"), str);
> + _("unexpected value %s for on_poweroff"), on_poweroff);
> return -1;
> }
>
> - if (xenConfigGetString(conf, "on_reboot", &str, "restart") < 0)
> + if (xenConfigGetString(conf, "on_reboot", &on_reboot, "restart") < 0)
> return -1;
>
> - if ((def->_onReboot_ = virDomainLifecycleActionTypeFromString(str)) < 0) {
> + if ((def->_onReboot_ = virDomainLifecycleActionTypeFromString(on_reboot)) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("unexpected value %s for on_reboot"), str);
> + _("unexpected value %s for on_reboot"), on_reboot);
> return -1;
> }
>
> - if (xenConfigGetString(conf, "on_crash", &str, "restart") < 0)
> + if (xenConfigGetString(conf, "on_crash", &on_crash, "restart") < 0)
> return -1;
>
> - if ((def->_onCrash_ = virDomainLifecycleActionTypeFromString(str)) < 0) {
> + if ((def->_onCrash_ = virDomainLifecycleActionTypeFromString(on_crash)) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("unexpected value %s for on_crash"), str);
> + _("unexpected value %s for on_crash"), on_crash);
> return -1;
> }
>
> @@ -488,7 +492,8 @@ xenParseCPUFeatures(virConfPtr conf,
> virDomainXMLOptionPtr xmlopt)
> {
> unsigned long count = 0;
> - const char *str = NULL;
> + VIR_AUTOFREE(char *) cpus = NULL;
> + VIR_AUTOFREE(char *) tsc_mode = NULL;
> int val = 0;
> virDomainTimerDefPtr timer;
>
> @@ -509,16 +514,16 @@ xenParseCPUFeatures(virConfPtr conf,
> return -1;
> }
>
> - if (xenConfigGetString(conf, "cpus", &str, NULL) < 0)
> + if (xenConfigGetString(conf, "cpus", &cpus, NULL) < 0)
> return -1;
>
> - if (str && (virBitmapParse(str, &def->cpumask, 4096) < 0))
> + if (cpus && (virBitmapParse(cpus, &def->cpumask, 4096) < 0))
> return -1;
>
> - if (xenConfigGetString(conf, "tsc_mode", &str, NULL) < 0)
> + if (xenConfigGetString(conf, "tsc_mode", &tsc_mode, NULL) < 0)
> return -1;
>
> - if (str) {
> + if (tsc_mode) {
> if (VIR_EXPAND_N(def->clock.timers, def->clock.ntimers, 1) < 0 ||
> VIR_ALLOC(timer) < 0)
> return -1;
> @@ -528,11 +533,11 @@ xenParseCPUFeatures(virConfPtr conf,
> timer->tickpolicy = -1;
> timer->mode = VIR_DOMAIN_TIMER_MODE_AUTO;
> timer->track = -1;
> - if (STREQ_NULLABLE(str, "always_emulate"))
> + if (STREQ_NULLABLE(tsc_mode, "always_emulate"))
> timer->mode = VIR_DOMAIN_TIMER_MODE_EMULATE;
> - else if (STREQ_NULLABLE(str, "native"))
> + else if (STREQ_NULLABLE(tsc_mode, "native"))
> timer->mode = VIR_DOMAIN_TIMER_MODE_NATIVE;
> - else if (STREQ_NULLABLE(str, "native_paravirt"))
> + else if (STREQ_NULLABLE(tsc_mode, "native_paravirt"))
@str couldn't have been NULL and certainly the right side isn't either!
Jim must've been super-paranoid for commit b4386fdac or perhaps worried
about the default value of NULL.
> timer->mode = VIR_DOMAIN_TIMER_MODE_PARAVIRT; Then there's this one where STRNEQ_NULLABLE would be the same check,
>
> def->clock.timers[def->clock.ntimers - 1] = timer;
> @@ -746,15 +751,15 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def)
> static int
> xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat)
> {
> - const char *str;
> virConfValuePtr value = NULL;
> virDomainChrDefPtr chr = NULL;
>
> if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
> - if (xenConfigGetString(conf, "parallel", &str, NULL) < 0)
> + VIR_AUTOFREE(char *) parallel = NULL;
> + if (xenConfigGetString(conf, "parallel", ¶llel, NULL) < 0)
> goto cleanup;
> - if (str && STRNEQ(str, "none") &&
> - !(chr = xenParseSxprChar(str, NULL)))
> + if (parallel && STRNEQ(parallel, "none") &&
> + !(chr = xenParseSxprChar(parallel, NULL)))
just like the next one for @serial...
Again, I won't change - unless you think we should just change these...
> goto cleanup;
> if (chr) {
> if (VIR_ALLOC_N(def->parallels, 1) < 0)
> @@ -801,11 +806,12 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat)
> value = value->next;
> }
> } else {
> + VIR_AUTOFREE(char *) serial = NULL;
> /* If domain is not using multiple serial ports we parse data old way */
> - if (xenConfigGetString(conf, "serial", &str, NULL) < 0)
> + if (xenConfigGetString(conf, "serial", &serial, NULL) < 0)
> goto cleanup;
> - if (str && STRNEQ(str, "none") &&
> - !(chr = xenParseSxprChar(str, NULL)))
> + if (serial && STRNEQ(serial, "none") &&
> + !(chr = xenParseSxprChar(serial, NULL)))
> goto cleanup;
> if (chr) {
> if (VIR_ALLOC_N(def->serials, 1) < 0)
[...]
I can make the changes locally before pushing - no need for another
series... I'll do the simplification of the logic, but hold off on the
_NULLABLE changes unless you think those are worth changing too.
I totally agree and I don't think the _NULLABLE changes should be part of this commit/series.
Please, do the changes! :-)
Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
John
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
