[libvirt] [PATCH 0/4 v4] New API virNodeDeviceLookupSCSIHostByWWN

v3 - v4: * Only rebasing v2 - v3: * Validate the specified wwnn,wwpn pair before applying it to the new API in virsh-nodedev.c v1 - v2: * Per Daniel's suggestion, change the API name from virNodeDeviceLookupByWWN into virNodeDeviceLookupSCSIHostByWWN. Osier Yang (4): Introduce API virNodeDeviceLookupSCSIHostByWWN remote: Wire up the remote protocol nodedev: Implement virNodeDeviceLookupSCSIHostByWWN virsh: Use virNodeDeviceLookupSCSIHostByWWN include/libvirt/libvirt.h.in | 5 ++ src/driver.h | 6 ++ src/libvirt.c | 46 ++++++++++++++++ src/libvirt_public.syms | 5 ++ src/node_device/node_device_driver.c | 13 +++-- src/node_device/node_device_driver.h | 4 ++ src/node_device/node_device_hal.c | 1 + src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 13 ++++- src/remote_protocol-structs | 9 +++ src/rpc/gendispatch.pl | 5 ++- tools/virsh-nodedev.c | 98 ++++++++++++++++++++++++++-------- tools/virsh.pod | 15 +++-- 14 files changed, 187 insertions(+), 35 deletions(-) -- 1.7.7.6

