[libvirt] [PATCH 09/11] storage_scsi: Allow the direct PCI address for 'parent'
Osier Yang
jyang at redhat.com
Thu Jun 20 03:21:16 UTC 2013
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
More information about the libvir-list
mailing list