[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/4] Validate the bios_date format for <sysinfo>



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



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]