[libvirt] [PATCH] conf: rework code around 'append' attribute to make coverity happy
Signed-off-by: Dmitry Mishin <dim@virtuozzo.com> --- src/conf/domain_conf.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f210c0b..8895e10 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1723,10 +1723,11 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, switch (src->type) { case VIR_DOMAIN_CHR_TYPE_FILE: - dest->data.file.append = src->data.file.append; case VIR_DOMAIN_CHR_TYPE_PTY: case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_PIPE: + if (src->type == VIR_DOMAIN_CHR_TYPE_FILE) + dest->data.file.append = src->data.file.append; if (VIR_STRDUP(dest->data.file.path, src->data.file.path) < 0) return -1; break; @@ -9386,12 +9387,12 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, switch ((virDomainChrType) def->type) { case VIR_DOMAIN_CHR_TYPE_FILE: - if (!append) - append = virXMLPropString(cur, "append"); case VIR_DOMAIN_CHR_TYPE_PTY: case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_PIPE: case VIR_DOMAIN_CHR_TYPE_UNIX: + if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE) + append = virXMLPropString(cur, "append"); /* PTY path is only parsed from live xml. */ if (!path && (def->type != VIR_DOMAIN_CHR_TYPE_PTY || @@ -9476,15 +9477,15 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, break; case VIR_DOMAIN_CHR_TYPE_FILE: + case VIR_DOMAIN_CHR_TYPE_PTY: + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_PIPE: if (append && (def->data.file.append = virTristateSwitchTypeFromString(append)) <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid append attribute value '%s'"), append); goto error; } - case VIR_DOMAIN_CHR_TYPE_PTY: - case VIR_DOMAIN_CHR_TYPE_DEV: - case VIR_DOMAIN_CHR_TYPE_PIPE: if (!path && def->type != VIR_DOMAIN_CHR_TYPE_PTY) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", -- 1.8.3.1
On 12/26/2015 12:18 PM, Dmitry Mishin wrote:
Signed-off-by: Dmitry Mishin <dim@virtuozzo.com> --- src/conf/domain_conf.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f210c0b..8895e10 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1723,10 +1723,11 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
switch (src->type) { case VIR_DOMAIN_CHR_TYPE_FILE: - dest->data.file.append = src->data.file.append; case VIR_DOMAIN_CHR_TYPE_PTY: case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_PIPE: + if (src->type == VIR_DOMAIN_CHR_TYPE_FILE) + dest->data.file.append = src->data.file.append; if (VIR_STRDUP(dest->data.file.path, src->data.file.path) < 0) return -1; break; @@ -9386,12 +9387,12 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
switch ((virDomainChrType) def->type) { case VIR_DOMAIN_CHR_TYPE_FILE: - if (!append) - append = virXMLPropString(cur, "append"); case VIR_DOMAIN_CHR_TYPE_PTY: case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_PIPE: case VIR_DOMAIN_CHR_TYPE_UNIX: + if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE) + append = virXMLPropString(cur, "append"); /* PTY path is only parsed from live xml. */ if (!path && (def->type != VIR_DOMAIN_CHR_TYPE_PTY || @@ -9476,15 +9477,15 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, break;
case VIR_DOMAIN_CHR_TYPE_FILE: + case VIR_DOMAIN_CHR_TYPE_PTY: + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_PIPE: if (append &&
Wouldn't this last one be : if (append && def->type == VIR_DOMAIN_CHR_TYPE_FILE &&
(def->data.file.append = virTristateSwitchTypeFromString(append)) <= 0) {
NB: Other uses of def->data.file.append could have used the virTristateSwitch constants (VIR_TRISTATE_SWITCH_ABSENT, VIR_TRISTATE_SWITCH_ON, and VIR_TRISTATE_SWITCH_OFF). That is rather than "if (dev->data.file.append)", it could be "if (dev->data.file.append != VIR_TRISTATE_SWITCH_ABSENT). It's just "safer" that way. Does adding ",append=off" make much sense? Or do you expect some day in the future that append=on becomes the "default"? You could have added an example where append='off' - it shouldn't matter, but it is a legal option and the right XML should be generated and the right .args would have append=off added to the command line. John
virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid append attribute value '%s'"), append); goto error; } - case VIR_DOMAIN_CHR_TYPE_PTY: - case VIR_DOMAIN_CHR_TYPE_DEV: - case VIR_DOMAIN_CHR_TYPE_PIPE: if (!path && def->type != VIR_DOMAIN_CHR_TYPE_PTY) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
On 29/12/15 01:55, "John Ferlan" <jferlan@redhat.com> wrote:
On 12/26/2015 12:18 PM, Dmitry Mishin wrote:
Signed-off-by: Dmitry Mishin <dim@virtuozzo.com> --- src/conf/domain_conf.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f210c0b..8895e10 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1723,10 +1723,11 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
switch (src->type) { case VIR_DOMAIN_CHR_TYPE_FILE: - dest->data.file.append = src->data.file.append; case VIR_DOMAIN_CHR_TYPE_PTY: case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_PIPE: + if (src->type == VIR_DOMAIN_CHR_TYPE_FILE) + dest->data.file.append = src->data.file.append; if (VIR_STRDUP(dest->data.file.path, src->data.file.path) < 0) return -1; break; @@ -9386,12 +9387,12 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
switch ((virDomainChrType) def->type) { case VIR_DOMAIN_CHR_TYPE_FILE: - if (!append) - append = virXMLPropString(cur, "append"); case VIR_DOMAIN_CHR_TYPE_PTY: case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_PIPE: case VIR_DOMAIN_CHR_TYPE_UNIX: + if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE) + append = virXMLPropString(cur, "append"); /* PTY path is only parsed from live xml. */ if (!path && (def->type != VIR_DOMAIN_CHR_TYPE_PTY || @@ -9476,15 +9477,15 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, break;
case VIR_DOMAIN_CHR_TYPE_FILE: + case VIR_DOMAIN_CHR_TYPE_PTY: + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_PIPE: if (append &&
Wouldn't this last one be :
if (append && def->type == VIR_DOMAIN_CHR_TYPE_FILE &&
(def->data.file.append = virTristateSwitchTypeFromString(append)) <= 0) {
It is guaranteed by the check above, but ok, I will enhance this one as well.
NB:
Other uses of def->data.file.append could have used the virTristateSwitch constants (VIR_TRISTATE_SWITCH_ABSENT, VIR_TRISTATE_SWITCH_ON, and VIR_TRISTATE_SWITCH_OFF).
That is rather than "if (dev->data.file.append)", it could be "if (dev->data.file.append != VIR_TRISTATE_SWITCH_ABSENT). It's just "safer" that way.
Good catch, thank you - fixed in v2.
Does adding ",append=off" make much sense? Yes, because it is explicit specification of desired behaviour. It is always better than relying on defaults.
Or do you expect some day in the future that append=on becomes the "default"? I believe 'on' is better than 'off'. But also believe that unlikely the default value could be changed easily due to backward compatibility considerations.
You could have added an example where append='off' - it shouldn't matter, but it is a legal option and the right XML should be generated and the right .args would have append=off added to the command line.
Yes, it is a legal option - where should I add such an example? I've send a patch for formatdomain.html.in with the attribute's description, but it is no accepted yet. Thank you, Dmitry.
John
virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid append attribute value '%s'"), append); goto error; } - case VIR_DOMAIN_CHR_TYPE_PTY: - case VIR_DOMAIN_CHR_TYPE_DEV: - case VIR_DOMAIN_CHR_TYPE_PIPE: if (!path && def->type != VIR_DOMAIN_CHR_TYPE_PTY) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
participants (2)
-
Dmitry Mishin -
John Ferlan