Re: [PATCH v3 01/14] domain_conf: move boot timeouts check to domain_validate.c
On 12/9/20 7:52 AM, Daniel P. Berrangé wrote: On Wed, Dec 09, 2020 at 07:35:09AM -0300, Daniel Henrique Barboza wrote: On 12/9/20 5:13 AM, Michal Privoznik wrote: On 12/8/20 11:20 PM, Daniel Henrique Barboza wrote: This patch creates a new function, virDomainDefBootValidate(), to host the validation of boot menu timeout and rebootTimeout outside of parse time. The checks in virDomainDefParseBootXML() were changed to throw VIR_ERR_XML_ERROR in case of parse error of those values. In an attempt to alleviate the amount of code being stacked inside domain_conf.c, let's put this new function in a new domain_validate.c file that will be used to place these validations. Suggested-by: Michal Privoznik Signed-off-by: Daniel Henrique Barboza [...] diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c new file mode 100644 index 00..e4d947c553 --- /dev/null +++ b/src/conf/domain_validate.c @@ -0,0 +1,51 @@ +/* + * domain_validate.c: domain general validation functions + * + * Copyright IBM Corp, 2020 Honestly, I have only vague idea how these Copyright lines work, but shouldn't they also include (at least subset) of the lines from the original file? I mean, my common sense tells me that if I have a file written by person X, and later the file is split into two the person X is still the original author. Extending that, if a company holds a copyright on a file then moving bits out to a different file should keep the copyright. But I admit that law has completely different model of "common sense". And also there is a disconnection between files and these Copyright lines. If a copyright holder Y changed a tiny bit that is not moved - should their Copyright line also appear in the new file? TBH I have no idea what's the best practice here. What I did was simply copy the Copyright header from qemu_validate.c. I believe I can add a "This file was based on src/qemu/qemu_validate.c", since the inspiration is quite obvious in this case, right after the legal text. I see some people doing this in QEMU. If you're copying code from one file to a new file, then the simplest way is to copy the existing copyright headers unchanged. Not copying a copyright header would require you to audit the history of the code to determine whether it is valid to exclude them. This is usually a waste of time, so preserving the full existing copyright headers is normal practice for sake of speed. You can then optionally also add new copyright lines if you're also making code changes after the copy. I see people putting copyright nominal in their own name as well. I guess this means that the original author wasn't bind to a company contract by the time the file was created or something like that. I am not sure about the implications of having a copyright in your own name, aside from people emailing you to ask for a license change (Linus and the GPLv3 versus LGPLv2 comes to mind). A combination of your country's laws, and your employer's rules generally determine whether copyright statements have to be in your name or your employer's name. Most commonly it is your employer's name, as most companies require their employees to assign copyright on all their work as part of terms of employment. There are exceptions though, so I can't explicitly say what is right for you personally. Thanks for the info. Just double checked internally and my case is the most most common scenario (copyright is in the employer's name). So it's all good. DHB Regards, Daniel
Re: [PATCH v3 01/14] domain_conf: move boot timeouts check to domain_validate.c
On Wed, Dec 09, 2020 at 07:35:09AM -0300, Daniel Henrique Barboza wrote: > > > On 12/9/20 5:13 AM, Michal Privoznik wrote: > > On 12/8/20 11:20 PM, Daniel Henrique Barboza wrote: > > > This patch creates a new function, virDomainDefBootValidate(), to host > > > the validation of boot menu timeout and rebootTimeout outside of parse > > > time. The checks in virDomainDefParseBootXML() were changed to throw > > > VIR_ERR_XML_ERROR in case of parse error of those values. > > > > > > In an attempt to alleviate the amount of code being stacked inside > > > domain_conf.c, let's put this new function in a new domain_validate.c > > > file that will be used to place these validations. > > > > > > Suggested-by: Michal Privoznik > > > Signed-off-by: Daniel Henrique Barboza > > [...] > > > > diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c > > > new file mode 100644 > > > index 00..e4d947c553 > > > --- /dev/null > > > +++ b/src/conf/domain_validate.c > > > @@ -0,0 +1,51 @@ > > > +/* > > > + * domain_validate.c: domain general validation functions > > > + * > > > + * Copyright IBM Corp, 2020 > > > > Honestly, I have only vague idea how these Copyright lines work, but > > shouldn't they also include (at least subset) of the lines from the > > original file? I mean, my common sense tells me that if I have a file > > written by person X, and later the file is split into two the person X is > > still the original author. Extending that, if a company holds a copyright > > on a file then moving bits out to a different file should keep the > > copyright. But I admit that law has completely different model of "common > > sense". And also there is a disconnection between files and these Copyright > > lines. If a copyright holder Y changed a tiny bit that is not moved - > > should their Copyright line also appear in the new file? > > TBH I have no idea what's the best practice here. What I did was simply > copy the Copyright header from qemu_validate.c. I believe I can add > a "This file was based on src/qemu/qemu_validate.c", since the inspiration > is quite obvious in this case, right after the legal text. I see some > people doing this in QEMU. If you're copying code from one file to a new file, then the simplest way is to copy the existing copyright headers unchanged. Not copying a copyright header would require you to audit the history of the code to determine whether it is valid to exclude them. This is usually a waste of time, so preserving the full existing copyright headers is normal practice for sake of speed. You can then optionally also add new copyright lines if you're also making code changes after the copy. > I see people putting copyright nominal in their own name as well. I guess > this means that the original author wasn't bind to a company contract by > the time the file was created or something like that. I am not sure about > the implications of having a copyright in your own name, aside from people > emailing you to ask for a license change (Linus and the GPLv3 versus LGPLv2 > comes to mind). A combination of your country's laws, and your employer's rules generally determine whether copyright statements have to be in your name or your employer's name. Most commonly it is your employer's name, as most companies require their employees to assign copyright on all their work as part of terms of employment. There are exceptions though, so I can't explicitly say what is right for you personally. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v3 01/14] domain_conf: move boot timeouts check to domain_validate.c
On 12/9/20 5:13 AM, Michal Privoznik wrote: On 12/8/20 11:20 PM, Daniel Henrique Barboza wrote: This patch creates a new function, virDomainDefBootValidate(), to host the validation of boot menu timeout and rebootTimeout outside of parse time. The checks in virDomainDefParseBootXML() were changed to throw VIR_ERR_XML_ERROR in case of parse error of those values. In an attempt to alleviate the amount of code being stacked inside domain_conf.c, let's put this new function in a new domain_validate.c file that will be used to place these validations. Suggested-by: Michal Privoznik Signed-off-by: Daniel Henrique Barboza [...] diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c new file mode 100644 index 00..e4d947c553 --- /dev/null +++ b/src/conf/domain_validate.c @@ -0,0 +1,51 @@ +/* + * domain_validate.c: domain general validation functions + * + * Copyright IBM Corp, 2020 Honestly, I have only vague idea how these Copyright lines work, but shouldn't they also include (at least subset) of the lines from the original file? I mean, my common sense tells me that if I have a file written by person X, and later the file is split into two the person X is still the original author. Extending that, if a company holds a copyright on a file then moving bits out to a different file should keep the copyright. But I admit that law has completely different model of "common sense". And also there is a disconnection between files and these Copyright lines. If a copyright holder Y changed a tiny bit that is not moved - should their Copyright line also appear in the new file? TBH I have no idea what's the best practice here. What I did was simply copy the Copyright header from qemu_validate.c. I believe I can add a "This file was based on src/qemu/qemu_validate.c", since the inspiration is quite obvious in this case, right after the legal text. I see some people doing this in QEMU. I see people putting copyright nominal in their own name as well. I guess this means that the original author wasn't bind to a company contract by the time the file was created or something like that. I am not sure about the implications of having a copyright in your own name, aside from people emailing you to ask for a license change (Linus and the GPLv3 versus LGPLv2 comes to mind). Anyway, I'm happy to hear suggestions about how to handle this copyright text :) Thanks, DHB Michal
Re: [PATCH v3 01/14] domain_conf: move boot timeouts check to domain_validate.c
On 12/8/20 11:20 PM, Daniel Henrique Barboza wrote:
This patch creates a new function, virDomainDefBootValidate(), to host
the validation of boot menu timeout and rebootTimeout outside of parse
time. The checks in virDomainDefParseBootXML() were changed to throw
VIR_ERR_XML_ERROR in case of parse error of those values.
In an attempt to alleviate the amount of code being stacked inside
domain_conf.c, let's put this new function in a new domain_validate.c
file that will be used to place these validations.
Suggested-by: Michal Privoznik
Signed-off-by: Daniel Henrique Barboza
---
po/POTFILES.in | 1 +
src/conf/domain_conf.c | 20 +++
src/conf/domain_validate.c | 51 ++
src/conf/domain_validate.h | 27
src/conf/meson.build | 1 +
tests/qemuxml2argvtest.c | 3 ++-
6 files changed, 92 insertions(+), 11 deletions(-)
create mode 100644 src/conf/domain_validate.c
create mode 100644 src/conf/domain_validate.h
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 9782b2beb4..14636d4b93 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -26,6 +26,7 @@
@SRCDIR@src/conf/domain_capabilities.c
@SRCDIR@src/conf/domain_conf.c
@SRCDIR@src/conf/domain_event.c
+@SRCDIR@src/conf/domain_validate.c
@SRCDIR@src/conf/interface_conf.c
@SRCDIR@src/conf/netdev_bandwidth_conf.c
@SRCDIR@src/conf/netdev_vlan_conf.c
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 66ee658e7b..4a45c76cf1 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -33,6 +33,7 @@
#include "datatypes.h"
#include "domain_addr.h"
#include "domain_conf.h"
+#include "domain_validate.h"
#include "snapshot_conf.h"
#include "viralloc.h"
#include "virxml.h"
@@ -7344,6 +7345,9 @@ virDomainDefValidateInternal(const virDomainDef *def,
if (virDomainDefCputuneValidate(def) < 0)
return -1;
+if (virDomainDefBootValidate(def) < 0)
+return -1;
+
if (virDomainNumaDefValidate(def->numa) < 0)
return -1;
@@ -18867,11 +18871,9 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt,
tmp = virXMLPropString(node, "timeout");
if (tmp && def->os.bootmenu == VIR_TRISTATE_BOOL_YES) {
-if (virStrToLong_uip(tmp, NULL, 0, >os.bm_timeout) < 0 ||
-def->os.bm_timeout > 65535) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("invalid value for boot menu timeout, "
- "must be in range [0,65535]"));
+if (virStrToLong_uip(tmp, NULL, 0, >os.bm_timeout) < 0) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("invalid value for boot menu timeout"));
return -1;
}
def->os.bm_timeout_set = true;
@@ -18892,11 +18894,9 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt,
if (tmp) {
/* that was really just for the check if it is there */
-if (virStrToLong_i(tmp, NULL, 0, >os.bios.rt_delay) < 0 ||
-def->os.bios.rt_delay < -1 || def->os.bios.rt_delay > 65535) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("invalid value for rebootTimeout, "
- "must be in range [-1,65535]"));
+if (virStrToLong_i(tmp, NULL, 0, >os.bios.rt_delay) < 0) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("invalid value for rebootTimeout"));
return -1;
}
def->os.bios.rt_set = true;
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
new file mode 100644
index 00..e4d947c553
--- /dev/null
+++ b/src/conf/domain_validate.c
@@ -0,0 +1,51 @@
+/*
+ * domain_validate.c: domain general validation functions
+ *
+ * Copyright IBM Corp, 2020
Honestly, I have only vague idea how these Copyright lines work, but
shouldn't they also include (at least subset) of the lines from the
original file? I mean, my common sense tells me that if I have a file
written by person X, and later the file is split into two the person X
is still the original author. Extending that, if a company holds a
copyright on a file then moving bits out to a different file should keep
the copyright. But I admit that law has completely different model of
"common sense". And also there is a disconnection between files and
these Copyright lines. If a copyright holder Y changed a tiny bit that
is not moved - should their Copyright line also appear in the new file?
Michal
[PATCH v3 01/14] domain_conf: move boot timeouts check to domain_validate.c
This patch creates a new function, virDomainDefBootValidate(), to host
the validation of boot menu timeout and rebootTimeout outside of parse
time. The checks in virDomainDefParseBootXML() were changed to throw
VIR_ERR_XML_ERROR in case of parse error of those values.
In an attempt to alleviate the amount of code being stacked inside
domain_conf.c, let's put this new function in a new domain_validate.c
file that will be used to place these validations.
Suggested-by: Michal Privoznik
Signed-off-by: Daniel Henrique Barboza
---
po/POTFILES.in | 1 +
src/conf/domain_conf.c | 20 +++
src/conf/domain_validate.c | 51 ++
src/conf/domain_validate.h | 27
src/conf/meson.build | 1 +
tests/qemuxml2argvtest.c | 3 ++-
6 files changed, 92 insertions(+), 11 deletions(-)
create mode 100644 src/conf/domain_validate.c
create mode 100644 src/conf/domain_validate.h
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 9782b2beb4..14636d4b93 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -26,6 +26,7 @@
@SRCDIR@src/conf/domain_capabilities.c
@SRCDIR@src/conf/domain_conf.c
@SRCDIR@src/conf/domain_event.c
+@SRCDIR@src/conf/domain_validate.c
@SRCDIR@src/conf/interface_conf.c
@SRCDIR@src/conf/netdev_bandwidth_conf.c
@SRCDIR@src/conf/netdev_vlan_conf.c
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 66ee658e7b..4a45c76cf1 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -33,6 +33,7 @@
#include "datatypes.h"
#include "domain_addr.h"
#include "domain_conf.h"
+#include "domain_validate.h"
#include "snapshot_conf.h"
#include "viralloc.h"
#include "virxml.h"
@@ -7344,6 +7345,9 @@ virDomainDefValidateInternal(const virDomainDef *def,
if (virDomainDefCputuneValidate(def) < 0)
return -1;
+if (virDomainDefBootValidate(def) < 0)
+return -1;
+
if (virDomainNumaDefValidate(def->numa) < 0)
return -1;
@@ -18867,11 +18871,9 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt,
tmp = virXMLPropString(node, "timeout");
if (tmp && def->os.bootmenu == VIR_TRISTATE_BOOL_YES) {
-if (virStrToLong_uip(tmp, NULL, 0, >os.bm_timeout) < 0 ||
-def->os.bm_timeout > 65535) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("invalid value for boot menu timeout, "
- "must be in range [0,65535]"));
+if (virStrToLong_uip(tmp, NULL, 0, >os.bm_timeout) < 0) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("invalid value for boot menu timeout"));
return -1;
}
def->os.bm_timeout_set = true;
@@ -18892,11 +18894,9 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt,
if (tmp) {
/* that was really just for the check if it is there */
-if (virStrToLong_i(tmp, NULL, 0, >os.bios.rt_delay) < 0 ||
-def->os.bios.rt_delay < -1 || def->os.bios.rt_delay > 65535) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("invalid value for rebootTimeout, "
- "must be in range [-1,65535]"));
+if (virStrToLong_i(tmp, NULL, 0, >os.bios.rt_delay) < 0) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("invalid value for rebootTimeout"));
return -1;
}
def->os.bios.rt_set = true;
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
new file mode 100644
index 00..e4d947c553
--- /dev/null
+++ b/src/conf/domain_validate.c
@@ -0,0 +1,51 @@
+/*
+ * domain_validate.c: domain general validation functions
+ *
+ * Copyright IBM Corp, 2020
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ *
