On 05/09/2013 01:43 PM, John Ferlan wrote:
> On 05/09/2013 06:59 AM, Martin Kletzander wrote:
>> On 04/30/2013 08:19 PM, John Ferlan wrote:
>>> ---
>>> src/conf/domain_conf.c | 24 ++++++++++++++++++++++++
>>> 1 file changed, 24 insertions(+)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index a8b5dfd..43273f8 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -11591,6 +11591,30 @@ virDomainDefParseXML(xmlDocPtr xml,
>>> goto error;
>>> }
>>> }
>>> + if (def->sysinfo->bios_date != NULL) {
>>> + char *date = def->sysinfo->bios_date;
>>> + char *ptr;
>>> + struct tm tm;
>>> + memset(&tm, 0, sizeof(tm));
>>> +
>>> + /* Validate just the format of the date
>>> + * Expect mm/dd/yyyy or mm/dd/yy,
>>> + * where yy must be 00->99 and would be assumed to be 19xx
>>> + * a yyyy date should be 1900 and beyond
>>> + */
>>> + if (virStrToLong_i(date, &ptr, 10, &tm.tm_mon) < 0 ||
>>> + *ptr != '/' ||
>>> + virStrToLong_i(ptr+1, &ptr, 10, &tm.tm_mday) < 0 ||
>>> + *ptr != '/' ||
>>> + virStrToLong_i(ptr+1, &ptr, 10, &tm.tm_year) < 0 ||
>>> + *ptr != '\0' ||
>>> + (tm.tm_year < 0 || (tm.tm_year >= 100 && tm.tm_year < 1900))) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>
>> Seems like another abuse of internal error, but I don't know what to use here,
>> properly. Maybe VIR_ERR_XML_DETAIL?
>>
>>> + _("Invalid BIOS 'date' format: %s"),
>>> + def->sysinfo->bios_date);
>>
>> Unnecessarily long, you can do 's/def->sysinfo->bios_//' and save one
>> line here ;-)
>>
>>> + goto error;
>>> + }
>>> + }
>>> }
>>>
>>> if ((tmp = virXPathString("string(./os/smbios/@mode)", ctxt))) {
>>>
>
> FYI: The above is essentially a cut-n-reformat for this particular need
> of virDomainGraphicsAuthDefParseXML(). And while I agree it's an eye
> strain to read - I also tried various strptime() formats then using
> strftime() to format it back..
>
I haven't seen it being used somewhere else, but makes sense also due
to the rest of the mail.
>>
>> I find it a bit harder to read. Wouldn't this be more nicer if we used
>> sscanf()? Or we could take care a bit about the date and do it even
>> shorter with strptime(), something like this:
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index d55ce6b..61f385c 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -11588,6 +11588,20 @@ virDomainDefParseXML(xmlDocPtr xml,
>> goto error;
>> }
>> }
>> + if (def->sysinfo->bios_date != NULL) {
>> + char *date = def->sysinfo->bios_date;
>> + char *end;
>> + struct tm tm;
>> + memset(&tm, 0, sizeof(struct tm));
>> +
>> + end = strptime(date, "%D", &tm);
>
> I did try using strptime() in order to validate, but it was far from
> perfect, although easier to read...
>
> The %D is the equivalent to %m/%d/%y which doesn't work when the date is
> presented as "5/9/2013" a resulting strftime() provides "05/09/20". The
> "best" format has been "%m/%d/%Y" and it's perfectly reasonable to use
> it rather than the virStrToLong_i() calls.
>
I was sure that %y can take both 2 and 4 digit year numbers, but after
trying that one more time, you're right.
> The purpose for the tm_year validation/check comes from the spec which
> has requirement regarding using 'yy' vs. 'yyyy'. In particular, is
> 1/1/1850 a valid date? Well yes, technically according to strptime(),
> but not necessarily "right" according to the spec.
>
> There is an SMBIOS spec which describes the various fields and their
> requirements. See page 28 of the following:
>
> http://dmtf.org/sites/default/files/standards/documents/DSP0134_2.8.0.pdf
>
>> +
>> + if (!end || *end != '\0') {
>> + virReportError(VIR_ERR_XML_DETAIL,
>> + _("Invalid BIOS 'date' format: %s"), date);
>> + goto error;
>> + }
>> + }
>> }
>>
>> if ((tmp = virXPathString("string(./os/smbios/@mode)", ctxt))) {
>> --
>>
>> Or should we allow even dates like "99/99/9999"?
>
> Which would fail using strptime(), but not the above algorithm.
>
Yes, that's why I asked, but I definitely don't insist on such strict
checking.
I haven't thought my proposal through enough and what you say makes more sense, so ACK. Feel free to squash in the test proposed below as an ACK from your side.
>>
>> Martin
>>
>> P.S.: I don't mean to be rude with nit-picking, but a test for that
>> would be nice ;-)
>
> Nit picking is fine - wasn't quite sure where to put a test on something
> like this.
>
I'd add it to qemuxml2argvtest with invalid date and
DO_TEST_PARSE_ERROR.
Example (feel free to use it, it's tested):
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios-date.xml b/tests/qemuxml2argvdata/qemuxml2argv-smbios-date.xml
new file mode 100644
index 0000000..7b2f33a
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios-date.xml
@@ -0,0 +1,23 @@
+<domain type='qemu'>
+ <name>smbios</name>
+ <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid>
+ <memory unit='KiB'>1048576</memory>
+ <currentMemory unit='KiB'>1048576</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <smbios mode="sysinfo"/>
+ </os>
+ <sysinfo type="smbios">
+ <bios>
+ <entry name="date">999/999/123</entry>
+ </bios>
+ </sysinfo>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>restart</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 1286273..7d5c3d0 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -814,6 +814,7 @@ mymain(void)
DO_TEST("smbios", QEMU_CAPS_SMBIOS_TYPE);
+ DO_TEST_PARSE_ERROR("smbios-date", QEMU_CAPS_SMBIOS_TYPE);
DO_TEST("watchdog", NONE);
DO_TEST("watchdog-device", QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
--
Martin
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list