[Libvir] [RFC] 2 of 4 Linux Container support - lxc_conf files

[Jim Meyering]

Dave Leskovec <dlesko at linux.vnet.ibm.com> wrote:
> This patch adds the lxc_conf source files.

Looks good.

Please add lxcError to the err_func_re list in Makefile.maint, so that
"make syntax-check" checks all uses for translatable string mark-up.

> +#define LXC_MAX_TTY_NAME 32

LXC_MAX_TTY_NAME_LENGTH would be a tiny bit clearer.
Otherwise, one might think this represents the maximum number of TTYs.

> +typedef struct __lxc_driver lxc_driver_t;
> +struct __lxc_driver {
> +    lxc_vm_t *vms;
> +    int nactivevms;
> +    int ninactivevms;

I find mono-case, no-_-separator names hard to read.
Maybe nActiveVMs and nInactiveVMs instead, since that
seems to be the style here?

> +    char* configDir;
> +};
> +
> +/* Types and structs */
> +
> +/* Inline Functions */
> +static inline int lxcIsActiveVM(lxc_vm_t *vm)

That pointer parameter can (and hence should) be "const".
Otherwise, any caller with a const *vm pointer must add a dangerous
const-removing cast just to apply this function without compiler warning.
Please audit the other interfaces below, and make pointer parameters
"const" wherever possible.

For example, at least the "vm" parameter to lxcGenerateXML
looks like it may be made const.

...
> +char *lxcGenerateXML(virConnectPtr conn,
> +                     lxc_driver_t *driver,
> +                     lxc_vm_t *vm,
> +                     lxc_vm_def_t *def);
...

> +/* Functions */
> +void lxcError(virConnectPtr conn, virDomainPtr dom, int code,
> +              const char *fmt, ...)
> +{

This function is almost identical to at least 3 other <subsys>Error
functions in libvirt.  No problem adding it now, but eventually, we
really should factor out the common bits.

> +    va_list args;
> +    char errorMessage[LXC_MAX_ERROR_LEN];
> +    const char *codeErrorMessage;
> +
> +    if (fmt) {
> +        va_start(args, fmt);
> +        vsnprintf(errorMessage, LXC_MAX_ERROR_LEN-1, fmt, args);
> +        va_end(args);
> +    } else {
> +        errorMessage[0] = '\0';
> +    }
> +
> +    codeErrorMessage = __virErrorMsg(code, fmt);
> +    __virRaiseError(conn, dom, NULL, VIR_FROM_LXC, code, VIR_ERR_ERROR,
> +                    codeErrorMessage, errorMessage, NULL, 0, 0,
> +                    codeErrorMessage, errorMessage);
> +}
...
> +static int lxcParseMountXML(virConnectPtr conn, xmlNodePtr nodePtr,
> +                            lxc_mount_t *lxcMount)
> +{
> +    xmlChar *fsType = NULL;
> +    xmlNodePtr curNode;
> +    xmlChar *mountSource = NULL;
> +    xmlChar *mountTarget = NULL;
> +    int strLen;
> +    int rc = -1;
> +
> +    if (NULL == (fsType = xmlGetProp(nodePtr, BAD_CAST "type"))) {
> +        lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
> +                 _("missing filesystem type"));
> +        goto error;
> +    }
> +
> +    if (xmlStrEqual(fsType, BAD_CAST "mount") == 0) {
> +        lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
> +                 _("invalid filesystem type"));

It'd be nice to include the offending type name in the diagnostic.

...
> +    strLen = xmlStrlen(mountSource);
> +    if ((strLen > (PATH_MAX-1)) || (0 == strLen)) {
> +        lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
> +                 _("empty or invalid mount source"));

Just say it's invalid, and include the offending string in the diagnostic:
    _("invalid mount source: '%s'"), ...

> +        goto error;
> +    }
> +
...
> +int lxcLoadContainerInfo(virConnectPtr conn)
> +{
> +    int rc = -1;
> +    lxc_driver_t *driverPtr = (lxc_driver_t*)conn->privateData;
> +    DIR *dir;
> +    struct dirent *dirEntry;
> +
> +    if (!(dir = opendir(driverPtr->configDir))) {
> +        if (ENOENT == errno) {
> +            /* no config dir => no containers */
> +            rc = 0;
> +        } else {
> +            lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
> +                     _("failed to open config directory: %s"), strerror(errno));

Another "it'd be nice" suggestion:
include the directory name in the diagnostic.

> +        }
> +
> +        goto load_complete;
> +    }
...
> +int lxcDeleteConfig(virConnectPtr conn,
> +                    lxc_driver_t *driver ATTRIBUTE_UNUSED,
> +                    const char *configFile,
> +                    const char *name)
> +{
> +    if (!configFile[0]) {
> +        lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
> +                 _("no config file for %s"), name);
> +        return -1;
> +    }
> +
> +    if (unlink(configFile) < 0) {
> +        lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
> +                 _("cannot remove config for %s"), name);

It'd be good to include strerror(errno).

> +        return -1;
> +    }
...