Re: [libvirt] [PATCH 4/4] lxc: store tty pid

[Jim Meyering <jim meyering net>]

Hi Dave,

All four of your patches look good.
I've included a few comments on the fourth below.

Dave Leskovec <dlesko linux vnet ibm com> wrote:
> Index: b/src/lxc_conf.h
> ===================================================================
...
> +int lxcStoreTtyPid(lxc_driver_t *driver, lxc_vm_t *vm);
> +int lxcLoadTtyPid(lxc_driver_t *driver, lxc_vm_t *vm);
> +int lxcDeleteTtyPid(lxc_vm_t *vm);

It looks like each of the above pointer parameters
may/should be "const".

Renaming lxcDeleteTtyPid to lxcDeleteTtyPidFile would make it
clear that it deletes the file, not the PID.

> Index: b/src/lxc_conf.c
> ===================================================================
...
> +int lxcStoreTtyPid(lxc_driver_t *driver, lxc_vm_t *vm)
> +{
> +    int rc = -1;
> +    int fd = -1;

There is no need to initialize fd here.

> +    FILE *file = NULL;
> +
> +    if (vm->ttyPidFile[0] == 0x00) {
> +        if (virFileBuildPath(driver->configDir, vm->def->name, ".pid",
> +                             vm->ttyPidFile, PATH_MAX) < 0) {
> +            lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
...

> +int lxcDeleteTtyPid(lxc_vm_t *vm)
> +{
> +    if (vm->ttyPidFile[0] == 0x00) {
> +        goto no_file;
> +    }
> +
> +    if (unlink(vm->ttyPidFile) < 0) {
> +        lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> +                 _("cannot remove ttyPidFile %s"), vm->ttyPidFile);

Please include strerror(errno) in the diagnostic,
as you've done for other failed sys/lib calls.

Do you want to give a diagnostic even for ENOENT?