On 07.12.2014 04:17, Ryan Cleere wrote:
> ---
> src/conf/domain_conf.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 12 +++++
> src/lxc/lxc_controller.c | 12 +++++
> src/util/virprocess.c | 4 +-
> src/util/virprocess.h | 3 ++
> 5 files changed, 144 insertions(+), 2 deletions(-)
First of all, how would this play with systemd? I mean, what if systemd within the container wants to set up its own limits?
Then, you are extending the domain XML space without any documentation. All XML changes must go hand in hand with documentation and RNG schema change too.
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 2d81c37..a673dc2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -26,6 +26,7 @@
> #include <dirent.h>
> #include <fcntl.h>
> #include <strings.h>
> +#include <sys/resource.h>
> #include <sys/stat.h>
> #include <unistd.h>
>
> @@ -1003,7 +1004,86 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root,
> return -1;
> }
>
> +static virDomainRLimitsPtr
> +virDomainRLimitsNew(void)
> +{
> + virDomainRLimitsPtr def = NULL;
> +
> + if (VIR_ALLOC(def) < 0)
> + return NULL;
> +
> + return def;
> +}
What's the reason for this function? I mean, unless it is setting some defauls, or doing something different than VIR_ALLOC() I don't think we need that. Esp. if there's no Free() counterpart.
> +
> +static virDomainRLimitsPtr
> +virDomainRLimitParseXML(xmlNodePtr node)
> +{
> + char *c = NULL;
> + long long val;
> + virDomainRLimitsPtr def;
>
> + if (!(def = virDomainRLimitsNew()))
> + return NULL;
> +
> + if (node->type == XML_ELEMENT_NODE) {
> + c = (char *)xmlNodeGetContent(node);
> + if (virStrToLong_ll(c, NULL, 10, &val) < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("could not parse rlimit value of %s"),
> + c);
> + goto error;
> + }
> + VIR_FREE(c);
> +
> + def->limit = val;
> + if (VIR_STRDUP(def->name, (char *)node->name) < 0)
> + goto error;
And here we go. If this VIR_STRDUP succeeds but the node->name is not known to the following huge if-else chunk, the string is leaked. Moreover, the def->name is just string translation of def->resource. The translation is static so I think you should use VIR_ENUM_{DECL,IMPL} macros rather than the following.
So, if you go with VIR_ENUM_* that I've mentioned above, this code would boil down to:
> +
> + if (xmlStrEqual(node->name, BAD_CAST "as")) {
> + def->resource = RLIMIT_AS;
> + } else if (xmlStrEqual(node->name, BAD_CAST "core")) {
> + def->resource = RLIMIT_CORE;
> + } else if (xmlStrEqual(node->name, BAD_CAST "cpu")) {
> + def->resource = RLIMIT_CPU;
> + } else if (xmlStrEqual(node->name, BAD_CAST "data")) {
> + def->resource = RLIMIT_DATA;
> + } else if (xmlStrEqual(node->name, BAD_CAST "fsize")) {
> + def->resource = RLIMIT_FSIZE;
> + } else if (xmlStrEqual(node->name, BAD_CAST "locks")) {
> + def->resource = RLIMIT_LOCKS;
> + } else if (xmlStrEqual(node->name, BAD_CAST "memlock")) {
> + def->resource = RLIMIT_MEMLOCK;
> + } else if (xmlStrEqual(node->name, BAD_CAST "msgqueue")) {
> + def->resource = RLIMIT_MSGQUEUE;
> + } else if (xmlStrEqual(node->name, BAD_CAST "nice")) {
> + def->resource = RLIMIT_NICE;
> + } else if (xmlStrEqual(node->name, BAD_CAST "nofile")) {
> + def->resource = RLIMIT_NOFILE;
> + } else if (xmlStrEqual(node->name, BAD_CAST "nproc")) {
> + def->resource = RLIMIT_NPROC;
> + } else if (xmlStrEqual(node->name, BAD_CAST "rss")) {
> + def->resource = RLIMIT_RSS;
> + } else if (xmlStrEqual(node->name, BAD_CAST "rtprio")) {
> + def->resource = RLIMIT_RTPRIO;
> + } else if (xmlStrEqual(node->name, BAD_CAST "sigpending")) {
> + def->resource = RLIMIT_SIGPENDING;
> + } else if (xmlStrEqual(node->name, BAD_CAST "stack")) {
> + def->resource = RLIMIT_STACK;
if ((def->resource = virRLimitTypeFromString((const char *)node->name)) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("could not determine resource type of '%s'"),
node->name);
goto error;
}
> + } else {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("could not determine resource type of '%s'"),
> + node->name);
> + goto error;
The def->name is allocated here...
> + }
> + }
> +
> + return def;
> +
> + error:
> + VIR_FREE(c);
> + VIR_FREE(def);
.. and leaked here.
Since we will have VIR_ENUM_* we have TypeToString() function too. So the code can be rewritten to something like this:
> + return NULL;
> +}
>
> static void
> virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED)
> @@ -14180,6 +14260,28 @@ virDomainDefParseXML(xmlDocPtr xml,
>
> virHashFree(bootHash);
>
> + if ((node = virXPathNode("./rlimits[1]", ctxt)) != NULL && (n = virXMLChildElementCount(node)) > 0) {
> + xmlNodePtr cur = node->children;
> + if (n && VIR_ALLOC_N(def->rlimits, n) < 0)
> + goto error;
> +
> + for (i = 0; i < n; i++) {
> + if (!(def->rlimits[i] = virDomainRLimitParseXML(cur)))
> + goto error;
> + def->nrlimits++;
> + for (j = 0; j < i; j++) {
> + if (def->rlimits[j]->resource == def->rlimits[i]->resource) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("duplicate rlimit resources '%s'"),
> + def->rlimits[j]->name);
> + goto error;
> + }
> + }
> + cur = cur->next;
> + }
> + }
> + VIR_FREE(node);
> +
> return def;
>
> error:
> @@ -19759,6 +19861,19 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> goto error;
> }
>
> + if (def->nrlimits > 0) {
> + virBufferAddLit(buf, "<rlimits>\n");
> + virBufferAdjustIndent(buf, 2);
> + for (n = 0; n < def->nrlimits; n++) {
> + virBufferAsprintf(buf, "<%s>%lld</%s>\n",
> + def->rlimits[n]->name,
> + def->rlimits[n]->limit,
> + def->rlimits[n]->name);
virBufferAsprintf(buf, "<%s>%lld</%s>\n",
virRLimitTypeToString(def->rlimits[n]->resource),
def->rlimits[n]->limit,
virRLimitTypeToString(def->rlimits[n]->resource));
That way, the def->rlimits[n]->name is useless and can be dropped.
I'm surprised the linker wasn't unhappy with this. You need to expose the virProcessPrLimit in src/libvirt_private.syms too.
> + }
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</rlimits>\n");
> + }
> +
> virBufferAdjustIndent(buf, -2);
> virBufferAddLit(buf, "</domain>\n");
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 439f3c0..abad30e 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2021,6 +2021,15 @@ struct _virDomainPowerManagement {
> int s4;
> };
>
> +typedef struct _virDomainRLimits virDomainRLimits;
> +typedef virDomainRLimits *virDomainRLimitsPtr;
> +
> +struct _virDomainRLimits {
> + char *name;
> + int resource;
> + long long limit;
> +};
> +
> /*
> * Guest VM main configuration
> *
> @@ -2138,6 +2147,9 @@ struct _virDomainDef {
> size_t nshmems;
> virDomainShmemDefPtr *shmems;
>
> + size_t nrlimits;
> + virDomainRLimitsPtr *rlimits;
> +
> /* Only 1 */
> virDomainWatchdogDefPtr watchdog;
> virDomainMemballoonDefPtr memballoon;
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index 53a2c8d..ef5551e 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -2490,6 +2490,18 @@ int main(int argc, char *argv[])
> }
> }
>
> + VIR_INFO("nrlimits = %d", (int)ctrl->def->nrlimits);
> + if (ctrl->def->nrlimits > 0) {
> + struct rlimit rlim;
> + int n;
> + for (i = 0; i < ctrl->def->nrlimits; i++) {
> + rlim.rlim_max = rlim.rlim_cur = ctrl->def->rlimits[i]->limit;
> + n = virProcessPrLimit(0, ctrl->def->rlimits[i]->resource, &rlim);
> + if (n < 0)
> + goto cleanup;
> + }
> + }
> +
> rc = virLXCControllerRun(ctrl);
>
> cleanup:
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index 0c8a32f..9bb5370 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -675,13 +675,13 @@ int virProcessSetNamespaces(size_t nfdlist,
> }
>
> #if HAVE_PRLIMIT
> -static int
> +int
> virProcessPrLimit(pid_t pid, int resource, struct rlimit *rlim)
> {
> return prlimit(pid, resource, rlim, NULL);
> }
> #elif HAVE_SETRLIMIT
> -static int
> +int
> virProcessPrLimit(pid_t pid ATTRIBUTE_UNUSED,
> int resource ATTRIBUTE_UNUSED,
> struct rlimit *rlim ATTRIBUTE_UNUSED)
> diff --git a/src/util/virprocess.h b/src/util/virprocess.h
> index bcaede5..045f8d4 100644
> --- a/src/util/virprocess.h
> +++ b/src/util/virprocess.h
> @@ -22,6 +22,7 @@
> #ifndef __VIR_PROCESS_H__
> # define __VIR_PROCESS_H__
>
> +# include <sys/resource.h>
> # include <sys/types.h>
>
> # include "internal.h"
> @@ -73,4 +74,6 @@ typedef int (*virProcessNamespaceCallback)(pid_t pid, void *opaque);
> int virProcessRunInMountNamespace(pid_t pid,
> virProcessNamespaceCallback cb,
> void *opaque);
> +
> +int virProcessPrLimit(pid_t pid, int resource, struct rlimit *rlim);
> #endif /* __VIR_PROCESS_H__ */
>
Looking forward to v2.
Michal