On 12/19/2016 10:57 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/security/security_dac.c | 193 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 193 insertions(+)
>
Might have been nice to add some basic/sparse documentation over return
values...
BTW: I'm stopping here since I have some functionality concerns that
would probably be replicated in _selinux and the qemu implementation.
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index b6f75fe1d..0208c1751 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -68,6 +68,117 @@ struct _virSecurityDACCallbackData {
> virSecurityLabelDefPtr secdef;
> };
>
> +typedef struct _virSecurityDACChownItem virSecurityDACChownItem;
> +typedef virSecurityDACChownItem *virSecurityDACChownItemPtr;
> +struct _virSecurityDACChownItem {
> + const char *path;
> + const virStorageSource *src;
> + uid_t uid;
> + gid_t gid;
> +};
> +
> +typedef struct _virSecurityDACChownList virSecurityDACChownList;
> +typedef virSecurityDACChownList *virSecurityDACChownListPtr;
> +struct _virSecurityDACChownList {
> + virSecurityDACDataPtr priv;
> + virSecurityDACChownItemPtr *items;
> + size_t nItems;
> +};
> +
> +
> +virThreadLocal chownList;
> +
> +static int
> +virSecurityDACChownListAppend(virSecurityDACChownListPtr list,
> + const char *path,
> + const virStorageSource *src,
> + uid_t uid,
> + gid_t gid)
> +{
> + virSecurityDACChownItemPtr item;
> +
> + if (VIR_ALLOC(item) < 0)
> + return -1;
> +
> + item->path = path;
> + item->src = src;
> + item->uid = uid;
> + item->gid = gid;
> +
> + if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) {
> + VIR_FREE(item);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static void
> +virSecurityDACChownListFree(void *opaque)
> +{
> + virSecurityDACChownListPtr list = opaque;
> + size_t i;
> +
> + if (!list)
> + return;
> +
> + for (i = 0; i < list->nItems; i++)
> + VIR_FREE(list->items[i]);
> + VIR_FREE(list);
> +}
> +
> +
This is one example where documenting return values would help... Since
whether 'list' is NULL or not is important from the callers viewpoint.
I also note the caller will do something different when ret > 0, but I
see no way for that to happen...
> +static int
> +virSecurityDACTransactionAppend(const char *path,
> + const virStorageSource *src,
> + uid_t uid,
> + gid_t gid)
> +{
> + virSecurityDACChownListPtr list;
> + int ret = -1;
> +
> + list = virThreadLocalGet(&chownList);
> + if (!list)
> + return 0;
> +
> + if (virSecurityDACChownListAppend(list, path, src, uid, gid) < 0)
> + goto cleanup;
> +
> + ret = 0;
Should this be ret = 1 so the caller "knows" something was put on list?
and thus returns 0 rather than running the rest of the code.
> + cleanup:
> + return ret;
> +}
> +
> +
> +static int virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv,
> + const virStorageSource *src,
> + const char *path,
> + uid_t uid,
> + gid_t gid);
> +
Some comments here would be nice.
> +static int
> +virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
> + void *opaque)
> +{
> + virSecurityDACChownListPtr list = opaque;
> + size_t i;
> +
> + ignore_value(virThreadLocalSet(&chownList, NULL));
Not that it should fail, but part of me wonders if a VIR_DEBUG should
issued...
This point in the process is the key step in the whole process - it's
from this point on that we do the "real" work.
Still after a bit of reading on and thinking - should this clearing be
done in Commit instead prior to calling Run
> + for (i = 0; i < list->nItems; i++) {
> + virSecurityDACChownItemPtr item = list->items[i];
> +
> + if (virSecurityDACSetOwnershipInternal(list->priv,
> + item->src,
> + item->path,
> + item->uid,
> + item->gid) < 0)
> + return -1;
If we have a failure, then should we rollback everything done to this
point and of course how would we accomplish that?
The problem being list is available only here and in Commit so how do we
really know how much has been *really* committed and of course does that
matter if our caller can "rollback" for us.
If I read things right, the caller would generate the same "list" in
rollback fashion and rollback everything rather than perhaps being
smarter and rolling back only what was changed. Although I suppose
rolling back everything would/could cause failure at the same point as
Commit.
Still we have no real way of restoring/rolling back from the point of
failure unless commit and/or this function returns that mechanism. It
would seem the rollback of RestoreAll labels would/could be overkill.
> + }
> +
> + return 0;
> +}
> +
> +
> /* returns -1 on error, 0 on success */
> int
> virSecurityDACSetUserAndGroup(virSecurityManagerPtr mgr,
> @@ -238,6 +349,13 @@ virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED)
> static int
> virSecurityDACOpen(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED)
> {
> + if (virThreadLocalInit(&chownList,
> + virSecurityDACChownListFree) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to initialize thread local variable"));
> + return -1;
> + }
> +
> return 0;
> }
>
> @@ -278,6 +396,69 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr)
> return 0;
> }
>
> +static int
> +virSecurityDACTransactionStart(virSecurityManagerPtr mgr)
> +{
> + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> + virSecurityDACChownListPtr list;
> +
> + list = virThreadLocalGet(&chownList);
> + if (list) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Another relabel transaction is already started"));
> + return -1;
> + }
> +
> + if (VIR_ALLOC(list) < 0)
> + return -1;
> +
> + list->priv = priv;
> +
> + if (virThreadLocalSet(&chownList, list) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to set thread local variable"));
> + VIR_FREE(list);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +virSecurityDACTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + pid_t pid)
> +{
> + virSecurityDACChownListPtr list;
> + int ret = 0;
> +
> + list = virThreadLocalGet(&chownList);
> + if (!list)
> + return 0;
Wouldn't !list only happen if there was an error and we're calling
Commit more or less unexpectedly (either after some error that caused
Abort() to be called or a second time)? IOW: Should this be an error?
> +
> + if (virProcessRunInMountNamespace(pid,
> + virSecurityDACTransactionRun,
> + list) < 0)
> + ret = -1;
> +
> + ignore_value(virThreadLocalSet(&chownList, NULL));
Not that it should fail, but part of me wonders if a VIR_DEBUG should
issued... Secondary to that - should setting to NULL be done before
RunInMountNamespace and not in TransactionRun?
Also for a rollback/error type handling, rather than passing list -
should we pass a structure that includes list and a count of successful
commits. That could be compared against list.nItems and on error
returned to the caller to have some chance at successful rollback.
> + virSecurityDACChownListFree(list);
> + return ret;
> +}
> +
> +static void
> +virSecurityDACTransactionAbort(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED)
> +{
> + virSecurityDACChownListPtr list;
> +
> + list = virThreadLocalGet(&chownList);
> + if (!list)
> + return;
> +
> + ignore_value(virThreadLocalSet(&chownList, NULL));
Not that it should fail, but part of me wonders if a VIR_DEBUG should
issued...
> + virSecurityDACChownListFree(list);
> +}
> +
> +
> static int
> virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv,
> const virStorageSource *src,
> @@ -287,6 +468,14 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv,
> {
> int rc;
>
> + /* Be aware that this function might run in a separate process.
> + * Therefore, any driver state changes would be thrown away. */
> +
> + if ((rc = virSecurityDACTransactionAppend(path, src, uid, gid)) < 0)
> + return -1;
> + else if (rc > 0)
> + return 0;
> +
If we call virSecurityDACChownListAppend and ret = 0, then we still fall
into the code below which from my read of what this code is going to do
may not be expected...
John
> VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
> NULLSTR(src ? src->path : path), (long) uid, (long) gid);
>
> @@ -1626,6 +1815,10 @@ virSecurityDriver virSecurityDriverDAC = {
>
> .preFork = virSecurityDACPreFork,
>
> + .transactionStart = virSecurityDACTransactionStart,
> + .transactionCommit = virSecurityDACTransactionCommit,
> + .transactionAbort = virSecurityDACTransactionAbort,
> +
> .domainSecurityVerify = virSecurityDACVerify,
>
> .domainSetSecurityDiskLabel = virSecurityDACSetDiskLabel,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
