[libvirt] [PATCH v1 0/3] Forbid negative values in config files
*** BLURB HERE *** Michal Privoznik (3): virConfSetValue: Simplify condition virConfType: switch to VIR_ENUM_{DECL,IMPL} virconf: Introduce VIR_CONF_ULONG daemon/libvirtd-config.c | 55 ++++++++++++++++++++++++--------------- src/libvirt_private.syms | 2 ++ src/locking/lock_daemon_config.c | 24 ++++++++++++----- src/locking/lock_driver_lockd.c | 4 +-- src/locking/lock_driver_sanlock.c | 6 ++--- src/lxc/lxc_conf.c | 6 ++--- src/qemu/qemu_conf.c | 43 ++++++++++++++++++++---------- src/util/virconf.c | 12 +++++++-- src/util/virconf.h | 29 +++++++-------------- src/xenconfig/xen_common.c | 6 ++--- tests/libvirtdconftest.c | 9 ++++--- 11 files changed, 119 insertions(+), 77 deletions(-) -- 2.0.4
There's no need for condition of the following form: if (str && STREQ(str, dst)) since we have STREQ_NULLABLE macro that handles NULL cases. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virconf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virconf.c b/src/util/virconf.c index 0a17eff..74e695a 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -884,7 +884,7 @@ virConfSetValue(virConfPtr conf, cur = conf->entries; while (cur != NULL) { - if ((cur->name != NULL) && (STREQ(cur->name, setting))) + if (STREQ_NULLABLE(cur->name, setting)) break; prev = cur; cur = cur->next; -- 2.0.4
On Tue, Dec 09, 2014 at 04:52:14PM +0100, Michal Privoznik wrote:
There's no need for condition of the following form:
if (str && STREQ(str, dst))
since we have STREQ_NULLABLE macro that handles NULL cases.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virconf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
diff --git a/src/util/virconf.c b/src/util/virconf.c index 0a17eff..74e695a 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -884,7 +884,7 @@ virConfSetValue(virConfPtr conf,
cur = conf->entries; while (cur != NULL) { - if ((cur->name != NULL) && (STREQ(cur->name, setting))) + if (STREQ_NULLABLE(cur->name, setting)) break; prev = cur; cur = cur->next; -- 2.0.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
There's no need to implement ToString() function like we do if we can use our shiny macros. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/libvirtd-config.c | 4 ++-- src/libvirt_private.syms | 2 ++ src/locking/lock_daemon_config.c | 4 ++-- src/util/virconf.c | 6 ++++++ src/util/virconf.h | 28 +++++++++------------------- 5 files changed, 21 insertions(+), 23 deletions(-) diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c index c31ef16..929dd1a 100644 --- a/daemon/libvirtd-config.c +++ b/daemon/libvirtd-config.c @@ -123,8 +123,8 @@ checkType(virConfValuePtr p, const char *filename, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("remoteReadConfigFile: %s: %s: invalid type:" " got %s; expected %s"), filename, key, - virConfTypeName(p->type), - virConfTypeName(required_type)); + virConfTypeToString(p->type), + virConfTypeToString(required_type)); return -1; } return 0; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6df2784..57cf4d3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1211,6 +1211,8 @@ virConfNew; virConfReadFile; virConfReadMem; virConfSetValue; +virConfTypeFromString; +virConfTypeToString; virConfWalk; virConfWriteFile; virConfWriteMem; diff --git a/src/locking/lock_daemon_config.c b/src/locking/lock_daemon_config.c index 62b8b02..abe6aba 100644 --- a/src/locking/lock_daemon_config.c +++ b/src/locking/lock_daemon_config.c @@ -47,8 +47,8 @@ checkType(virConfValuePtr p, const char *filename, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("remoteReadConfigFile: %s: %s: invalid type:" " got %s; expected %s"), filename, key, - virConfTypeName(p->type), - virConfTypeName(required_type)); + virConfTypeToString(p->type), + virConfTypeToString(required_type)); return -1; } return 0; diff --git a/src/util/virconf.c b/src/util/virconf.c index 74e695a..5b4b4c3 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -80,6 +80,12 @@ struct _virConfParserCtxt { * * ************************************************************************/ +VIR_ENUM_IMPL(virConf, VIR_CONF_LAST, + "*unexpected*", + "long", + "string", + "list"); + typedef struct _virConfEntry virConfEntry; typedef virConfEntry *virConfEntryPtr; diff --git a/src/util/virconf.h b/src/util/virconf.h index 2a6b050..6176d43 100644 --- a/src/util/virconf.h +++ b/src/util/virconf.h @@ -23,6 +23,8 @@ #ifndef __VIR_CONF_H__ # define __VIR_CONF_H__ +# include "virutil.h" + /** * virConfType: * one of the possible type for a value from the configuration file @@ -30,12 +32,15 @@ * TODO: we probably need a float too. */ typedef enum { - VIR_CONF_NONE = 0, /* undefined */ - VIR_CONF_LONG = 1, /* a long int */ - VIR_CONF_STRING = 2, /* a string */ - VIR_CONF_LIST = 3 /* a list */ + VIR_CONF_NONE = 0, /* undefined */ + VIR_CONF_LONG, /* a long int */ + VIR_CONF_STRING, /* a string */ + VIR_CONF_LIST, /* a list */ + VIR_CONF_LAST, /* sentinel */ } virConfType; +VIR_ENUM_DECL(virConf) + typedef enum { VIR_CONF_FLAG_VMX_FORMAT = 1, /* allow ':', '.' and '-' in names for compatibility with VMware VMX configuration file, but restrict @@ -45,21 +50,6 @@ typedef enum { to string only and don't expect quotes for values */ } virConfFlags; -static inline const char * -virConfTypeName (virConfType t) -{ - switch (t) { - case VIR_CONF_LONG: - return "long"; - case VIR_CONF_STRING: - return "string"; - case VIR_CONF_LIST: - return "list"; - default: - return "*unexpected*"; - } -} - /** * virConfValue: * a value from the configuration file -- 2.0.4
On Tue, Dec 09, 2014 at 04:52:15PM +0100, Michal Privoznik wrote:
There's no need to implement ToString() function like we do if we can use our shiny macros.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/libvirtd-config.c | 4 ++-- src/libvirt_private.syms | 2 ++ src/locking/lock_daemon_config.c | 4 ++-- src/util/virconf.c | 6 ++++++ src/util/virconf.h | 28 +++++++++------------------- 5 files changed, 21 insertions(+), 23 deletions(-)
[...]
diff --git a/src/util/virconf.h b/src/util/virconf.h index 2a6b050..6176d43 100644 --- a/src/util/virconf.h +++ b/src/util/virconf.h @@ -23,6 +23,8 @@ #ifndef __VIR_CONF_H__ # define __VIR_CONF_H__
+# include "virutil.h" + /** * virConfType: * one of the possible type for a value from the configuration file @@ -30,12 +32,15 @@ * TODO: we probably need a float too. */ typedef enum { - VIR_CONF_NONE = 0, /* undefined */ - VIR_CONF_LONG = 1, /* a long int */ - VIR_CONF_STRING = 2, /* a string */ - VIR_CONF_LIST = 3 /* a list */ + VIR_CONF_NONE = 0, /* undefined */ + VIR_CONF_LONG, /* a long int */ + VIR_CONF_STRING, /* a string */ + VIR_CONF_LIST, /* a list */
Could you add an empty line here before the sentinel? ACK with that changed.
+ VIR_CONF_LAST, /* sentinel */ } virConfType;
https://bugzilla.redhat.com/show_bug.cgi?id=1160995 In our config files users are expected to pass several integer values for different configuration knobs. However, majority of them expect a nonnegative number and only a few of them accept a negative number too (notably keepalive_interval in libvirtd.conf). Therefore, a new type to config value is introduced: VIR_CONF_ULONG that is set whenever an integer is positive or zero. With this approach knobs accepting VIR_CONF_LONG should accept VIR_CONF_ULONG too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/libvirtd-config.c | 51 ++++++++++++++++++++++++--------------- src/locking/lock_daemon_config.c | 20 ++++++++++++--- src/locking/lock_driver_lockd.c | 4 +-- src/locking/lock_driver_sanlock.c | 6 ++--- src/lxc/lxc_conf.c | 6 ++--- src/qemu/qemu_conf.c | 43 ++++++++++++++++++++++----------- src/util/virconf.c | 4 ++- src/util/virconf.h | 1 + src/xenconfig/xen_common.c | 6 ++--- tests/libvirtdconftest.c | 9 ++++--- 10 files changed, 97 insertions(+), 53 deletions(-) diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c index 929dd1a..3694455 100644 --- a/daemon/libvirtd-config.c +++ b/daemon/libvirtd-config.c @@ -146,17 +146,30 @@ checkType(virConfValuePtr p, const char *filename, } \ } while (0) -/* Like GET_CONF_STR, but for integral values. */ +/* Like GET_CONF_STR, but for signed integral values. */ #define GET_CONF_INT(conf, filename, var_name) \ do { \ virConfValuePtr p = virConfGetValue(conf, #var_name); \ if (p) { \ - if (checkType(p, filename, #var_name, VIR_CONF_LONG) < 0) \ + if (p->type != VIR_CONF_ULONG && \ + checkType(p, filename, #var_name, VIR_CONF_LONG) < 0) \ goto error; \ data->var_name = p->l; \ } \ } while (0) +/* Like GET_CONF_STR, but for unsigned integral values. */ +#define GET_CONF_UINT(conf, filename, var_name) \ + do { \ + virConfValuePtr p = virConfGetValue(conf, #var_name); \ + if (p) { \ + if (checkType(p, filename, #var_name, VIR_CONF_ULONG) < 0) \ + goto error; \ + data->var_name = p->l; \ + } \ + } while (0) + + static int remoteConfigGetAuth(virConfPtr conf, @@ -361,8 +374,8 @@ daemonConfigLoadOptions(struct daemonConfig *data, const char *filename, virConfPtr conf) { - GET_CONF_INT(conf, filename, listen_tcp); - GET_CONF_INT(conf, filename, listen_tls); + GET_CONF_UINT(conf, filename, listen_tcp); + GET_CONF_UINT(conf, filename, listen_tls); GET_CONF_STR(conf, filename, tls_port); GET_CONF_STR(conf, filename, tcp_port); GET_CONF_STR(conf, filename, listen_addr); @@ -396,11 +409,11 @@ daemonConfigLoadOptions(struct daemonConfig *data, GET_CONF_STR(conf, filename, unix_sock_dir); - GET_CONF_INT(conf, filename, mdns_adv); + GET_CONF_UINT(conf, filename, mdns_adv); GET_CONF_STR(conf, filename, mdns_name); - GET_CONF_INT(conf, filename, tls_no_sanity_certificate); - GET_CONF_INT(conf, filename, tls_no_verify_certificate); + GET_CONF_UINT(conf, filename, tls_no_sanity_certificate); + GET_CONF_UINT(conf, filename, tls_no_verify_certificate); GET_CONF_STR(conf, filename, key_file); GET_CONF_STR(conf, filename, cert_file); @@ -417,29 +430,29 @@ daemonConfigLoadOptions(struct daemonConfig *data, goto error; - GET_CONF_INT(conf, filename, min_workers); - GET_CONF_INT(conf, filename, max_workers); - GET_CONF_INT(conf, filename, max_clients); - GET_CONF_INT(conf, filename, max_queued_clients); - GET_CONF_INT(conf, filename, max_anonymous_clients); + GET_CONF_UINT(conf, filename, min_workers); + GET_CONF_UINT(conf, filename, max_workers); + GET_CONF_UINT(conf, filename, max_clients); + GET_CONF_UINT(conf, filename, max_queued_clients); + GET_CONF_UINT(conf, filename, max_anonymous_clients); - GET_CONF_INT(conf, filename, prio_workers); + GET_CONF_UINT(conf, filename, prio_workers); GET_CONF_INT(conf, filename, max_requests); - GET_CONF_INT(conf, filename, max_client_requests); + GET_CONF_UINT(conf, filename, max_client_requests); - GET_CONF_INT(conf, filename, audit_level); - GET_CONF_INT(conf, filename, audit_logging); + GET_CONF_UINT(conf, filename, audit_level); + GET_CONF_UINT(conf, filename, audit_logging); GET_CONF_STR(conf, filename, host_uuid); - GET_CONF_INT(conf, filename, log_level); + GET_CONF_UINT(conf, filename, log_level); GET_CONF_STR(conf, filename, log_filters); GET_CONF_STR(conf, filename, log_outputs); GET_CONF_INT(conf, filename, keepalive_interval); - GET_CONF_INT(conf, filename, keepalive_count); - GET_CONF_INT(conf, filename, keepalive_required); + GET_CONF_UINT(conf, filename, keepalive_count); + GET_CONF_UINT(conf, filename, keepalive_required); return 0; diff --git a/src/locking/lock_daemon_config.c b/src/locking/lock_daemon_config.c index abe6aba..8a6d18f 100644 --- a/src/locking/lock_daemon_config.c +++ b/src/locking/lock_daemon_config.c @@ -70,12 +70,24 @@ checkType(virConfValuePtr p, const char *filename, } \ } while (0) -/* Like GET_CONF_STR, but for integral values. */ +/* Like GET_CONF_STR, but for signed integer values. */ #define GET_CONF_INT(conf, filename, var_name) \ do { \ virConfValuePtr p = virConfGetValue(conf, #var_name); \ if (p) { \ - if (checkType(p, filename, #var_name, VIR_CONF_LONG) < 0) \ + if (p->type != VIR_CONF_ULONG && \ + checkType(p, filename, #var_name, VIR_CONF_LONG) < 0) \ + goto error; \ + data->var_name = p->l; \ + } \ + } while (0) + +/* Like GET_CONF_STR, but for unsigned integer values. */ +#define GET_CONF_UINT(conf, filename, var_name) \ + do { \ + virConfValuePtr p = virConfGetValue(conf, #var_name); \ + if (p) { \ + if (checkType(p, filename, #var_name, VIR_CONF_ULONG) < 0) \ goto error; \ data->var_name = p->l; \ } \ @@ -137,10 +149,10 @@ virLockDaemonConfigLoadOptions(virLockDaemonConfigPtr data, const char *filename, virConfPtr conf) { - GET_CONF_INT(conf, filename, log_level); + GET_CONF_UINT(conf, filename, log_level); GET_CONF_STR(conf, filename, log_filters); GET_CONF_STR(conf, filename, log_outputs); - GET_CONF_INT(conf, filename, max_clients); + GET_CONF_UINT(conf, filename, max_clients); return 0; diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index a642122..2af3f22 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -108,7 +108,7 @@ static int virLockManagerLockDaemonLoadConfig(const char *configFile) } p = virConfGetValue(conf, "auto_disk_leases"); - CHECK_TYPE("auto_disk_leases", VIR_CONF_LONG); + CHECK_TYPE("auto_disk_leases", VIR_CONF_ULONG); if (p) driver->autoDiskLease = p->l; p = virConfGetValue(conf, "file_lockspace_dir"); @@ -142,7 +142,7 @@ static int virLockManagerLockDaemonLoadConfig(const char *configFile) } p = virConfGetValue(conf, "require_lease_for_disks"); - CHECK_TYPE("require_lease_for_disks", VIR_CONF_LONG); + CHECK_TYPE("require_lease_for_disks", VIR_CONF_ULONG); if (p) driver->requireLeaseForDisks = p->l; else diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index d84a419..b24e910 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -127,7 +127,7 @@ static int virLockManagerSanlockLoadConfig(const char *configFile) } p = virConfGetValue(conf, "auto_disk_leases"); - CHECK_TYPE("auto_disk_leases", VIR_CONF_LONG); + CHECK_TYPE("auto_disk_leases", VIR_CONF_ULONG); if (p) driver->autoDiskLease = p->l; p = virConfGetValue(conf, "disk_lease_dir"); @@ -141,11 +141,11 @@ static int virLockManagerSanlockLoadConfig(const char *configFile) } p = virConfGetValue(conf, "host_id"); - CHECK_TYPE("host_id", VIR_CONF_LONG); + CHECK_TYPE("host_id", VIR_CONF_UONG); if (p) driver->hostID = p->l; p = virConfGetValue(conf, "require_lease_for_disks"); - CHECK_TYPE("require_lease_for_disks", VIR_CONF_LONG); + CHECK_TYPE("require_lease_for_disks", VIR_CONF_ULONG); if (p) driver->requireLeaseForDisks = p->l; else diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c index e713ff8..b6d1797 100644 --- a/src/lxc/lxc_conf.c +++ b/src/lxc/lxc_conf.c @@ -270,7 +270,7 @@ virLXCLoadDriverConfig(virLXCDriverConfigPtr cfg, } p = virConfGetValue(conf, "log_with_libvirtd"); - CHECK_TYPE("log_with_libvirtd", VIR_CONF_LONG); + CHECK_TYPE("log_with_libvirtd", VIR_CONF_ULONG); if (p) cfg->log_libvirtd = p->l; p = virConfGetValue(conf, "security_driver"); @@ -283,11 +283,11 @@ virLXCLoadDriverConfig(virLXCDriverConfigPtr cfg, } p = virConfGetValue(conf, "security_default_confined"); - CHECK_TYPE("security_default_confined", VIR_CONF_LONG); + CHECK_TYPE("security_default_confined", VIR_CONF_ULONG); if (p) cfg->securityDefaultConfined = p->l; p = virConfGetValue(conf, "security_require_confined"); - CHECK_TYPE("security_require_confined", VIR_CONF_LONG); + CHECK_TYPE("security_require_confined", VIR_CONF_ULONG); if (p) cfg->securityRequireConfined = p->l; diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 4764bef..8d3818e 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -412,15 +412,29 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; \ } -#define GET_VALUE_LONG(NAME, VAR) \ +#define CHECK_TYPE_ALT(name, type1, type2) \ + if (p && (p->type != (type1) && p->type != (type2))) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "%s: %s: expected type " #type1, \ + filename, (name)); \ + goto cleanup; \ + } + +#define GET_VALUE_LONG(NAME, VAR) \ + p = virConfGetValue(conf, NAME); \ + CHECK_TYPE_ALT(NAME, VIR_CONF_LONG, VIR_CONF_ULONG); \ + if (p) \ + VAR = p->l; + +#define GET_VALUE_ULONG(NAME, VAR) \ p = virConfGetValue(conf, NAME); \ - CHECK_TYPE(NAME, VIR_CONF_LONG); \ + CHECK_TYPE(NAME, VIR_CONF_ULONG); \ if (p) \ VAR = p->l; #define GET_VALUE_BOOL(NAME, VAR) \ p = virConfGetValue(conf, NAME); \ - CHECK_TYPE(NAME, VIR_CONF_LONG); \ + CHECK_TYPE(NAME, VIR_CONF_ULONG); \ if (p) \ VAR = p->l != 0; @@ -489,7 +503,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, GET_VALUE_STR("spice_password", cfg->spicePassword); - GET_VALUE_LONG("remote_websocket_port_min", cfg->webSocketPortMin); + GET_VALUE_ULONG("remote_websocket_port_min", cfg->webSocketPortMin); if (cfg->webSocketPortMin < QEMU_WEBSOCKET_PORT_MIN) { /* if the port is too low, we can't get the display name * to tell to vnc (usually subtract 5700, e.g. localhost:1 @@ -501,7 +515,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; } - GET_VALUE_LONG("remote_websocket_port_max", cfg->webSocketPortMax); + GET_VALUE_ULONG("remote_websocket_port_max", cfg->webSocketPortMax); if (cfg->webSocketPortMax > QEMU_WEBSOCKET_PORT_MAX || cfg->webSocketPortMax < cfg->webSocketPortMin) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -518,7 +532,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; } - GET_VALUE_LONG("remote_display_port_min", cfg->remotePortMin); + GET_VALUE_ULONG("remote_display_port_min", cfg->remotePortMin); if (cfg->remotePortMin < QEMU_REMOTE_PORT_MIN) { /* if the port is too low, we can't get the display name * to tell to vnc (usually subtract 5900, e.g. localhost:1 @@ -530,7 +544,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; } - GET_VALUE_LONG("remote_display_port_max", cfg->remotePortMax); + GET_VALUE_ULONG("remote_display_port_max", cfg->remotePortMax); if (cfg->remotePortMax > QEMU_REMOTE_PORT_MAX || cfg->remotePortMax < cfg->remotePortMin) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -547,7 +561,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; } - GET_VALUE_LONG("migration_port_min", cfg->migrationPortMin); + GET_VALUE_ULONG("migration_port_min", cfg->migrationPortMin); if (cfg->migrationPortMin <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("%s: migration_port_min: port must be greater than 0"), @@ -555,7 +569,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; } - GET_VALUE_LONG("migration_port_max", cfg->migrationPortMax); + GET_VALUE_ULONG("migration_port_max", cfg->migrationPortMax); if (cfg->migrationPortMax > 65535 || cfg->migrationPortMax < cfg->migrationPortMin) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -694,15 +708,15 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, GET_VALUE_BOOL("clear_emulator_capabilities", cfg->clearEmulatorCapabilities); GET_VALUE_BOOL("allow_disk_format_probing", cfg->allowDiskFormatProbing); GET_VALUE_BOOL("set_process_name", cfg->setProcessName); - GET_VALUE_LONG("max_processes", cfg->maxProcesses); - GET_VALUE_LONG("max_files", cfg->maxFiles); + GET_VALUE_ULONG("max_processes", cfg->maxProcesses); + GET_VALUE_ULONG("max_files", cfg->maxFiles); GET_VALUE_STR("lock_manager", cfg->lockManagerName); - GET_VALUE_LONG("max_queued", cfg->maxQueuedJobs); + GET_VALUE_ULONG("max_queued", cfg->maxQueuedJobs); GET_VALUE_LONG("keepalive_interval", cfg->keepAliveInterval); - GET_VALUE_LONG("keepalive_count", cfg->keepAliveCount); + GET_VALUE_ULONG("keepalive_count", cfg->keepAliveCount); GET_VALUE_LONG("seccomp_sandbox", cfg->seccompSandbox); @@ -775,8 +789,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, virConfFree(conf); return ret; } -#undef GET_VALUE_BOOL #undef GET_VALUE_LONG +#undef GET_VALUE_ULONG +#undef GET_VALUE_BOOL #undef GET_VALUE_STR virQEMUDriverConfigPtr virQEMUDriverGetConfig(virQEMUDriverPtr driver) diff --git a/src/util/virconf.c b/src/util/virconf.c index 5b4b4c3..b1509fe 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -83,6 +83,7 @@ struct _virConfParserCtxt { VIR_ENUM_IMPL(virConf, VIR_CONF_LAST, "*unexpected*", "long", + "unsigned long", "string", "list"); @@ -273,6 +274,7 @@ virConfSaveValue(virBufferPtr buf, virConfValuePtr val) case VIR_CONF_NONE: return -1; case VIR_CONF_LONG: + case VIR_CONF_ULONG: virBufferAsprintf(buf, "%ld", val->l); break; case VIR_CONF_STRING: @@ -534,9 +536,9 @@ virConfParseValue(virConfParserCtxtPtr ctxt) _("numbers not allowed in VMX format")); return NULL; } + type = (c_isdigit(CUR) || CUR == '+') ? VIR_CONF_ULONG : VIR_CONF_LONG; if (virConfParseLong(ctxt, &l) < 0) return NULL; - type = VIR_CONF_LONG; } else { virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("expecting a value")); return NULL; diff --git a/src/util/virconf.h b/src/util/virconf.h index 6176d43..2b4b943 100644 --- a/src/util/virconf.h +++ b/src/util/virconf.h @@ -34,6 +34,7 @@ typedef enum { VIR_CONF_NONE = 0, /* undefined */ VIR_CONF_LONG, /* a long int */ + VIR_CONF_ULONG, /* an unsigned long int */ VIR_CONF_STRING, /* a string */ VIR_CONF_LIST, /* a list */ VIR_CONF_LAST, /* sentinel */ diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 7f4ec89..33ac127 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -57,7 +57,7 @@ xenConfigGetBool(virConfPtr conf, return 0; } - if (val->type == VIR_CONF_LONG) { + if (val->type == VIR_CONF_ULONG) { *value = val->l ? 1 : 0; } else if (val->type == VIR_CONF_STRING) { *value = STREQ(val->str, "1") ? 1 : 0; @@ -87,7 +87,7 @@ xenConfigGetULong(virConfPtr conf, return 0; } - if (val->type == VIR_CONF_LONG) { + if (val->type == VIR_CONF_ULONG) { *value = val->l; } else if (val->type == VIR_CONF_STRING) { if (virStrToLong_ul(val->str, NULL, 10, value) < 0) { @@ -121,7 +121,7 @@ xenConfigGetULongLong(virConfPtr conf, return 0; } - if (val->type == VIR_CONF_LONG) { + if (val->type == VIR_CONF_ULONG) { *value = val->l; } else if (val->type == VIR_CONF_STRING) { if (virStrToLong_ull(val->str, NULL, 10, value) < 0) { diff --git a/tests/libvirtdconftest.c b/tests/libvirtdconftest.c index 8b93f4e..d589d51 100644 --- a/tests/libvirtdconftest.c +++ b/tests/libvirtdconftest.c @@ -65,7 +65,7 @@ munge_param(const char *datain, if (c_isspace(*tmp)) continue; if (c_isdigit(*tmp)) { - *type = VIR_CONF_LONG; + *type = VIR_CONF_ULONG; replace = "\"foo\""; } else if (*tmp == '[') { *type = VIR_CONF_LIST; @@ -130,15 +130,16 @@ testCorrupt(const void *opaque) #endif switch (type) { - case VIR_CONF_LONG: - if (!strstr(err->message, "invalid type: got string; expected long")) { + case VIR_CONF_ULONG: + if (!strstr(err->message, "invalid type: got string; expected unsigned long") && + !strstr(err->message, "invalid type: got string; expected long")) { VIR_DEBUG("Wrong error for long: '%s'", err->message); ret = -1; } break; case VIR_CONF_STRING: - if (!strstr(err->message, "invalid type: got long; expected string")) { + if (!strstr(err->message, "invalid type: got unsigned long; expected string")) { VIR_DEBUG("Wrong error for string: '%s'", err->message); ret = -1; -- 2.0.4
On Tue, Dec 09, 2014 at 04:52:16PM +0100, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1160995
In our config files users are expected to pass several integer values for different configuration knobs. However, majority of them expect a nonnegative number and only a few of them accept a negative number too (notably keepalive_interval in libvirtd.conf). Therefore, a new type to config value is introduced: VIR_CONF_ULONG that is set whenever an integer is positive or zero. With this approach knobs accepting VIR_CONF_LONG should accept VIR_CONF_ULONG too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/libvirtd-config.c | 51 ++++++++++++++++++++++++--------------- src/locking/lock_daemon_config.c | 20 ++++++++++++--- src/locking/lock_driver_lockd.c | 4 +-- src/locking/lock_driver_sanlock.c | 6 ++--- src/lxc/lxc_conf.c | 6 ++--- src/qemu/qemu_conf.c | 43 ++++++++++++++++++++++----------- src/util/virconf.c | 4 ++- src/util/virconf.h | 1 + src/xenconfig/xen_common.c | 6 ++--- tests/libvirtdconftest.c | 9 ++++--- 10 files changed, 97 insertions(+), 53 deletions(-)
[...]
diff --git a/tests/libvirtdconftest.c b/tests/libvirtdconftest.c index 8b93f4e..d589d51 100644 --- a/tests/libvirtdconftest.c +++ b/tests/libvirtdconftest.c @@ -65,7 +65,7 @@ munge_param(const char *datain, if (c_isspace(*tmp)) continue; if (c_isdigit(*tmp)) { - *type = VIR_CONF_LONG; + *type = VIR_CONF_ULONG; replace = "\"foo\""; } else if (*tmp == '[') { *type = VIR_CONF_LIST; @@ -130,15 +130,16 @@ testCorrupt(const void *opaque) #endif
switch (type) { - case VIR_CONF_LONG: - if (!strstr(err->message, "invalid type: got string; expected long")) { + case VIR_CONF_ULONG: + if (!strstr(err->message, "invalid type: got string; expected unsigned long") && + !strstr(err->message, "invalid type: got string; expected long")) { VIR_DEBUG("Wrong error for long: '%s'", err->message); ret = -1; } break; case VIR_CONF_STRING: - if (!strstr(err->message, "invalid type: got long; expected string")) { + if (!strstr(err->message, "invalid type: got unsigned long; expected string")) { VIR_DEBUG("Wrong error for string: '%s'", err->message); ret = -1; -- 2.0.4
Special-casing the ULONG and adding a '-2' somewhere in the tests would show you've done the right thing. Anyway, ACK, but I'd wait after release since this does not fix anything, strictly speaking. Martin
participants (2)
-
Martin Kletzander -
Michal Privoznik