[libvirt] [PATCH 1/5] storage: split virStorageBackendCreateExecCommand in two functions
John Ferlan
jferlan at redhat.com
Tue Dec 8 15:31:16 UTC 2015
On 12/08/2015 09:11 AM, Dmitry Andreev wrote:
> This patch introduces virStorageBackendCreateExec function
> that has simple type arguments so it can be used without
> 'virStorageXXX' structures.
>
> ---
> src/storage/storage_backend.c | 130 +++++++++++++++++++++++-------------------
> 1 file changed, 70 insertions(+), 60 deletions(-)
>
caveat... only looked at this patch of the series... Mostly because
I've recently had the (dis)pleasure of working in this code. There's
also a backlog of other changes that I want to look go through first
prior to the rest of this series. Still seeing this particular code
change just caused my "warning, danger, radar" to go off...
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 194736b..2e14af9 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -675,92 +675,89 @@ virStorageGenerateQcowEncryption(virConnectPtr conn,
> return ret;
> }
>
> -static int
> -virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
> - virStorageVolDefPtr vol,
> - virCommandPtr cmd)
> +static
> +int virStorageBackendCreateExec(virCommandPtr cmd, const char* path,
> + uid_t uid, gid_t gid, mode_t mode)
The coding practices are one argument per line... Generating a comment
regarding input arguments might have been nice too.
Also, if your goal was to remove the "pool pointer", then why not pass
the pool->def->type as well and thus avoid the need to remove/rework the
if loop below?
> {
> struct stat st;
> - gid_t gid;
> - uid_t uid;
> - mode_t mode = (vol->target.perms->mode == (mode_t) -1 ?
> - VIR_STORAGE_DEFAULT_VOL_PERM_MODE :
> - vol->target.perms->mode);
> bool filecreated = false;
> int ret = -1;
>
> - if ((pool->def->type == VIR_STORAGE_POOL_NETFS)
> - && (((geteuid() == 0)
> - && (vol->target.perms->uid != (uid_t) -1)
> - && (vol->target.perms->uid != 0))
> - || ((vol->target.perms->gid != (gid_t) -1)
> - && (vol->target.perms->gid != getegid())))) {
> -
> - virCommandSetUID(cmd, vol->target.perms->uid);
> - virCommandSetGID(cmd, vol->target.perms->gid);
> - virCommandSetUmask(cmd, S_IRWXUGO ^ mode);
> -
> - if (virCommandRun(cmd, NULL) == 0) {
> - /* command was successfully run, check if the file was created */
> - if (stat(vol->target.path, &st) >= 0) {
> - filecreated = true;
> -
> - /* seems qemu-img disregards umask and open/creates using 0644.
> - * If that doesn't match what we expect, then let's try to
> - * re-open the file and attempt to force the mode change.
> - */
> - if (mode != (st.st_mode & S_IRWXUGO)) {
> - int fd = -1;
> - int flags = VIR_FILE_OPEN_FORK | VIR_FILE_OPEN_FORCE_MODE;
> -
> - if ((fd = virFileOpenAs(vol->target.path, O_RDWR, mode,
> - vol->target.perms->uid,
> - vol->target.perms->gid,
> - flags)) >= 0) {
> - /* Success - means we're good */
> - VIR_FORCE_CLOSE(fd);
> - ret = 0;
> - goto cleanup;
> - }
> - }
> - }
> + /* check is there a need to set uid/gid */
> + if ((uid == (uid_t) -1 || geteuid() == uid)
> + && (gid == (gid_t) -1 || geteuid() == gid))
> + goto retry;
Not the label retry really conveys the right meaning. To me it's more of
a "skip_netfs_when_running_as_root".
It seems there is a slight difference to the original code which checked
geteuid() == 0 first, then "uid != 0" (where uid would have been the
passed vol->target.perms->uid).
So the next question is - have you considered whether this is a non
privileged connection? e.g. qemu:///system vs. qemu:///session. The
geteuid() may not be 0, but the NETFS check was ensuring that if going
through that path, not only was it a NETFS storage pool target, but that
we were running as root and the passed uid was set and it wasn't 0. This
all because of the NFS root_squash environment.
As an aside, generally the order for geteuid/getegid checks is swapped -
eg uid == geteuid() and gid == getegid()
> +
> + /* first try - create with uid/gid/umask
> + * Originaly this try was added for NETFS
> + * that doesn't support chmod.
> + */
Not sure I agree with the comment. NETFS is a "special case" especially
when the target NFS Server has root_squash enabled. I would suggest you
check the very recent history on this code... I'll claim short term
memory lapse (or purge out of unnecessary pages in memory) if you ask
too many questions though!
It's not that NETFS doesn't support chmod, it's written this way because
it's possible that a storage volume being created using kemu-img,
qmeu-img, or qcow-create is found on a NFS Server with root_squash
enabled. That means when the command runs, it has to run under the
specified uid/gid; otherwise, the file gets created with (I think the
default of) nfsnobody because NFS takes an incoming root connection and
"squashes" it. As for the 'mode' - as I found it in my recent foray into
the code... If that's provided, then a 'umask' is done during the
command run which "theoretically" would set the right protection bits.
However, as I learned qemu-img does it's own thing thus the odd comment.
With your/this change every pool type target could try the setting of
uid, gid, and mode in this manner as long as they've been provided in
the volume.xml. While perhaps technically not different than providing
uid, gid, and mode, then creating the file, then changing uid, gid, and
mode afterwards.
So that's input - I'm not sure doing the reworked/retry logic is
necessary and I'd certainly caution against it. You can also look at
virFileOpenAs, virFileRemove, and virDirCreate to see other code has the
(dis)pleasure of dealing with this NFS root_squash environment.
John
> + virCommandSetUID(cmd, uid);
> + virCommandSetGID(cmd, gid);
> + virCommandSetUmask(cmd, S_IRWXUGO ^ mode);
> +
> + if (virCommandRun(cmd, NULL) < 0)
> + goto retry;
> +
> + if (stat(path, &st) < 0)
> + goto retry;
> +
> + filecreated = true;
> +
> + /* seems qemu-img disregards umask and open/creates using 0644.
> + * If that doesn't match what we expect, then let's try to
> + * re-open the file and attempt to force the mode change.
> + */
> + if (mode != (st.st_mode & S_IRWXUGO)) {
> + int fd = -1;
> + int flags = VIR_FILE_OPEN_FORK | VIR_FILE_OPEN_FORCE_MODE;
> +
> + if ((fd = virFileOpenAs(path, O_RDWR, mode,
> + uid, gid, flags)) >= 0) {
> + /* Success - means we're good */
> + VIR_FORCE_CLOSE(fd);
> + ret = 0;
> + virReportSystemError(errno, _("no need to retry %s"), path);
> + goto cleanup;
> }
> }
>
> + retry:
> + /* second try - set uid/gid/umask after creation */
> if (!filecreated) {
> - /* don't change uid/gid/mode if we retry */
> virCommandSetUID(cmd, -1);
> virCommandSetGID(cmd, -1);
> virCommandSetUmask(cmd, 0);
>
> if (virCommandRun(cmd, NULL) < 0)
> goto cleanup;
> - if (stat(vol->target.path, &st) < 0) {
> +
> + if (stat(path, &st) < 0) {
> virReportSystemError(errno,
> - _("failed to create %s"), vol->target.path);
> + _("failed to create %s"), path);
> goto cleanup;
> }
> +
> filecreated = true;
> }
>
> - uid = (vol->target.perms->uid != st.st_uid) ? vol->target.perms->uid
> - : (uid_t) -1;
> - gid = (vol->target.perms->gid != st.st_gid) ? vol->target.perms->gid
> - : (gid_t) -1;
> + uid = (uid != st.st_uid) ? uid : (uid_t) -1;
> + gid = (gid != st.st_gid) ? gid : (gid_t) -1;
> +
> if (((uid != (uid_t) -1) || (gid != (gid_t) -1))
> - && (chown(vol->target.path, uid, gid) < 0)) {
> + && (chown(path, uid, gid) < 0)) {
> virReportSystemError(errno,
> _("cannot chown %s to (%u, %u)"),
> - vol->target.path, (unsigned int) uid,
> + path, (unsigned int) uid,
> (unsigned int) gid);
> goto cleanup;
> }
>
> if (mode != (st.st_mode & S_IRWXUGO) &&
> - chmod(vol->target.path, mode) < 0) {
> + chmod(path, mode) < 0) {
> virReportSystemError(errno,
> _("cannot set mode of '%s' to %04o"),
> - vol->target.path, mode);
> + path, mode);
> goto cleanup;
> }
>
> @@ -768,11 +765,24 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
>
> cleanup:
> if (ret < 0 && filecreated)
> - virFileRemove(vol->target.path, vol->target.perms->uid,
> - vol->target.perms->gid);
> + virFileRemove(path, uid, gid);
> return ret;
> }
>
> +static int
> +virStorageBackendCreateExecCommand(virStorageVolDefPtr vol,
> + virCommandPtr cmd)
> +{
> + mode_t mode = (vol->target.perms->mode == (mode_t) -1 ?
> + VIR_STORAGE_DEFAULT_VOL_PERM_MODE :
> + vol->target.perms->mode);
> +
> + return virStorageBackendCreateExec(cmd, vol->target.path,
> + vol->target.perms->uid,
> + vol->target.perms->gid,
> + mode);
> +}
> +
> enum {
> QEMU_IMG_BACKING_FORMAT_NONE = 0,
> QEMU_IMG_BACKING_FORMAT_FLAG,
> @@ -1153,7 +1163,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
> if (!cmd)
> goto cleanup;
>
> - ret = virStorageBackendCreateExecCommand(pool, vol, cmd);
> + ret = virStorageBackendCreateExecCommand(vol, cmd);
>
> virCommandFree(cmd);
> cleanup:
> @@ -1167,7 +1177,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
> */
> static int
> virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED,
> - virStoragePoolObjPtr pool,
> + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> virStorageVolDefPtr vol,
> virStorageVolDefPtr inputvol,
> unsigned int flags)
> @@ -1217,7 +1227,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED,
>
> cmd = virCommandNewArgList("qcow-create", size, vol->target.path, NULL);
>
> - ret = virStorageBackendCreateExecCommand(pool, vol, cmd);
> + ret = virStorageBackendCreateExecCommand(vol, cmd);
> virCommandFree(cmd);
> VIR_FREE(size);
>
>
More information about the libvir-list
mailing list