11:18 a.m.
Signed-off-by: Dmitry Mishin <dim(a)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
4:55 p.m.
On 12/26/2015 12:18 PM, Dmitry Mishin wrote:
Signed-off-by: Dmitry Mishin <dim(a)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",
11:05 a.m.
On 29/12/15 01:55, "John Ferlan" <jferlan(a)redhat.com> wrote:
On 12/26/2015 12:18 PM, Dmitry Mishin wrote:
> Signed-off-by: Dmitry Mishin <dim(a)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",
>
3251
days inactive
3254
days old
2 comments
2 participants
participants (2)
-
Dmitry Mishin -
John Ferlan