6:13 p.m.
Explain why we check it at process startup time, and not parse time
where most other XML validation checks are performed
---
src/qemu/qemu_process.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c087300..628b4b6 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4550,6 +4550,8 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
virDomainDefCheckDuplicateDiskInfo(vm->def) < 0)
return -1;
+ /* Previously we silently accepted this parameter; we can't reject
+ it at parse time without breaking those configs, so check it here */
if (vm->def->mem.min_guarantee) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Parameter 'min_guarantee' "
--
2.7.3
3:39 a.m.
On Mon, Apr 18, 2016 at 19:13:08 -0400, Cole Robinson wrote:
Explain why we check it at process startup time, and not parse time
where most other XML validation checks are performed
This is far from being a singular case ...
---
src/qemu/qemu_process.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c087300..628b4b6 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4550,6 +4550,8 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
virDomainDefCheckDuplicateDiskInfo(vm->def) < 0)
... this is yet another example of a similar check.
return -1;
+ /* Previously we silently accepted this parameter; we can't reject
+ it at parse time without breaking those configs, so check it here */
I don't think this helps here and implies that other checks are not here
due to that case. If you want to be explicit I think it warrants a
separate function with this fact stated in it's comment. If you insist
on being explicit in the purpose of this check
if (vm->def->mem.min_guarantee) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Parameter 'min_guarantee' "
--
2.7.3
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
11:03 a.m.
On 04/19/2016 04:39 AM, Peter Krempa wrote:
On Mon, Apr 18, 2016 at 19:13:08 -0400, Cole Robinson wrote:
> Explain why we check it at process startup time, and not parse time
> where most other XML validation checks are performed
This is far from being a singular case ...
> ---
> src/qemu/qemu_process.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index c087300..628b4b6 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4550,6 +4550,8 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
> virDomainDefCheckDuplicateDiskInfo(vm->def) < 0)
... this is yet another example of a similar check.
> return -1;
>
> + /* Previously we silently accepted this parameter; we can't reject
> + it at parse time without breaking those configs, so check it here */
I don't think this helps here and implies that other checks are not here
due to that case. If you want to be explicit I think it warrants a
separate function with this fact stated in it's comment. If you insist
on being explicit in the purpose of this check
It's not a matter of 'insist'ing or not; when in this area of the code I saw
the min_guarantee check, which seemed out of place, since there are already
several such checks in the postparse handler.There's no comment explaining why
the min_guarantee check is there specifically (or the other XML checks), and
nothing specific in the commit message for the min_guarantee block. Hence my
confusion and desire to document it, and hopefully save other people the
confusion, and prevent future XML validation checks being added there which
are safe to do at parse time.
So it seems like the XML validation checks are:
if (qemuValidateCpuCount(vm->def, qemuCaps) < 0)
return -1;
if (!migration && !snapshot &&
virDomainDefCheckDuplicateDiskInfo(vm->def) < 0)
return -1;
if (vm->def->mem.min_guarantee) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Parameter 'min_guarantee' "
"not supported by QEMU."));
return -1;
}
And they are explicitly done at process startup time for back compat reasons.
So I'll stick them in a function like qemuProcessStartValidateXML(), with a
comment explaining why here and not at parse time. Sound good?
Thanks,
Cole
2:29 a.m.
On Tue, Apr 19, 2016 at 12:03:08 -0400, Cole Robinson wrote:
It's not a matter of 'insist'ing or not; when in this
area of the code I saw
the min_guarantee check, which seemed out of place, since there are already
several such checks in the postparse handler.There's no comment explaining why
the min_guarantee check is there specifically (or the other XML checks), and
nothing specific in the commit message for the min_guarantee block. Hence my
confusion and desire to document it, and hopefully save other people the
confusion, and prevent future XML validation checks being added there which
are safe to do at parse time.
Well I think the semantics of adding checks are very simple. If you are
adding a new feature/element you can add it to the post parse check. If
you are trying to check something that was already released you should
not add it to the post parse check. Otherwise defined domains may
vanish.
...
And they are explicitly done at process startup time for back compat
reasons.
So I'll stick them in a function like qemuProcessStartValidateXML(), with a
comment explaining why here and not at parse time. Sound good?
Yes.
Peter
4:02 p.m.
On 04/20/2016 03:29 AM, Peter Krempa wrote:
On Tue, Apr 19, 2016 at 12:03:08 -0400, Cole Robinson wrote:
> It's not a matter of 'insist'ing or not; when in this area of the code I
saw
> the min_guarantee check, which seemed out of place, since there are already
> several such checks in the postparse handler.There's no comment explaining why
> the min_guarantee check is there specifically (or the other XML checks), and
> nothing specific in the commit message for the min_guarantee block. Hence my
> confusion and desire to document it, and hopefully save other people the
> confusion, and prevent future XML validation checks being added there which
> are safe to do at parse time.
Well I think the semantics of adding checks are very simple. If you are
adding a new feature/element you can add it to the post parse check. If
you are trying to check something that was already released you should
not add it to the post parse check. Otherwise defined domains may
vanish.
Yes they are simple... once you know them ;) I didn't when wandering into that
part of the code, hence the desire to document it.
> And they are explicitly done at process startup time for back compat reasons.
> So I'll stick them in a function like qemuProcessStartValidateXML(), with a
> comment explaining why here and not at parse time. Sound good?
Yes.
Thanks, patches sent
- Cole
3140
days inactive
3142
days old
4 comments
2 participants
participants (2)
-
Cole Robinson -
Peter Krempa