[libvirt] [PATCH 1/2] storage: Fix autostart of pool with "fc_host" type adapter

[Osier Yang]

On 22/01/14 21:36, John Ferlan wrote:
>
> On 01/07/2014 06:07 AM, Osier Yang wrote:
>> On 07/01/14 01:21, John Ferlan wrote:
>>> On 01/06/2014 05:19 AM, Osier Yang wrote:
>>>> The "checkPool" is a bit different for pool with "fc_host"
>>>> type source adapter, since the vHBA it's based on might be
>>>> not created yet (it's created by "startPool", which is
>>>> involked after "checkPool" in storageDriverAutostart). So it
>>>> should not fail, otherwise the "autostart" of the pool will
>>>> fail either.
>>>>
>>>> The problem is easy to reproduce:
>>>>       * Enable "autostart" for the pool
>>>>       * Restart libvirtd service
>>>>       * Check the pool's state
>>>> ---
>>>>    src/storage/storage_backend_scsi.c | 14 ++++++++++++--
>>>>    1 file changed, 12 insertions(+), 2 deletions(-)
>>>>
>>> Not sure this is the right thing to do. With this change it doesn't
>>> matter what getAdapterName() returns for fc_host's...
>> It matters with if "*isActive" is true or false in the end. We don't
>> need to try to start the pool if it's already active.
>>
> Fair enough; however, let's consider the failure case.  On failure, a
> message is reported and name == NULL.  Do we need to clear that error now?

Indeed.

>
>
>
> I've attached a format-patch output for this logic - your call as to
> whether or not you want to use it...  If you keep your logic, then just
> decide upon how to handle the error message... It won't be necessary for
> the check case, but would be for the refresh case.
>
> I am OK with things as they are, it just looks odd in the CheckPool
> function to be special casing FC_HOST just because it's not created yet
> and then to have the start function do the create anyway.  It just seems
> we could be smarter/better.
>

I agree your method is more linear, except there is no need for the error
statement in startPool (hope the indention is not broken), it will just
override the useful errors in the code path.

diff --git a/src/storage/storage_backend_scsi.c 
b/src/storage/storage_backend_scsi.c
index d3d14ce..f6cb820 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -744,13 +744,8 @@ virStorageBackendSCSIStartPool(virConnectPtr conn 
ATTRIBUTE_UNUSED,
  {
      char *name = NULL;
      virStoragePoolSourceAdapter adapter = pool->def->source.adapter;
-    if (!(name = getAdapterName(pool->def->source.adapter))) {
-        virReportError(VIR_ERR_XML_ERROR,
-                       _("Failed to find SCSI host with wwnn='%s', "
-                         "wwpn='%s'"), adapter.data.fchost.wwnn,
-                       adapter.data.fchost.wwpn);
+    if (!(name = getAdapterName(pool->def->source.adapter)))
          return -1;
-    }
      VIR_FREE(name);
      virFileWaitForDevices();
      return 0;

To ensure things work well, I'm going to put the patch into practice 
tomorrow
on a NPIV machine. If it goes well, I will push it with above change.

Osier