Since the name (like scsi_host10) is not stable for vHBA, (it can be changed either after recreating or system rebooting), current API virNodeDeviceLookupByName is not nice to use for management app in this case. (E.g. one wants to destroy the vHBA whose name has been changed after system rebooting, he has to find out current name first). Later patches will support the persistent vHBA via storage pool, with which one can identify the vHBA stably by the wwnn && wwpn pair. So this new API comes. --- include/libvirt/libvirt.h.in | 5 ++++ src/driver.h | 6 +++++ src/libvirt.c | 46 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 4 files changed, 62 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 30762b1..ad30cd0 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3257,6 +3257,11 @@ int virConnectListAllNodeDevices (virConnectPtr conn, virNodeDevicePtr virNodeDeviceLookupByName (virConnectPtr conn, const char *name); +virNodeDevicePtr virNodeDeviceLookupSCSIHostByWWN (virConnectPtr conn, + const char *wwnn, + const char *wwpn, + unsigned int flags); + const char * virNodeDeviceGetName (virNodeDevicePtr dev); const char * virNodeDeviceGetParent (virNodeDevicePtr dev); diff --git a/src/driver.h b/src/driver.h index 02ddd83..8d0f0a5 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1553,6 +1553,11 @@ typedef int (*virDevMonListAllNodeDevices)(virConnectPtr conn, typedef virNodeDevicePtr (*virDevMonDeviceLookupByName)(virConnectPtr conn, const char *name); +typedef virNodeDevicePtr (*virDevMonDeviceLookupSCSIHostByWWN)(virConnectPtr conn, + const char *wwnn, + const char *wwpn, + unsigned int flags); + typedef char * (*virDevMonDeviceGetXMLDesc)(virNodeDevicePtr dev, unsigned int flags); @@ -1584,6 +1589,7 @@ struct _virDeviceMonitor { virDevMonListDevices listDevices; virDevMonListAllNodeDevices listAllNodeDevices; virDevMonDeviceLookupByName deviceLookupByName; + virDevMonDeviceLookupSCSIHostByWWN deviceLookupSCSIHostByWWN; virDevMonDeviceGetXMLDesc deviceGetXMLDesc; virDevMonDeviceGetParent deviceGetParent; virDevMonDeviceNumOfCaps deviceNumOfCaps; diff --git a/src/libvirt.c b/src/libvirt.c index f81a3de..1e78500 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -14334,6 +14334,52 @@ error: return NULL; } +/** + * virNodeDeviceLookupSCSIHostByWWN: + * @conn: pointer to the hypervisor connection + * @wwnn: WWNN of the SCSI Host. + * @wwpn: WWPN of the SCSI Host. + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Lookup SCSI Host which is capable with 'fc_host' by its WWNN and WWPN. + * + * Returns a virNodeDevicePtr if found, NULL otherwise. + */ +virNodeDevicePtr +virNodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, + const char *wwnn, + const char *wwpn, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, wwnn=%p, wwpn=%p, flags=%x", conn, wwnn, wwpn, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return NULL; + } + + virCheckNonNullArgGoto(wwnn, error); + virCheckNonNullArgGoto(wwpn, error); + + if (conn->deviceMonitor && + conn->deviceMonitor->deviceLookupSCSIHostByWWN) { + virNodeDevicePtr ret; + ret = conn->deviceMonitor->deviceLookupSCSIHostByWWN(conn, wwnn, + wwpn, flags); + if (!ret) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return NULL; +} /** * virNodeDeviceGetXMLDesc: diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 9777703..3bdfd57 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -603,4 +603,9 @@ LIBVIRT_1.0.2 { virTypedParamsGetULLong; } LIBVIRT_1.0.1; +LIBVIRT_1.0.3 { + global: + virNodeDeviceLookupSCSIHostByWWN; +} LIBVIRT_1.0.2; + # .... define new API here using predicted next version number .... -- 1.7.7.6

On Mon, Feb 04, 2013 at 09:03:09PM +0800, Osier Yang wrote:
Since the name (like scsi_host10) is not stable for vHBA, (it can be changed either after recreating or system rebooting), current API virNodeDeviceLookupByName is not nice to use for management app in this case. (E.g. one wants to destroy the vHBA whose name has been changed after system rebooting, he has to find out current name first).
Later patches will support the persistent vHBA via storage pool, with which one can identify the vHBA stably by the wwnn && wwpn pair.
So this new API comes. --- include/libvirt/libvirt.h.in | 5 ++++ src/driver.h | 6 +++++ src/libvirt.c | 46 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 4 files changed, 62 insertions(+), 0 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Like virNodeDeviceCreateXML, virNodeDeviceLookupSCSIHostByWWN has to be treated specially when generating the RPC codes. Also new rules are added in fixup_name to keep the name SCSIHostByWWN. --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 13 ++++++++++++- src/remote_protocol-structs | 9 +++++++++ src/rpc/gendispatch.pl | 5 ++++- 4 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 341321b..feb372e 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6359,6 +6359,7 @@ static virDeviceMonitor dev_monitor = { .listDevices = remoteNodeListDevices, /* 0.5.0 */ .listAllNodeDevices = remoteConnectListAllNodeDevices, /* 0.10.2 */ .deviceLookupByName = remoteNodeDeviceLookupByName, /* 0.5.0 */ + .deviceLookupSCSIHostByWWN = remoteNodeDeviceLookupSCSIHostByWWN, /* 1.0.2 */ .deviceGetXMLDesc = remoteNodeDeviceGetXMLDesc, /* 0.5.0 */ .deviceGetParent = remoteNodeDeviceGetParent, /* 0.5.0 */ .deviceNumOfCaps = remoteNodeDeviceNumOfCaps, /* 0.5.0 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 9035776..5ace31f 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1852,6 +1852,16 @@ struct remote_node_device_lookup_by_name_ret { remote_nonnull_node_device dev; }; +struct remote_node_device_lookup_scsi_host_by_wwn_args { + remote_nonnull_string wwnn; + remote_nonnull_string wwpn; + unsigned int flags; +}; + +struct remote_node_device_lookup_scsi_host_by_wwn_ret { + remote_nonnull_node_device dev; +}; + struct remote_node_device_get_xml_desc_args { remote_nonnull_string name; unsigned int flags; @@ -3049,7 +3059,8 @@ enum remote_procedure { REMOTE_PROC_NODE_GET_CPU_MAP = 293, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_FSTRIM = 294, /* autogen autogen */ REMOTE_PROC_DOMAIN_SEND_PROCESS_SIGNAL = 295, /* autogen autogen */ - REMOTE_PROC_DOMAIN_OPEN_CHANNEL = 296 /* autogen autogen | readstream@2 */ + REMOTE_PROC_DOMAIN_OPEN_CHANNEL = 296, /* autogen autogen | readstream@2 */ + REMOTE_PROC_NODE_DEVICE_LOOKUP_SCSI_HOST_BY_WWN = 297 /* autogen autogen priority:high */ /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 91414d4..b8ca88b 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1400,6 +1400,14 @@ struct remote_node_device_lookup_by_name_args { struct remote_node_device_lookup_by_name_ret { remote_nonnull_node_device dev; }; +struct remote_node_device_lookup_scsi_host_by_wwn_args { + remote_nonull_string wwnn; + remote_nonull_string wwpn; + unsigned int flags; +}; +struct remote_node_device_lookup_scsi_host_by_wwn_ret { + remote_nonnull_node_device dev; +}; struct remote_node_device_get_xml_desc_args { remote_nonnull_string name; u_int flags; @@ -2453,4 +2461,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_FSTRIM = 294, REMOTE_PROC_DOMAIN_SEND_PROCESS_SIGNAL = 295, REMOTE_PROC_DOMAIN_OPEN_CHANNEL = 296, + REMOTE_PROC_NODE_DEVICE_LOOKUP_SCSI_HOST_BY_WWN = 297, }; diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index f00ed7c..1da28a4 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -45,6 +45,8 @@ sub fixup_name { $name =~ s/Nmi$/NMI/; $name =~ s/Pm/PM/; $name =~ s/Fstrim$/FSTrim/; + $name =~ s/Scsi/SCSI/; + $name =~ s/Wwn$/WWN/; return $name; } @@ -402,7 +404,8 @@ elsif ($opt_b) { # node device is special, as it's identified by name if ($argtype =~ m/^remote_node_device_/ and !($argtype =~ m/^remote_node_device_lookup_by_name_/) and - !($argtype =~ m/^remote_node_device_create_xml_/)) { + !($argtype =~ m/^remote_node_device_create_xml_/) and + !($argtype =~ m/^remote_node_device_lookup_scsi_host_by_wwn_/)) { $has_node_device = 1; push(@vars_list, "virNodeDevicePtr dev = NULL"); push(@getters_list, -- 1.7.7.6

On Mon, Feb 04, 2013 at 09:03:10PM +0800, Osier Yang wrote:
Like virNodeDeviceCreateXML, virNodeDeviceLookupSCSIHostByWWN has to be treated specially when generating the RPC codes. Also new rules are added in fixup_name to keep the name SCSIHostByWWN. --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 13 ++++++++++++- src/remote_protocol-structs | 9 +++++++++ src/rpc/gendispatch.pl | 5 ++++- 4 files changed, 26 insertions(+), 2 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

This just simply changes nodeDeviceLookupByWWN to be not static, and its name into nodeDeviceLookupSCSIHostByWWN. And use that for udev and HAL backends. --- src/node_device/node_device_driver.c | 13 ++++++++----- src/node_device/node_device_driver.h | 4 ++++ src/node_device/node_device_hal.c | 1 + src/node_device/node_device_udev.c | 1 + 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 2b46041..64fd831 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -224,10 +224,11 @@ cleanup: } -static virNodeDevicePtr -nodeDeviceLookupByWWN(virConnectPtr conn, - const char *wwnn, - const char *wwpn) +virNodeDevicePtr +nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, + const char *wwnn, + const char *wwpn, + unsigned int flags) { unsigned int i; virDeviceMonitorStatePtr driver = conn->devMonPrivateData; @@ -236,6 +237,8 @@ nodeDeviceLookupByWWN(virConnectPtr conn, virNodeDeviceObjPtr obj = NULL; virNodeDevicePtr dev = NULL; + virCheckFlags(0, NULL); + nodeDeviceLock(driver); for (i = 0; i < devs->count; i++) { @@ -546,7 +549,7 @@ find_new_device(virConnectPtr conn, const char *wwnn, const char *wwpn) virFileWaitForDevices(); - dev = nodeDeviceLookupByWWN(conn, wwnn, wwpn); + dev = nodeDeviceLookupSCSIHostByWWN(conn, wwnn, wwpn, 0); if (dev != NULL) { break; diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 5520740..510ae73 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -77,6 +77,10 @@ int nodeListAllNodeDevices(virConnectPtr conn, virNodeDevicePtr **devices, unsigned int flags); virNodeDevicePtr nodeDeviceLookupByName(virConnectPtr conn, const char *name); +virNodeDevicePtr nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, + const char *wwnn, + const char *wwpn, + unsigned int flags); char *nodeDeviceGetXMLDesc(virNodeDevicePtr dev, unsigned int flags); char *nodeDeviceGetParent(virNodeDevicePtr dev); int nodeDeviceNumOfCaps(virNodeDevicePtr dev); diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 610df8d..20714d3 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -765,6 +765,7 @@ static virDeviceMonitor halDeviceMonitor = { .listDevices = nodeListDevices, /* 0.5.0 */ .listAllNodeDevices = nodeListAllNodeDevices, /* 0.10.2 */ .deviceLookupByName = nodeDeviceLookupByName, /* 0.5.0 */ + .deviceLookupSCSIHostByWWN = nodeDeviceLookupSCSIHostByWWN, /* 1.0.2 */ .deviceGetXMLDesc = nodeDeviceGetXMLDesc, /* 0.5.0 */ .deviceGetParent = nodeDeviceGetParent, /* 0.5.0 */ .deviceNumOfCaps = nodeDeviceNumOfCaps, /* 0.5.0 */ diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index a9b30b2..1be8526 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1754,6 +1754,7 @@ static virDeviceMonitor udevDeviceMonitor = { .listDevices = nodeListDevices, /* 0.7.3 */ .listAllNodeDevices = nodeListAllNodeDevices, /* 0.10.2 */ .deviceLookupByName = nodeDeviceLookupByName, /* 0.7.3 */ + .deviceLookupSCSIHostByWWN = nodeDeviceLookupSCSIHostByWWN, /* 1.0.2 */ .deviceGetXMLDesc = nodeDeviceGetXMLDesc, /* 0.7.3 */ .deviceGetParent = nodeDeviceGetParent, /* 0.7.3 */ .deviceNumOfCaps = nodeDeviceNumOfCaps, /* 0.7.3 */ -- 1.7.7.6

On Mon, Feb 04, 2013 at 09:03:11PM +0800, Osier Yang wrote:
This just simply changes nodeDeviceLookupByWWN to be not static, and its name into nodeDeviceLookupSCSIHostByWWN. And use that for udev and HAL backends. --- src/node_device/node_device_driver.c | 13 ++++++++----- src/node_device/node_device_driver.h | 4 ++++ src/node_device/node_device_hal.c | 1 + src/node_device/node_device_udev.c | 1 + 4 files changed, 14 insertions(+), 5 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Only nodedev-destroy and nodedev-dumpxml can benifit from the new API, other commands like nodedev-detach only works for PCI devices, WWN makes no sense for them. --- tools/virsh-nodedev.c | 98 ++++++++++++++++++++++++++++++++++++++----------- tools/virsh.pod | 15 ++++--- 2 files changed, 85 insertions(+), 28 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 11135ab..6603803 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -101,9 +101,14 @@ static const vshCmdInfo info_node_device_destroy[] = { static const vshCmdOptDef opts_node_device_destroy[] = { {.name = "name", + .type = VSH_OT_ALIAS, + .flags = 0, + .help = "device" + }, + {.name = "device", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, - .help = N_("name of the device to be destroyed") + .help = N_("device name or wwn pair in 'wwnn,wwpn' format") }, {.name = NULL} }; @@ -112,21 +117,47 @@ static bool cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) { virNodeDevicePtr dev = NULL; - bool ret = true; - const char *name = NULL; + bool ret = false; + const char *device_value = NULL; + char **arr = NULL; + int narr; - if (vshCommandOptString(cmd, "name", &name) <= 0) + if (vshCommandOptString(cmd, "device", &device_value) <= 0) return false; - dev = virNodeDeviceLookupByName(ctl->conn, name); + if (strchr(device_value, ',')) { + narr = vshStringToArray(device_value, &arr); + if (narr != 2) { + vshError(ctl, _("Malformed device value '%s'"), device_value); + goto cleanup; + } + + if (!virValidateWWN(arr[0]) || !virValidateWWN(arr[1])) + goto cleanup; + + dev = virNodeDeviceLookupSCSIHostByWWN(ctl->conn, arr[0], arr[1], 0); + } else { + dev = virNodeDeviceLookupByName(ctl->conn, device_value); + } + + if (!dev) { + vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); + goto cleanup; + } if (virNodeDeviceDestroy(dev) == 0) { - vshPrint(ctl, _("Destroyed node device '%s'\n"), name); + vshPrint(ctl, _("Destroyed node device '%s'\n"), device_value); } else { - vshError(ctl, _("Failed to destroy node device '%s'"), name); - ret = false; + vshError(ctl, _("Failed to destroy node device '%s'"), device_value); + goto cleanup; } + ret = true; +cleanup: + if (arr) { + VIR_FREE(*arr); + VIR_FREE(arr); + } virNodeDeviceFree(dev); return ret; } @@ -476,7 +507,7 @@ static const vshCmdOptDef opts_node_device_dumpxml[] = { {.name = "device", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, - .help = N_("device key") + .help = N_("device name or wwn pair in 'wwnn,wwpn' format"), }, {.name = NULL} }; @@ -484,27 +515,50 @@ static const vshCmdOptDef opts_node_device_dumpxml[] = { static bool cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd) { - const char *name = NULL; - virNodeDevicePtr device; - char *xml; + virNodeDevicePtr device = NULL; + char *xml = NULL; + const char *device_value = NULL; + char **arr = NULL; + int narr; + bool ret = false; + + if (vshCommandOptString(cmd, "device", &device_value) <= 0) + return false; + + if (strchr(device_value, ',')) { + narr = vshStringToArray(device_value, &arr); + if (narr != 2) { + vshError(ctl, _("Malformed device value '%s'"), device_value); + goto cleanup; + } - if (vshCommandOptString(cmd, "device", &name) <= 0) - return false; - if (!(device = virNodeDeviceLookupByName(ctl->conn, name))) { - vshError(ctl, "%s '%s'", _("Could not find matching device"), name); - return false; + if (!virValidateWWN(arr[0]) || !virValidateWWN(arr[1])) + goto cleanup; + + device = virNodeDeviceLookupSCSIHostByWWN(ctl->conn, arr[0], arr[1], 0); + } else { + device = virNodeDeviceLookupByName(ctl->conn, device_value); } - xml = virNodeDeviceGetXMLDesc(device, 0); - if (!xml) { - virNodeDeviceFree(device); - return false; + if (!device) { + vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); + goto cleanup; } + if (!(xml = virNodeDeviceGetXMLDesc(device, 0))) + goto cleanup; + vshPrint(ctl, "%s\n", xml); + + ret = true; +cleanup: + if (arr) { + VIR_FREE(*arr); + VIR_FREE(arr); + } VIR_FREE(xml); virNodeDeviceFree(device); - return true; + return ret; } /* diff --git a/tools/virsh.pod b/tools/virsh.pod index ec1772d..96666c4 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1953,11 +1953,12 @@ host nodes are available for use, but this allows registration of host hardware that libvirt did not automatically detect. I<file> contains xml for a top-level <device> description of a node device. -=item B<nodedev-destroy> I<nodedev> +=item B<nodedev-destroy> I<device> -Destroy (stop) a device on the host. Note that this makes libvirt -quit managing a host device, and may even make that device unusable -by the rest of the physical host until a reboot. +Destroy (stop) a device on the host. I<device> can be either device +name or wwn pair in "wwnn,wwpn" format (only works for HBA). Note +that this makes libvirt quit managing a host device, and may even make +that device unusable by the rest of the physical host until a reboot. =item B<nodedev-detach> I<nodedev> @@ -1967,12 +1968,14 @@ B<nodedev-reattach>, and is done automatically for managed devices. For compatibility purposes, this command can also be spelled B<nodedev-dettach>. -=item B<nodedev-dumpxml> I<nodedev> +=item B<nodedev-dumpxml> I<device> Dump a <device> XML representation for the given node device, including such information as the device name, which bus owns the device, the vendor and product id, and any capabilities of the device usable by -libvirt (such as whether device reset is supported). +libvirt (such as whether device reset is supported). I<device> can +be either device name or wwn pair in "wwnn,wwpn" format (only works +for HBA). =item B<nodedev-list> I<cap> I<--tree> -- 1.7.7.6

On 2013年02月04日 21:03, Osier Yang wrote:
Only nodedev-destroy and nodedev-dumpxml can benifit from the new API, other commands like nodedev-detach only works for PCI devices, WWN makes no sense for them. --- tools/virsh-nodedev.c | 98 ++++++++++++++++++++++++++++++++++++++----------- tools/virsh.pod | 15 ++++--- 2 files changed, 85 insertions(+), 28 deletions(-)
diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 11135ab..6603803 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -101,9 +101,14 @@ static const vshCmdInfo info_node_device_destroy[] = {
static const vshCmdOptDef opts_node_device_destroy[] = { {.name = "name", + .type = VSH_OT_ALIAS, + .flags = 0, + .help = "device" + }, + {.name = "device", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, - .help = N_("name of the device to be destroyed") + .help = N_("device name or wwn pair in 'wwnn,wwpn' format") }, {.name = NULL} }; @@ -112,21 +117,47 @@ static bool cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) { virNodeDevicePtr dev = NULL; - bool ret = true; - const char *name = NULL; + bool ret = false; + const char *device_value = NULL; + char **arr = NULL; + int narr;
- if (vshCommandOptString(cmd, "name",&name)<= 0) + if (vshCommandOptString(cmd, "device",&device_value)<= 0)
Things like this need to be changed as Peter's helper is pushed now. I'm going to post v2. Osier

Only nodedev-destroy and nodedev-dumpxml can benifit from the new API, other commands like nodedev-detach only works for PCI devices, WWN makes no sense for them. --- Rebased on Peter's virsh cleanup patches. --- tools/virsh-nodedev.c | 84 +++++++++++++++++++++++++++++++++++++++--------- tools/virsh.pod | 15 +++++--- 2 files changed, 77 insertions(+), 22 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index f85bded..7c51a56 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -101,9 +101,14 @@ static const vshCmdInfo info_node_device_destroy[] = { static const vshCmdOptDef opts_node_device_destroy[] = { {.name = "name", + .type = VSH_OT_ALIAS, + .flags = 0, + .help = "device" + }, + {.name = "device", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, - .help = N_("name of the device to be destroyed") + .help = N_("device name or wwn pair in 'wwnn,wwpn' format") }, {.name = NULL} }; @@ -112,21 +117,47 @@ static bool cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) { virNodeDevicePtr dev = NULL; - bool ret = true; - const char *name = NULL; + bool ret = false; + const char *device_value = NULL; + char **arr = NULL; + int narr; - if (vshCommandOptStringReq(ctl, cmd, "name", &name) < 0) + if (vshCommandOptStringReq(ctl, cmd, "device", &device_value) < 0) return false; - dev = virNodeDeviceLookupByName(ctl->conn, name); + if (strchr(device_value, ',')) { + narr = vshStringToArray(device_value, &arr); + if (narr != 2) { + vshError(ctl, _("Malformed device value '%s'"), device_value); + goto cleanup; + } + + if (!virValidateWWN(arr[0]) || !virValidateWWN(arr[1])) + goto cleanup; + + dev = virNodeDeviceLookupSCSIHostByWWN(ctl->conn, arr[0], arr[1], 0); + } else { + dev = virNodeDeviceLookupByName(ctl->conn, device_value); + } + + if (!dev) { + vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); + goto cleanup; + } if (virNodeDeviceDestroy(dev) == 0) { - vshPrint(ctl, _("Destroyed node device '%s'\n"), name); + vshPrint(ctl, _("Destroyed node device '%s'\n"), device_value); } else { - vshError(ctl, _("Failed to destroy node device '%s'"), name); - ret = false; + vshError(ctl, _("Failed to destroy node device '%s'"), device_value); + goto cleanup; } + ret = true; +cleanup: + if (arr) { + VIR_FREE(*arr); + VIR_FREE(arr); + } virNodeDeviceFree(dev); return ret; } @@ -476,7 +507,7 @@ static const vshCmdOptDef opts_node_device_dumpxml[] = { {.name = "device", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, - .help = N_("device key") + .help = N_("device name or wwn pair in 'wwnn,wwpn' format"), }, {.name = NULL} }; @@ -484,26 +515,47 @@ static const vshCmdOptDef opts_node_device_dumpxml[] = { static bool cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd) { - const char *name = NULL; virNodeDevicePtr device = NULL; char *xml = NULL; + const char *device_value = NULL; + char **arr = NULL; + int narr; bool ret = false; - if (vshCommandOptStringReq(ctl, cmd, "device", &name) < 0) - return false; + if (vshCommandOptStringReq(ctl, cmd, "device", &device_value) < 0) + return false; - if (!(device = virNodeDeviceLookupByName(ctl->conn, name))) { - vshError(ctl, _("Could not find matching device '%s'"), name); - return false; + if (strchr(device_value, ',')) { + narr = vshStringToArray(device_value, &arr); + if (narr != 2) { + vshError(ctl, _("Malformed device value '%s'"), device_value); + goto cleanup; + } + + if (!virValidateWWN(arr[0]) || !virValidateWWN(arr[1])) + goto cleanup; + + device = virNodeDeviceLookupSCSIHostByWWN(ctl->conn, arr[0], arr[1], 0); + } else { + device = virNodeDeviceLookupByName(ctl->conn, device_value); + } + + if (!device) { + vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); + goto cleanup; } if (!(xml = virNodeDeviceGetXMLDesc(device, 0))) goto cleanup; vshPrint(ctl, "%s\n", xml); - ret = true; + ret = true; cleanup: + if (arr) { + VIR_FREE(*arr); + VIR_FREE(arr); + } VIR_FREE(xml); virNodeDeviceFree(device); return ret; diff --git a/tools/virsh.pod b/tools/virsh.pod index ec1772d..96666c4 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1953,11 +1953,12 @@ host nodes are available for use, but this allows registration of host hardware that libvirt did not automatically detect. I<file> contains xml for a top-level <device> description of a node device. -=item B<nodedev-destroy> I<nodedev> +=item B<nodedev-destroy> I<device> -Destroy (stop) a device on the host. Note that this makes libvirt -quit managing a host device, and may even make that device unusable -by the rest of the physical host until a reboot. +Destroy (stop) a device on the host. I<device> can be either device +name or wwn pair in "wwnn,wwpn" format (only works for HBA). Note +that this makes libvirt quit managing a host device, and may even make +that device unusable by the rest of the physical host until a reboot. =item B<nodedev-detach> I<nodedev> @@ -1967,12 +1968,14 @@ B<nodedev-reattach>, and is done automatically for managed devices. For compatibility purposes, this command can also be spelled B<nodedev-dettach>. -=item B<nodedev-dumpxml> I<nodedev> +=item B<nodedev-dumpxml> I<device> Dump a <device> XML representation for the given node device, including such information as the device name, which bus owns the device, the vendor and product id, and any capabilities of the device usable by -libvirt (such as whether device reset is supported). +libvirt (such as whether device reset is supported). I<device> can +be either device name or wwn pair in "wwnn,wwpn" format (only works +for HBA). =item B<nodedev-list> I<cap> I<--tree> -- 1.7.7.6

On Mon, Feb 04, 2013 at 10:16:44PM +0800, Osier Yang wrote:
Only nodedev-destroy and nodedev-dumpxml can benifit from the new API, other commands like nodedev-detach only works for PCI devices, WWN makes no sense for them.
Is that really correct ? I thought we could use 'nodedev-detach' to delete an NPIV SCSI adapter, which would benefit from WWN wouldn't it ? Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 2013年02月07日 23:09, Daniel P. Berrange wrote:
On Mon, Feb 04, 2013 at 10:16:44PM +0800, Osier Yang wrote:
Only nodedev-destroy and nodedev-dumpxml can benifit from the new API, other commands like nodedev-detach only works for PCI devices, WWN makes no sense for them.
Is that really correct ? I thought we could use 'nodedev-detach' to delete an NPIV SCSI adapter, which would benefit from WWN wouldn't it ?
nodedev-destroy can be used to delete the NPIV scsi adapter. Do we also want to delete it using nodedev-detach? personally I think no need, it just can introduce confusion. Osier

On Mon, Feb 11, 2013 at 09:16:32PM +0800, Osier Yang wrote:
On 2013年02月07日 23:09, Daniel P. Berrange wrote:
On Mon, Feb 04, 2013 at 10:16:44PM +0800, Osier Yang wrote:
Only nodedev-destroy and nodedev-dumpxml can benifit from the new API, other commands like nodedev-detach only works for PCI devices, WWN makes no sense for them.
Is that really correct ? I thought we could use 'nodedev-detach' to delete an NPIV SCSI adapter, which would benefit from WWN wouldn't it ?
nodedev-destroy can be used to delete the NPIV scsi adapter. Do we also want to delete it using nodedev-detach? personally I think no need, it just can introduce confusion.
Oh yes, ignore my comment. I was mixing up the two commands Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 2013年02月11日 21:18, Daniel P. Berrange wrote:
On Mon, Feb 11, 2013 at 09:16:32PM +0800, Osier Yang wrote:
On 2013年02月07日 23:09, Daniel P. Berrange wrote:
On Mon, Feb 04, 2013 at 10:16:44PM +0800, Osier Yang wrote:
Only nodedev-destroy and nodedev-dumpxml can benifit from the new API, other commands like nodedev-detach only works for PCI devices, WWN makes no sense for them.
Is that really correct ? I thought we could use 'nodedev-detach' to delete an NPIV SCSI adapter, which would benefit from WWN wouldn't it ?
nodedev-destroy can be used to delete the NPIV scsi adapter. Do we also want to delete it using nodedev-detach? personally I think no need, it just can introduce confusion.
Oh yes, ignore my comment. I was mixing up the two commands
So, can you review this too? :-) Osier

On Mon, Feb 04, 2013 at 10:16:44PM +0800, Osier Yang wrote:
Only nodedev-destroy and nodedev-dumpxml can benifit from the new API, other commands like nodedev-detach only works for PCI devices, WWN makes no sense for them.
--- Rebased on Peter's virsh cleanup patches. --- tools/virsh-nodedev.c | 84 +++++++++++++++++++++++++++++++++++++++--------- tools/virsh.pod | 15 +++++--- 2 files changed, 77 insertions(+), 22 deletions(-)
diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index f85bded..7c51a56 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -101,9 +101,14 @@ static const vshCmdInfo info_node_device_destroy[] = {
static const vshCmdOptDef opts_node_device_destroy[] = { {.name = "name", + .type = VSH_OT_ALIAS, + .flags = 0, + .help = "device" + }, + {.name = "device", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, - .help = N_("name of the device to be destroyed") + .help = N_("device name or wwn pair in 'wwnn,wwpn' format") }, {.name = NULL} }; @@ -112,21 +117,47 @@ static bool cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) { virNodeDevicePtr dev = NULL; - bool ret = true; - const char *name = NULL; + bool ret = false; + const char *device_value = NULL; + char **arr = NULL; + int narr;
- if (vshCommandOptStringReq(ctl, cmd, "name", &name) < 0) + if (vshCommandOptStringReq(ctl, cmd, "device", &device_value) < 0) return false;
- dev = virNodeDeviceLookupByName(ctl->conn, name); + if (strchr(device_value, ',')) { + narr = vshStringToArray(device_value, &arr); + if (narr != 2) { + vshError(ctl, _("Malformed device value '%s'"), device_value); + goto cleanup; + } + + if (!virValidateWWN(arr[0]) || !virValidateWWN(arr[1])) + goto cleanup; + + dev = virNodeDeviceLookupSCSIHostByWWN(ctl->conn, arr[0], arr[1], 0); + } else { + dev = virNodeDeviceLookupByName(ctl->conn, device_value); + } + + if (!dev) { + vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); + goto cleanup; + }
if (virNodeDeviceDestroy(dev) == 0) { - vshPrint(ctl, _("Destroyed node device '%s'\n"), name); + vshPrint(ctl, _("Destroyed node device '%s'\n"), device_value); } else { - vshError(ctl, _("Failed to destroy node device '%s'"), name); - ret = false; + vshError(ctl, _("Failed to destroy node device '%s'"), device_value); + goto cleanup; }
+ ret = true; +cleanup: + if (arr) { + VIR_FREE(*arr);
Hmm, IIUC, that is equiv to VIR_FREE(arr[0]), so what is free'ing arr[1] ?
+ VIR_FREE(arr); + }
ACK if that leak is fixed, or otherwise explained. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 2013年02月11日 21:33, Daniel P. Berrange wrote:
On Mon, Feb 04, 2013 at 10:16:44PM +0800, Osier Yang wrote:
Only nodedev-destroy and nodedev-dumpxml can benifit from the new API, other commands like nodedev-detach only works for PCI devices, WWN makes no sense for them.
--- Rebased on Peter's virsh cleanup patches. --- tools/virsh-nodedev.c | 84 +++++++++++++++++++++++++++++++++++++++--------- tools/virsh.pod | 15 +++++--- 2 files changed, 77 insertions(+), 22 deletions(-)
diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index f85bded..7c51a56 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -101,9 +101,14 @@ static const vshCmdInfo info_node_device_destroy[] = {
static const vshCmdOptDef opts_node_device_destroy[] = { {.name = "name", + .type = VSH_OT_ALIAS, + .flags = 0, + .help = "device" + }, + {.name = "device", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, - .help = N_("name of the device to be destroyed") + .help = N_("device name or wwn pair in 'wwnn,wwpn' format") }, {.name = NULL} }; @@ -112,21 +117,47 @@ static bool cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) { virNodeDevicePtr dev = NULL; - bool ret = true; - const char *name = NULL; + bool ret = false; + const char *device_value = NULL; + char **arr = NULL; + int narr;
- if (vshCommandOptStringReq(ctl, cmd, "name",&name)< 0) + if (vshCommandOptStringReq(ctl, cmd, "device",&device_value)< 0) return false;
- dev = virNodeDeviceLookupByName(ctl->conn, name); + if (strchr(device_value, ',')) { + narr = vshStringToArray(device_value,&arr); + if (narr != 2) { + vshError(ctl, _("Malformed device value '%s'"), device_value); + goto cleanup; + } + + if (!virValidateWWN(arr[0]) || !virValidateWWN(arr[1])) + goto cleanup; + + dev = virNodeDeviceLookupSCSIHostByWWN(ctl->conn, arr[0], arr[1], 0); + } else { + dev = virNodeDeviceLookupByName(ctl->conn, device_value); + } + + if (!dev) { + vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); + goto cleanup; + }
if (virNodeDeviceDestroy(dev) == 0) { - vshPrint(ctl, _("Destroyed node device '%s'\n"), name); + vshPrint(ctl, _("Destroyed node device '%s'\n"), device_value); } else { - vshError(ctl, _("Failed to destroy node device '%s'"), name); - ret = false; + vshError(ctl, _("Failed to destroy node device '%s'"), device_value); + goto cleanup; }
+ ret = true; +cleanup: + if (arr) { + VIR_FREE(*arr);
Hmm, IIUC, that is equiv to VIR_FREE(arr[0]), so what is free'ing arr[1] ?
vshStringToArray substitutes the delimiter ',' with '\0', and the elements simply point to the pieces. So freeing the first element frees all the memory of the array elements.
+ VIR_FREE(arr); + }
ACK if that leak is fixed, or otherwise explained.
Thanks for the reviewing, I pushed this set. Osier

On 2013年02月11日 22:51, Osier Yang wrote:
On 2013年02月11日 21:33, Daniel P. Berrange wrote:
On Mon, Feb 04, 2013 at 10:16:44PM +0800, Osier Yang wrote:
Only nodedev-destroy and nodedev-dumpxml can benifit from the new API, other commands like nodedev-detach only works for PCI devices, WWN makes no sense for them.
--- Rebased on Peter's virsh cleanup patches. --- tools/virsh-nodedev.c | 84 +++++++++++++++++++++++++++++++++++++++--------- tools/virsh.pod | 15 +++++--- 2 files changed, 77 insertions(+), 22 deletions(-)
diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index f85bded..7c51a56 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -101,9 +101,14 @@ static const vshCmdInfo info_node_device_destroy[] = {
static const vshCmdOptDef opts_node_device_destroy[] = { {.name = "name", + .type = VSH_OT_ALIAS, + .flags = 0, + .help = "device" + }, + {.name = "device", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, - .help = N_("name of the device to be destroyed") + .help = N_("device name or wwn pair in 'wwnn,wwpn' format") }, {.name = NULL} }; @@ -112,21 +117,47 @@ static bool cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) { virNodeDevicePtr dev = NULL; - bool ret = true; - const char *name = NULL; + bool ret = false; + const char *device_value = NULL; + char **arr = NULL; + int narr;
- if (vshCommandOptStringReq(ctl, cmd, "name",&name)< 0) + if (vshCommandOptStringReq(ctl, cmd, "device",&device_value)< 0) return false;
- dev = virNodeDeviceLookupByName(ctl->conn, name); + if (strchr(device_value, ',')) { + narr = vshStringToArray(device_value,&arr); + if (narr != 2) { + vshError(ctl, _("Malformed device value '%s'"), device_value); + goto cleanup; + } + + if (!virValidateWWN(arr[0]) || !virValidateWWN(arr[1])) + goto cleanup; + + dev = virNodeDeviceLookupSCSIHostByWWN(ctl->conn, arr[0], arr[1], 0); + } else { + dev = virNodeDeviceLookupByName(ctl->conn, device_value); + } + + if (!dev) { + vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); + goto cleanup; + }
if (virNodeDeviceDestroy(dev) == 0) { - vshPrint(ctl, _("Destroyed node device '%s'\n"), name); + vshPrint(ctl, _("Destroyed node device '%s'\n"), device_value); } else { - vshError(ctl, _("Failed to destroy node device '%s'"), name); - ret = false; + vshError(ctl, _("Failed to destroy node device '%s'"), device_value); + goto cleanup; }
+ ret = true; +cleanup: + if (arr) { + VIR_FREE(*arr);
Hmm, IIUC, that is equiv to VIR_FREE(arr[0]), so what is free'ing arr[1] ?
vshStringToArray substitutes the delimiter ',' with '\0', and the elements simply point to the pieces. So freeing the first element frees all the memory of the array elements.
Btw, I think it's time to destroy the use of vshStringToArray , and use the more general virStringSplit now, (which not only supports the delimiter ','). It will be later patch.
+ VIR_FREE(arr); + }
ACK if that leak is fixed, or otherwise explained.
Thanks for the reviewing, I pushed this set.
Osier
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 02/04/2013 08:03 AM, Osier Yang wrote:
v3 - v4: * Only rebasing
v2 - v3: * Validate the specified wwnn,wwpn pair before applying it to the new API in virsh-nodedev.c
v1 - v2: * Per Daniel's suggestion, change the API name from virNodeDeviceLookupByWWN into virNodeDeviceLookupSCSIHostByWWN.
Osier Yang (4): Introduce API virNodeDeviceLookupSCSIHostByWWN remote: Wire up the remote protocol nodedev: Implement virNodeDeviceLookupSCSIHostByWWN virsh: Use virNodeDeviceLookupSCSIHostByWWN
include/libvirt/libvirt.h.in | 5 ++ src/driver.h | 6 ++ src/libvirt.c | 46 ++++++++++++++++ src/libvirt_public.syms | 5 ++ src/node_device/node_device_driver.c | 13 +++-- src/node_device/node_device_driver.h | 4 ++ src/node_device/node_device_hal.c | 1 + src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 13 ++++- src/remote_protocol-structs | 9 +++ src/rpc/gendispatch.pl | 5 ++- tools/virsh-nodedev.c | 98 ++++++++++++++++++++++++++-------- tools/virsh.pod | 15 +++-- 14 files changed, 187 insertions(+), 35 deletions(-)
I didn't see anything jump out at me with these changes. I don't remember looking at v1->v3 and I'm still learning the ins and outs of the code. John
participants (3)
-
Daniel P. Berrange
-
John Ferlan
-
Osier Yang