[libvirt] PATCH: Don't stop storage pools on daemon shutdown

[Chris Lalancette]

Daniel P. Berrange wrote:
> Tearing a guest's storage out from under its feet on libvirtd shutdown
> is just as bad as tearing out its network :-) This patch removes the
> code which shuts down storage pool when the daemon shuts down. So NFS
> mounts stay around, LVM VGs remain active, and iSCSI connections remain
> logged in. When we then start up again, we happily detect that these
> resources are already running, and mark the pool as such

Yes, this will fix a couple of bugs ovirt is having with iscsi as well, so this
is a good change.  In some basic testing here, it works like a charm, which is
great.  I do have a question though...

> 
> Daniel
> 
> diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c
> --- a/src/storage_backend_iscsi.c
> +++ b/src/storage_backend_iscsi.c
> @@ -573,6 +573,7 @@ virStorageBackendISCSIStartPool(virConne
>                                  virStoragePoolObjPtr pool)
>  {
>      char *portal = NULL;
> +    char *session;
>  
>      if (pool->def->source.host.name == NULL) {
>          virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
> @@ -587,13 +588,17 @@ virStorageBackendISCSIStartPool(virConne
>          return -1;
>      }
>  
> -    if ((portal = virStorageBackendISCSIPortal(conn, pool)) == NULL)
> -        return -1;
> -    if (virStorageBackendISCSILogin(conn, pool, portal) < 0) {
> +    if ((session = virStorageBackendISCSISession(conn, pool)) == NULL) {

OK, so here, we look up the session, and if it is NULL, this is a new pool that
iscsid doesn't know about, and we need to start it.  If we do not return NULL,
then this is an existing session.  That logic is fine, but my question has to do
with virStorageBackendISCSISession() itself.  If it detects a NULL session, it
does a "virStorageReportError(INTERNAL_ERROR)".  Now, the user never sees that
because we don't propagate an error code, but we have now set the internal error
code to that.  Is that going to be a problem?  Should we just remove the
virStorageReportError in that case?

-- 
Chris Lalancette