On 29/12/15 01:55, "John Ferlan" <jferlan@xxxxxxxxxx> wrote:
>
>
>On 12/26/2015 12:18 PM, Dmitry Mishin wrote:
>> Signed-off-by: Dmitry Mishin <dim@xxxxxxxxxxxxx>
>> ---
>> 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",
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list