On 05/09/2013 09:58 AM, Martin Kletzander wrote:
> 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 guess I agree in principal that a month of 99 or a date of 99 would be
incorrect and since we're doing some sort of validation it wouldn't hurt
to do a bit more. Doing full blown is the days in the month right and
handling leap year - is just outside the realm. My guess is that
somewhere some code will do a similar strptime() like call anyway.
So I made the change:
*ptr != '/' ||
virStrToLong_i(ptr+1, &ptr, 10, &tm.tm_year) < 0 ||
*ptr != '\0' ||
+ (tm.tm_mon < 0 || tm.tm_mon > 12) ||
+ (tm.tm_mday < 0 || tm.tm_mday > 31) ||
(tm.tm_year < 0 || (tm.tm_year >= 100 && tm.tm_year <
1900))) {
- virReportError(VIR_ERR_XML_DETAIL,
- _("Invalid BIOS 'date' format: %s"),
- def->sysinfo->bios_date);
+ virReportError(VIR_ERR_XML_DETAIL, "%s",
+ _("Invalid BIOS 'date' format"));
>>>
...
And did squash/add the test provided - thanks! I also tried a couple of
other dates (both good and bad) during self testing to make sure the
code validated properly...
Going to do something similar with uuid validation shortly...
John
>
> 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