Re: [libvirt] [PATCH 09/11] storage_scsi: Allow the direct PCI address for 'parent'

20 Jun 2013


      On 06/20/2013 05:21 AM, Osier Yang wrote:
...
On 20/06/13 02:28, John Ferlan wrote:
...
On 06/07/2013 01:16 PM, Osier Yang wrote:
...
On 08/06/13 01:03, Osier Yang wrote:
...
...
return NULL;
       }
   -    if (!(tokens = virStringSplit(parent, "_", 0)))
-        return NULL;
+    if (strchr(parent, '_')) {
+        if (!(tokens = virStringSplit(parent, "_", 0)))
+            return NULL;
   -    len = virStringListLength(tokens);
+        length = virStringListLength(tokens);
+
+        switch (length) {
+        case 4:
+            if (!(ret = virStringJoin((const char **)(&tokens[1]),
":")))
+                goto cleanup;
+            break;
   -    switch (len) {
-    case 4:
-        if (!(ret = virStringJoin((const char **)(&tokens[1]), ":")))
+        case 2:
+            vendor = tokens[1];
+            product = tokens[2];
+            if (!(ret = virFindPCIDeviceByVPD(NULL, vendor, product))) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("Unable to find PCI device with
vendor '%s' "
+                                 "product '%s'"), vendor, product);
+                goto cleanup;
+            }
+
+            break;
+        default:
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("'parent' of scsi_host adapter must be "
+                         "either consistent with name of mode "
+                         "device, or in domain:bus:slot:function "
+                         "format"));
Duplicated error message - same issues as before, plus I think you need
to consider determining which of the two places you got the error.  That
is if we see that message, then did we get an error because there wasn't
a "_" or ":" in the name or (in this case) because the address was
malformed since we expected only 2 or 4 numbers with a specific
separator but found more or less.  In this case, I would think you could
just indicate the parent %s is malformed, requires only 2 or 4 separators.
I don't think so, indicate it requires 2 or 4 separators doesn't give the
user what we expect clearly. That's why I use "duplicate" error messages,
even if
+    if (!strchr(parent, '_') &&
+        !strchr(parent, ':')) {
is false, we still have to let the user know what the format we expect for
"parent" attribute.
...
...
...
goto cleanup;
-        break;
+        }
+    } else if (strchr(parent, ':')) {
+        char *padstr = NULL;
+
+        if (!(tokens = virStringSplit(parent, ":", 0)))
+            return NULL;
   -    case 2:
-        vendor = tokens[1];
-        product = tokens[2];
-        if (!(ret = virFindPCIDeviceByVPD(NULL, vendor, product))) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("Unable to find PCI device with vendor
'%s' "
-                             "product '%s'"), vendor, product);
+        length = virStringListLength(tokens);
+
+        if (length != 4) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("invalid PCI address for scsi_host "
+                             "'parent' '%s'"), parent);
               goto cleanup;
           }
   -        break;
-    default:
-        virReportError(VIR_ERR_XML_ERROR, "%s",
-                       _("'parent' of scsi_host adapter must "
-                         "be consistent with name of node device"));
-        goto cleanup;
+        for (i = 0; i < length; i++) {
+            if (strlen(tokens[i]) == 0) {
+                virReportError(VIR_ERR_XML_ERROR,
+                               _("invalid PCI address for scsi_host "
+                                 "'parent' '%s'"), parent);
+                goto cleanup;
+            }
+        }
+
+        /* sysfs always exposes the PCI address like "0000:00:1f:2",
+         * this do the padding if the address prodived by user is
s/prodived/provided
...
...
+         * not padded (e.g. 0:0:2:0).
+         */
+        if (strlen(tokens[0]) != 4) {
+            if (!(padstr = virStringPad(tokens[0], '0', 4, false)))
+                goto cleanup;
+
+            virBufferAsprintf(&buf, "%s", padstr);
+            VIR_FREE(padstr);
+        } else {
+            virBufferAsprintf(&buf, "%s", tokens[0]);
+        }
+
+        for (i = 1; i < 3; i++) {
+            if (strlen(tokens[i]) != 2) {
+                if (!(padstr = virStringPad(tokens[i], '0', 2, false)))
+                    goto error;
+                virBufferAsprintf(&buf, "%s", padstr);
I think the following syntax will avoid any sort of virStringPad() and
whatever is going on above
virBufferAsprintf(&buf, "%04x:02x:02x:%s",
                   atoi(tokens[0]), atoi(tokens[1]),
                   atoi(tokens[2]), tokens[3]);
Assuming of course that each field is a base16 value and atoi() is "OK"
to use here...
glibc says atoi is absolete, and since it's not required to do any error
checking, strtol is recommended.
In libvirt, we have wrapper for strtol: virStrTo*.
But I don't see it's better than using virStringPad if converting the string
into int using virStringTo*.  We have to check the return value of virStringTo*
anyway here, because the user could input crazy data, e.g.
1234566789101112:01:02:02
Osier
What if we separated the fields in the XML? It feels wrong to store data as a
string separated by underscores, only to have to parse it again.

Instead of
  <adapter type='scsi_host' parent='pci_0000_00_1f_2' unique_id='2'/>
We could do:
  <adapter type='scsi_host' unique_id='2'>
    <parent>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
    </parent>
  </adapter>

or:
<parent>
  <vendor>0x8086</vendor>
  <device>0x1e03</device>
</parent>

Jan

Re: [libvirt] [PATCH 09/11] storage_scsi: Allow the direct PCI address for 'parent'

Ján Tomko