[libvirt] [REPOST PATCH 1/6] storage: Refactor and fix virStorageBackendCreateExecCommand

[Ján Tomko]

On Wed, Oct 28, 2015 at 07:54:48AM -0400, John Ferlan wrote:
> Refactor the code to handle the NETFS pool separately from other pool
> types. When the original code was developed (commit id 'e1f27784')
> the virCommandSetUmask did not exist (commit id '0e1a1a8c4'), so there
> was no way to set the permissions for the creation. Now that it exists,
> we can use it. The code currently fails to create a volume in a NETFS
> pool from some other volume within the pool because the chmod done after
> file creation fails.
> 

Correctly - we should fail if we cannot create the file with the
requested permissions. Or is the problem with chmod failing even though
the file already has the right mode? Can't we just skip the chmod when
the modes match, or -1 was requested, as we do for uid/gid?

> So adjust the code to jump to cleanup once the file is successfully
> created in the NETFS pool. If it fails or for non NETFS files, just
> perform the create, chown, and chmod in order
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/storage/storage_backend.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index a375fe0..037d6d7 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -678,7 +678,6 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
>      gid_t gid;
>      uid_t uid;
>      mode_t mode;
> -    bool filecreated = false;
>      int ret = -1;
>  
>      if ((pool->def->type == VIR_STORAGE_POOL_NETFS)
> @@ -690,26 +689,29 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
>  
>          virCommandSetUID(cmd, vol->target.perms->uid);
>          virCommandSetGID(cmd, vol->target.perms->gid);
> +        mode = (vol->target.perms->mode == (mode_t) -1 ?
> +            VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode);

This assignment is now done twice in this function.

> +        virCommandSetUmask(cmd, 0777 - mode);
>  

Dealing with bits, I'd rather use xor.

> -        if (virCommandRun(cmd, NULL) == 0) {
> -            /* command was successfully run, check if the file was created */
> -            if (stat(vol->target.path, &st) >= 0)
> -                filecreated = true;
> +        if ((ret = virCommandRun(cmd, NULL)) == 0) {
> +            /* command was successfully run, if the file was created
> +             * then we are done */

We should not ignore the explicitly requested uid/gid/mode just because
we're in a NETFS pool.

Also, this overwrites 'ret' from the default -1, without jumping to
cleanup in all cases.

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151103/d2ed3687/attachment-0001.sig>