On Fri, Jun 28, 2013 at 12:28:25PM +0200, Ján Tomko wrote:
> On 06/28/2013 11:39 AM, Daniel P. Berrange wrote:
> > On Fri, Jun 28, 2013 at 05:30:14PM +0800, Dennis Chen wrote:
> >> On 06/28/2013 04:39 PM, Ján Tomko wrote:
> >>> On 06/28/2013 03:22 AM, Dennis Chen wrote:
> >>>> When create a virtual FC HBA with virsh/libvirt API, an error message will be
> >>>> returned:"error: Node device not found",
> >>>> also the 'nodedev-dumpxml' shows wrong information of wwpn & wwnn for the new
> >>>> created device.
> >>>>
> >>>> Signed-off-by:xschen@xxxxxxxxxxxxx
> >>>> ---
> >>>> src/util/virutil.c | 4 ++--
> >>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/src/util/virutil.c b/src/util/virutil.c
> >>>> index 6fa0212e..569d035 100644
> >>>> --- a/src/util/virutil.c
> >>>> +++ b/src/util/virutil.c
> >>>> @@ -1792,8 +1792,8 @@ virManageVport(const int parent_host,
> >>>> if (virAsprintf(&vport_name,
> >>>> "%s:%s",
> >>>> - wwnn,
> >>>> - wwpn) < 0) {
> >>>> + wwpn,
> >>>> + wwnn) < 0) {
> >>>> virReportOOMError();
> >>>> goto cleanup;
> >>>> }
> >>>>
> >>> Hmm, this is what we've had before commit f90af69 [1]
> >>> but according to scsi_fc_transport.txt [2] in kernel docs, it should be
> >>> <WWPN>:<WWNN>. I wonder what that commit was trying to fix.
> >>>
> >>> Jan
> >>>
> >>> [1] http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=f90af69
> >>> [2] https://www.kernel.org/doc/Documentation/scsi/scsi_fc_transport.txt
> >>>
> >> Interesting! According to my testing result (kernel version 2.6.32),
> >> kernel docs is correct,it should be <WWPN>:<WWNN>. It's causing me
> >> trouble when creating the device with virsh after that commit...
> >
> > I say ACK to your patch. Your patch matches the kernel documentation
> > and you've confirmed that it fixes a clear bug. The original patch
> > of Osiers has zero information about what it was fixing, so there's
> > little reason to assume it was a correct change based on your feedback.
>
> I found it: it was being called with the parameters swapped in the scsi
> storage backend.
Ah that makes sense now. The node device code was calling it the
right way, so Osier's patch broke the node device code, but fixed
his storage code which was buggy.
>
> Pushing with this squashed in:
>
> diff --git a/src/storage/storage_backend_scsi.c
> b/src/storage/storage_backend_scsi.c
> index 285c5cb..3deceda 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -659,8 +659,8 @@ createVport(virStoragePoolSourceAdapter adapter)
> if (getHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
> return -1;
>
> - if (virManageVport(parent_host, adapter.data.fchost.wwnn,
> - adapter.data.fchost.wwpn, VPORT_CREATE) < 0)
> + if (virManageVport(parent_host, adapter.data.fchost.wwpn,
> + adapter.data.fchost.wwnn, VPORT_CREATE) < 0)
> return -1;
>
> virFileWaitForDevices();
> @@ -682,8 +682,8 @@ deleteVport(virStoragePoolSourceAdapter adapter)
> if (getHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
> return -1;
>
> - if (virManageVport(parent_host, adapter.data.fchost.wwnn,
> - adapter.data.fchost.wwpn, VPORT_DELETE) < 0)
> + if (virManageVport(parent_host, adapter.data.fchost.wwpn,
> + adapter.data.fchost.wwnn, VPORT_DELETE) < 0)
> return -1;
>
> return 0;
>
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list