10:21 p.m.
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:
>> To be more flexible, except allowing to specify 'parent' with name
>> produced by node device udev/HAL backends, this supports to specify
>> 'parent' with PCI address directly (e.g. 0000:00:1f:2). The specified
>> address will be padded if it's not consistent with what sysfs exposed.
>> (e.g 0:0:2:2 will be padded to 0000:00:02:2).
>> ---
>> src/storage/storage_backend_scsi.c | 117
>> +++++++++++++++++++++++++++++--------
>> 1 file changed, 93 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/storage/storage_backend_scsi.c
>> b/src/storage/storage_backend_scsi.c
>> index a77b9ae..5635f73 100644
>> --- a/src/storage/storage_backend_scsi.c
>> +++ b/src/storage/storage_backend_scsi.c
>> @@ -604,51 +604,120 @@ out:
>> static char *
>> getScsiHostParentAddress(const char *parent)
>> {
>> + virBuffer buf = VIR_BUFFER_INITIALIZER;
>> char **tokens = NULL;
>> char *vendor = NULL;
>> char *product = NULL;
>> char *ret = NULL;
>> - int len;
>> + int length;
>> + int i;
>> - if (!strchr(parent, '_')) {
>> + if (!strchr(parent, '_') &&
>> + !strchr(parent, ':')) {
>> virReportError(VIR_ERR_XML_ERROR, "%s",
>> - _("'parent' of scsi_host adapter must
"
>> - "be consistent with name of node device"));
>> + _("'parent' of scsi_host adapter must be
"
>> + "either consistent with name of mode "
>> + "device, or in domain:bus:slot:function "
>> + "format"));
"name of the node device or in"
not
"name of mode device, or in"
Okay.
>> 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