qemu-ga: use key-value store to avoid recycling fd handles after restart

Hosts hold on to handles provided by guest-file-open for periods that can
span beyond the life of the qemu-ga process that issued them. Since these
are issued starting from 0 on every restart, we run the risk of issuing
duplicate handles after restarts/reboots.

As a result, users with a stale copy of these handles may end up
reading/writing corrupted data due to their existing handles effectively
being re-assigned to an unexpected file or offset.

We unfortunately do not issue handles as strings, but as integers, so a
solution such as using UUIDs can't be implemented without introducing a
new interface.

As a workaround, we fix this by implementing a persistent key-value store
that will be used to track the value of the last handle that was issued
across restarts/reboots to avoid issuing duplicates.

The store is automatically written to the same directory we currently
set via --statedir to track fsfreeze state, and so should be applicable
for stable releases where this flag is supported.

A follow-up can use this same store for handling fsfreeze state, but
that change is cosmetic and left out for now.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Cc: qemu-stable@nongnu.org
---
 qga/commands-posix.c   |   25 +++++--
 qga/guest-agent-core.h |    1 +
 qga/main.c             |  184 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 204 insertions(+), 6 deletions(-)

On Fri, Mar 01, 2013 at 11:40:27AM -0600, Michael Roth wrote:
> Hosts hold on to handles provided by guest-file-open for periods that can
> span beyond the life of the qemu-ga process that issued them. Since these
> are issued starting from 0 on every restart, we run the risk of issuing
> duplicate handles after restarts/reboots.
> 
> As a result, users with a stale copy of these handles may end up
> reading/writing corrupted data due to their existing handles effectively
> being re-assigned to an unexpected file or offset.
> 
> We unfortunately do not issue handles as strings, but as integers, so a
> solution such as using UUIDs can't be implemented without introducing a
> new interface.
> 
> As a workaround, we fix this by implementing a persistent key-value store
> that will be used to track the value of the last handle that was issued
> across restarts/reboots to avoid issuing duplicates.
> 
> The store is automatically written to the same directory we currently
> set via --statedir to track fsfreeze state, and so should be applicable
> for stable releases where this flag is supported.
> 
> A follow-up can use this same store for handling fsfreeze state, but
> that change is cosmetic and left out for now.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Cc: qemu-stable@nongnu.org

Thanks, applied to qga branch with a s/uint64/int64/ fix-up for
guest_file_handle_add()'s return value.

> ---
>  qga/commands-posix.c   |   25 +++++--
>  qga/guest-agent-core.h |    1 +
>  qga/main.c             |  184 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 204 insertions(+), 6 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 7a0202e..5d12716 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -129,14 +129,22 @@ static struct {
>      QTAILQ_HEAD(, GuestFileHandle) filehandles;
>  } guest_file_state;
>  
> -static void guest_file_handle_add(FILE *fh)
> +static uint64_t guest_file_handle_add(FILE *fh, Error **errp)
>  {
>      GuestFileHandle *gfh;
> +    uint64_t handle;
> +
> +    handle = ga_get_fd_handle(ga_state, errp);
> +    if (error_is_set(errp)) {
> +        return 0;
> +    }
>  
>      gfh = g_malloc0(sizeof(GuestFileHandle));
> -    gfh->id = fileno(fh);
> +    gfh->id = handle;
>      gfh->fh = fh;
>      QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
> +
> +    return handle;
>  }
>  
>  static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
> @@ -158,7 +166,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
>  {
>      FILE *fh;
>      int fd;
> -    int64_t ret = -1;
> +    int64_t ret = -1, handle;
>  
>      if (!has_mode) {
>          mode = "r";
> @@ -184,9 +192,14 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
>          return -1;
>      }
>  
> -    guest_file_handle_add(fh);
> -    slog("guest-file-open, handle: %d", fd);
> -    return fd;
> +    handle = guest_file_handle_add(fh, err);
> +    if (error_is_set(err)) {
> +        fclose(fh);
> +        return -1;
> +    }
> +
> +    slog("guest-file-open, handle: %d", handle);
> +    return handle;
>  }
>  
>  void qmp_guest_file_close(int64_t handle, Error **err)
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> index 3354598..624a559 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h
> @@ -35,6 +35,7 @@ bool ga_is_frozen(GAState *s);
>  void ga_set_frozen(GAState *s);
>  void ga_unset_frozen(GAState *s);
>  const char *ga_fsfreeze_hook(GAState *s);
> +int64_t ga_get_fd_handle(GAState *s, Error **errp);
>  
>  #ifndef _WIN32
>  void reopen_fd_to_null(int fd);
> diff --git a/qga/main.c b/qga/main.c
> index db281a5..3635430 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -15,6 +15,7 @@
>  #include <stdbool.h>
>  #include <glib.h>
>  #include <getopt.h>
> +#include <glib/gstdio.h>
>  #ifndef _WIN32
>  #include <syslog.h>
>  #include <sys/wait.h>
> @@ -30,6 +31,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/qmp/dispatch.h"
>  #include "qga/channel.h"
> +#include "qemu/bswap.h"
>  #ifdef _WIN32
>  #include "qga/service-win32.h"
>  #include <windows.h>
> @@ -53,6 +55,11 @@
>  #endif
>  #define QGA_SENTINEL_BYTE 0xFF
>  
> +typedef struct GAPersistentState {
> +#define QGA_PSTATE_DEFAULT_FD_COUNTER 1000
> +    int64_t fd_counter;
> +} GAPersistentState;
> +
>  struct GAState {
>      JSONMessageParser parser;
>      GMainLoop *main_loop;
> @@ -76,6 +83,8 @@ struct GAState {
>  #ifdef CONFIG_FSFREEZE
>      const char *fsfreeze_hook;
>  #endif
> +    const gchar *pstate_filepath;
> +    GAPersistentState pstate;
>  };
>  
>  struct GAState *ga_state;
> @@ -724,6 +733,171 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
>  }
>  #endif
>  
> +static void set_persistent_state_defaults(GAPersistentState *pstate)
> +{
> +    g_assert(pstate);
> +    pstate->fd_counter = QGA_PSTATE_DEFAULT_FD_COUNTER;
> +}
> +
> +static void persistent_state_from_keyfile(GAPersistentState *pstate,
> +                                          GKeyFile *keyfile)
> +{
> +    g_assert(pstate);
> +    g_assert(keyfile);
> +    /* if any fields are missing, either because the file was tampered with
> +     * by agents of chaos, or because the field wasn't present at the time the
> +     * file was created, the best we can ever do is start over with the default
> +     * values. so load them now, and ignore any errors in accessing key-value
> +     * pairs
> +     */
> +    set_persistent_state_defaults(pstate);
> +
> +    if (g_key_file_has_key(keyfile, "global", "fd_counter", NULL)) {
> +        pstate->fd_counter =
> +            g_key_file_get_int64(keyfile, "global", "fd_counter", NULL);
> +    }
> +}
> +
> +static void persistent_state_to_keyfile(const GAPersistentState *pstate,
> +                                        GKeyFile *keyfile)
> +{
> +    g_assert(pstate);
> +    g_assert(keyfile);
> +
> +    g_key_file_set_int64(keyfile, "global", "fd_counter", pstate->fd_counter);
> +}
> +
> +static gboolean write_persistent_state(const GAPersistentState *pstate,
> +                                       const gchar *path)
> +{
> +    GKeyFile *keyfile = g_key_file_new();
> +    GError *gerr = NULL;
> +    gboolean ret = true;
> +    gchar *data = NULL;
> +    gsize data_len;
> +
> +    g_assert(pstate);
> +
> +    persistent_state_to_keyfile(pstate, keyfile);
> +    data = g_key_file_to_data(keyfile, &data_len, &gerr);
> +    if (gerr) {
> +        g_critical("failed to convert persistent state to string: %s",
> +                   gerr->message);
> +        ret = false;
> +        goto out;
> +    }
> +
> +    g_file_set_contents(path, data, data_len, &gerr);
> +    if (gerr) {
> +        g_critical("failed to write persistent state to %s: %s",
> +                    path, gerr->message);
> +        ret = false;
> +        goto out;
> +    }
> +
> +out:
> +    if (gerr) {
> +        g_error_free(gerr);
> +    }
> +    if (keyfile) {
> +        g_key_file_free(keyfile);
> +    }
> +    g_free(data);
> +    return ret;
> +}
> +
> +static gboolean read_persistent_state(GAPersistentState *pstate,
> +                                      const gchar *path, gboolean frozen)
> +{
> +    GKeyFile *keyfile = NULL;
> +    GError *gerr = NULL;
> +    struct stat st;
> +    gboolean ret = true;
> +
> +    g_assert(pstate);
> +
> +    if (stat(path, &st) == -1) {
> +        /* it's okay if state file doesn't exist, but any other error
> +         * indicates a permissions issue or some other misconfiguration
> +         * that we likely won't be able to recover from.
> +         */
> +        if (errno != ENOENT) {
> +            g_critical("unable to access state file at path %s: %s",
> +                       path, strerror(errno));
> +            ret = false;
> +            goto out;
> +        }
> +
> +        /* file doesn't exist. initialize state to default values and
> +         * attempt to save now. (we could wait till later when we have
> +         * modified state we need to commit, but if there's a problem,
> +         * such as a missing parent directory, we want to catch it now)
> +         *
> +         * there is a potential scenario where someone either managed to
> +         * update the agent from a version that didn't use a key store
> +         * while qemu-ga thought the filesystem was frozen, or
> +         * deleted the key store prior to issuing a fsfreeze, prior
> +         * to restarting the agent. in this case we go ahead and defer
> +         * initial creation till we actually have modified state to
> +         * write, otherwise fail to recover from freeze.
> +         */
> +        set_persistent_state_defaults(pstate);
> +        if (!frozen) {
> +            ret = write_persistent_state(pstate, path);
> +            if (!ret) {
> +                g_critical("unable to create state file at path %s", path);
> +                ret = false;
> +                goto out;
> +            }
> +        }
> +        ret = true;
> +        goto out;
> +    }
> +
> +    keyfile = g_key_file_new();
> +    g_key_file_load_from_file(keyfile, path, 0, &gerr);
> +    if (gerr) {
> +        g_critical("error loading persistent state from path: %s, %s",
> +                   path, gerr->message);
> +        ret = false;
> +        goto out;
> +    }
> +
> +    persistent_state_from_keyfile(pstate, keyfile);
> +
> +out:
> +    if (keyfile) {
> +        g_key_file_free(keyfile);
> +    }
> +    if (gerr) {
> +        g_error_free(gerr);
> +    }
> +
> +    return ret;
> +}
> +
> +int64_t ga_get_fd_handle(GAState *s, Error **errp)
> +{
> +    int64_t handle;
> +
> +    g_assert(s->pstate_filepath);
> +    /* we blacklist commands and avoid operations that potentially require
> +     * writing to disk when we're in a frozen state. this includes opening
> +     * new files, so we should never get here in that situation
> +     */
> +    g_assert(!ga_is_frozen(s));
> +
> +    handle = s->pstate.fd_counter++;
> +    if (s->pstate.fd_counter < 0) {
> +        s->pstate.fd_counter = 0;
> +    }
> +    if (!write_persistent_state(&s->pstate, s->pstate_filepath)) {
> +        error_setg(errp, "failed to commit persistent state to disk");
> +    }
> +
> +    return handle;
> +}
> +
>  int main(int argc, char **argv)
>  {
>      const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
> @@ -853,7 +1027,9 @@ int main(int argc, char **argv)
>      ga_enable_logging(s);
>      s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
>                                                   state_dir);
> +    s->pstate_filepath = g_strdup_printf("%s/qga.state", state_dir);
>      s->frozen = false;
> +
>  #ifndef _WIN32
>      /* check if a previous instance of qemu-ga exited with filesystems' state
>       * marked as frozen. this could be a stale value (a non-qemu-ga process
> @@ -910,6 +1086,14 @@ int main(int argc, char **argv)
>          }
>      }
>  
> +    /* load persistent state from disk */
> +    if (!read_persistent_state(&s->pstate,
> +                               s->pstate_filepath,
> +                               ga_is_frozen(s))) {
> +        g_critical("failed to load persistent state");
> +        goto out_bad;
> +    }
> +
>      if (blacklist) {
>          s->blacklist = blacklist;
>          do {
> -- 
> 1.7.9.5
>

On Fri,  1 Mar 2013 11:40:27 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> Hosts hold on to handles provided by guest-file-open for periods that can
> span beyond the life of the qemu-ga process that issued them. Since these
> are issued starting from 0 on every restart, we run the risk of issuing
> duplicate handles after restarts/reboots.
> 
> As a result, users with a stale copy of these handles may end up
> reading/writing corrupted data due to their existing handles effectively
> being re-assigned to an unexpected file or offset.
> 
> We unfortunately do not issue handles as strings, but as integers, so a
> solution such as using UUIDs can't be implemented without introducing a
> new interface.
> 
> As a workaround, we fix this by implementing a persistent key-value store
> that will be used to track the value of the last handle that was issued
> across restarts/reboots to avoid issuing duplicates.
> 
> The store is automatically written to the same directory we currently
> set via --statedir to track fsfreeze state, and so should be applicable
> for stable releases where this flag is supported.

Did you consider that statedir is normally set to /var/run which is
cleared by some distros on system reboot? Is this OK?

Also, I find it unfortunate that fd_counter never goes down...

One more comment below.

> 
> A follow-up can use this same store for handling fsfreeze state, but
> that change is cosmetic and left out for now.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Cc: qemu-stable@nongnu.org
> ---
>  qga/commands-posix.c   |   25 +++++--
>  qga/guest-agent-core.h |    1 +
>  qga/main.c             |  184 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 204 insertions(+), 6 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 7a0202e..5d12716 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -129,14 +129,22 @@ static struct {
>      QTAILQ_HEAD(, GuestFileHandle) filehandles;
>  } guest_file_state;
>  
> -static void guest_file_handle_add(FILE *fh)
> +static uint64_t guest_file_handle_add(FILE *fh, Error **errp)
>  {
>      GuestFileHandle *gfh;
> +    uint64_t handle;
> +
> +    handle = ga_get_fd_handle(ga_state, errp);
> +    if (error_is_set(errp)) {
> +        return 0;
> +    }
>  
>      gfh = g_malloc0(sizeof(GuestFileHandle));
> -    gfh->id = fileno(fh);
> +    gfh->id = handle;
>      gfh->fh = fh;
>      QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
> +
> +    return handle;
>  }
>  
>  static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
> @@ -158,7 +166,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
>  {
>      FILE *fh;
>      int fd;
> -    int64_t ret = -1;
> +    int64_t ret = -1, handle;
>  
>      if (!has_mode) {
>          mode = "r";
> @@ -184,9 +192,14 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
>          return -1;
>      }
>  
> -    guest_file_handle_add(fh);
> -    slog("guest-file-open, handle: %d", fd);
> -    return fd;
> +    handle = guest_file_handle_add(fh, err);
> +    if (error_is_set(err)) {
> +        fclose(fh);
> +        return -1;
> +    }
> +
> +    slog("guest-file-open, handle: %d", handle);
> +    return handle;
>  }
>  
>  void qmp_guest_file_close(int64_t handle, Error **err)
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> index 3354598..624a559 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h
> @@ -35,6 +35,7 @@ bool ga_is_frozen(GAState *s);
>  void ga_set_frozen(GAState *s);
>  void ga_unset_frozen(GAState *s);
>  const char *ga_fsfreeze_hook(GAState *s);
> +int64_t ga_get_fd_handle(GAState *s, Error **errp);
>  
>  #ifndef _WIN32
>  void reopen_fd_to_null(int fd);
> diff --git a/qga/main.c b/qga/main.c
> index db281a5..3635430 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -15,6 +15,7 @@
>  #include <stdbool.h>
>  #include <glib.h>
>  #include <getopt.h>
> +#include <glib/gstdio.h>
>  #ifndef _WIN32
>  #include <syslog.h>
>  #include <sys/wait.h>
> @@ -30,6 +31,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/qmp/dispatch.h"
>  #include "qga/channel.h"
> +#include "qemu/bswap.h"
>  #ifdef _WIN32
>  #include "qga/service-win32.h"
>  #include <windows.h>
> @@ -53,6 +55,11 @@
>  #endif
>  #define QGA_SENTINEL_BYTE 0xFF
>  
> +typedef struct GAPersistentState {
> +#define QGA_PSTATE_DEFAULT_FD_COUNTER 1000
> +    int64_t fd_counter;
> +} GAPersistentState;
> +
>  struct GAState {
>      JSONMessageParser parser;
>      GMainLoop *main_loop;
> @@ -76,6 +83,8 @@ struct GAState {
>  #ifdef CONFIG_FSFREEZE
>      const char *fsfreeze_hook;
>  #endif
> +    const gchar *pstate_filepath;
> +    GAPersistentState pstate;
>  };
>  
>  struct GAState *ga_state;
> @@ -724,6 +733,171 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
>  }
>  #endif
>  
> +static void set_persistent_state_defaults(GAPersistentState *pstate)
> +{
> +    g_assert(pstate);
> +    pstate->fd_counter = QGA_PSTATE_DEFAULT_FD_COUNTER;
> +}
> +
> +static void persistent_state_from_keyfile(GAPersistentState *pstate,
> +                                          GKeyFile *keyfile)
> +{
> +    g_assert(pstate);
> +    g_assert(keyfile);
> +    /* if any fields are missing, either because the file was tampered with
> +     * by agents of chaos, or because the field wasn't present at the time the
> +     * file was created, the best we can ever do is start over with the default
> +     * values. so load them now, and ignore any errors in accessing key-value
> +     * pairs
> +     */
> +    set_persistent_state_defaults(pstate);
> +
> +    if (g_key_file_has_key(keyfile, "global", "fd_counter", NULL)) {
> +        pstate->fd_counter =
> +            g_key_file_get_int64(keyfile, "global", "fd_counter", NULL);
> +    }
> +}
> +
> +static void persistent_state_to_keyfile(const GAPersistentState *pstate,
> +                                        GKeyFile *keyfile)
> +{
> +    g_assert(pstate);
> +    g_assert(keyfile);
> +
> +    g_key_file_set_int64(keyfile, "global", "fd_counter", pstate->fd_counter);
> +}
> +
> +static gboolean write_persistent_state(const GAPersistentState *pstate,
> +                                       const gchar *path)
> +{
> +    GKeyFile *keyfile = g_key_file_new();
> +    GError *gerr = NULL;
> +    gboolean ret = true;
> +    gchar *data = NULL;
> +    gsize data_len;
> +
> +    g_assert(pstate);
> +
> +    persistent_state_to_keyfile(pstate, keyfile);
> +    data = g_key_file_to_data(keyfile, &data_len, &gerr);
> +    if (gerr) {
> +        g_critical("failed to convert persistent state to string: %s",
> +                   gerr->message);
> +        ret = false;
> +        goto out;
> +    }
> +
> +    g_file_set_contents(path, data, data_len, &gerr);
> +    if (gerr) {
> +        g_critical("failed to write persistent state to %s: %s",
> +                    path, gerr->message);
> +        ret = false;
> +        goto out;
> +    }
> +
> +out:
> +    if (gerr) {
> +        g_error_free(gerr);
> +    }
> +    if (keyfile) {
> +        g_key_file_free(keyfile);
> +    }
> +    g_free(data);
> +    return ret;
> +}
> +
> +static gboolean read_persistent_state(GAPersistentState *pstate,
> +                                      const gchar *path, gboolean frozen)
> +{
> +    GKeyFile *keyfile = NULL;
> +    GError *gerr = NULL;
> +    struct stat st;
> +    gboolean ret = true;
> +
> +    g_assert(pstate);
> +
> +    if (stat(path, &st) == -1) {
> +        /* it's okay if state file doesn't exist, but any other error
> +         * indicates a permissions issue or some other misconfiguration
> +         * that we likely won't be able to recover from.
> +         */
> +        if (errno != ENOENT) {
> +            g_critical("unable to access state file at path %s: %s",
> +                       path, strerror(errno));
> +            ret = false;
> +            goto out;
> +        }
> +
> +        /* file doesn't exist. initialize state to default values and
> +         * attempt to save now. (we could wait till later when we have
> +         * modified state we need to commit, but if there's a problem,
> +         * such as a missing parent directory, we want to catch it now)
> +         *
> +         * there is a potential scenario where someone either managed to
> +         * update the agent from a version that didn't use a key store
> +         * while qemu-ga thought the filesystem was frozen, or
> +         * deleted the key store prior to issuing a fsfreeze, prior
> +         * to restarting the agent. in this case we go ahead and defer
> +         * initial creation till we actually have modified state to
> +         * write, otherwise fail to recover from freeze.
> +         */
> +        set_persistent_state_defaults(pstate);
> +        if (!frozen) {
> +            ret = write_persistent_state(pstate, path);
> +            if (!ret) {
> +                g_critical("unable to create state file at path %s", path);
> +                ret = false;
> +                goto out;
> +            }
> +        }
> +        ret = true;
> +        goto out;
> +    }
> +
> +    keyfile = g_key_file_new();
> +    g_key_file_load_from_file(keyfile, path, 0, &gerr);
> +    if (gerr) {
> +        g_critical("error loading persistent state from path: %s, %s",
> +                   path, gerr->message);
> +        ret = false;
> +        goto out;
> +    }
> +
> +    persistent_state_from_keyfile(pstate, keyfile);
> +
> +out:
> +    if (keyfile) {
> +        g_key_file_free(keyfile);
> +    }
> +    if (gerr) {
> +        g_error_free(gerr);
> +    }
> +
> +    return ret;
> +}
> +
> +int64_t ga_get_fd_handle(GAState *s, Error **errp)
> +{
> +    int64_t handle;
> +
> +    g_assert(s->pstate_filepath);
> +    /* we blacklist commands and avoid operations that potentially require
> +     * writing to disk when we're in a frozen state. this includes opening
> +     * new files, so we should never get here in that situation
> +     */
> +    g_assert(!ga_is_frozen(s));
> +
> +    handle = s->pstate.fd_counter++;
> +    if (s->pstate.fd_counter < 0) {
> +        s->pstate.fd_counter = 0;
> +    }

Is this handling the overflow case? Can't fd 0 be in use already?

I'd check against fd_counter max value and return an error if that's reached.

> +    if (!write_persistent_state(&s->pstate, s->pstate_filepath)) {
> +        error_setg(errp, "failed to commit persistent state to disk");
> +    }
> +
> +    return handle;
> +}
> +
>  int main(int argc, char **argv)
>  {
>      const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
> @@ -853,7 +1027,9 @@ int main(int argc, char **argv)
>      ga_enable_logging(s);
>      s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
>                                                   state_dir);
> +    s->pstate_filepath = g_strdup_printf("%s/qga.state", state_dir);
>      s->frozen = false;
> +
>  #ifndef _WIN32
>      /* check if a previous instance of qemu-ga exited with filesystems' state
>       * marked as frozen. this could be a stale value (a non-qemu-ga process
> @@ -910,6 +1086,14 @@ int main(int argc, char **argv)
>          }
>      }
>  
> +    /* load persistent state from disk */
> +    if (!read_persistent_state(&s->pstate,
> +                               s->pstate_filepath,
> +                               ga_is_frozen(s))) {
> +        g_critical("failed to load persistent state");
> +        goto out_bad;
> +    }
> +
>      if (blacklist) {
>          s->blacklist = blacklist;
>          do {

On Wed, Mar 20, 2013 at 10:54:51AM -0400, Luiz Capitulino wrote:
> 
> On Fri,  1 Mar 2013 11:40:27 -0600
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > Hosts hold on to handles provided by guest-file-open for periods that can
> > span beyond the life of the qemu-ga process that issued them. Since these
> > are issued starting from 0 on every restart, we run the risk of issuing
> > duplicate handles after restarts/reboots.
> > 
> > As a result, users with a stale copy of these handles may end up
> > reading/writing corrupted data due to their existing handles effectively
> > being re-assigned to an unexpected file or offset.
> > 
> > We unfortunately do not issue handles as strings, but as integers, so a
> > solution such as using UUIDs can't be implemented without introducing a
> > new interface.
> > 
> > As a workaround, we fix this by implementing a persistent key-value store
> > that will be used to track the value of the last handle that was issued
> > across restarts/reboots to avoid issuing duplicates.
> > 
> > The store is automatically written to the same directory we currently
> > set via --statedir to track fsfreeze state, and so should be applicable
> > for stable releases where this flag is supported.
> 
> Did you consider that statedir is normally set to /var/run which is
> cleared by some distros on system reboot? Is this OK?

Yes, though it's not ideal. The statedir should be a directory that
persists across reboots to completely fix the problem. But using a
directory that at least persists until reboot will avoid duplicate handles
being issued for qemu-ga restarts. So it's an improvement at least,
which is why I think it's worth considering for stable as well.

> 
> Also, I find it unfortunate that fd_counter never goes down...

The required book-keeping makes it just not worth it, imo, but it could
be done in the future without breaking compatibility (see below)

Not ideal certainly, but I think it's about the best we can do with
integer handles unfortunately.

> 
> One more comment below.
> 
> > 
> > A follow-up can use this same store for handling fsfreeze state, but
> > that change is cosmetic and left out for now.
> > 
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > Cc: qemu-stable@nongnu.org
> > ---
> >  qga/commands-posix.c   |   25 +++++--
> >  qga/guest-agent-core.h |    1 +
> >  qga/main.c             |  184 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 204 insertions(+), 6 deletions(-)
> > 
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 7a0202e..5d12716 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -129,14 +129,22 @@ static struct {
> >      QTAILQ_HEAD(, GuestFileHandle) filehandles;
> >  } guest_file_state;
> >  
> > -static void guest_file_handle_add(FILE *fh)
> > +static uint64_t guest_file_handle_add(FILE *fh, Error **errp)
> >  {
> >      GuestFileHandle *gfh;
> > +    uint64_t handle;
> > +
> > +    handle = ga_get_fd_handle(ga_state, errp);
> > +    if (error_is_set(errp)) {
> > +        return 0;
> > +    }
> >  
> >      gfh = g_malloc0(sizeof(GuestFileHandle));
> > -    gfh->id = fileno(fh);
> > +    gfh->id = handle;
> >      gfh->fh = fh;
> >      QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
> > +
> > +    return handle;
> >  }
> >  
> >  static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
> > @@ -158,7 +166,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
> >  {
> >      FILE *fh;
> >      int fd;
> > -    int64_t ret = -1;
> > +    int64_t ret = -1, handle;
> >  
> >      if (!has_mode) {
> >          mode = "r";
> > @@ -184,9 +192,14 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
> >          return -1;
> >      }
> >  
> > -    guest_file_handle_add(fh);
> > -    slog("guest-file-open, handle: %d", fd);
> > -    return fd;
> > +    handle = guest_file_handle_add(fh, err);
> > +    if (error_is_set(err)) {
> > +        fclose(fh);
> > +        return -1;
> > +    }
> > +
> > +    slog("guest-file-open, handle: %d", handle);
> > +    return handle;
> >  }
> >  
> >  void qmp_guest_file_close(int64_t handle, Error **err)
> > diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> > index 3354598..624a559 100644
> > --- a/qga/guest-agent-core.h
> > +++ b/qga/guest-agent-core.h
> > @@ -35,6 +35,7 @@ bool ga_is_frozen(GAState *s);
> >  void ga_set_frozen(GAState *s);
> >  void ga_unset_frozen(GAState *s);
> >  const char *ga_fsfreeze_hook(GAState *s);
> > +int64_t ga_get_fd_handle(GAState *s, Error **errp);
> >  
> >  #ifndef _WIN32
> >  void reopen_fd_to_null(int fd);
> > diff --git a/qga/main.c b/qga/main.c
> > index db281a5..3635430 100644
> > --- a/qga/main.c
> > +++ b/qga/main.c
> > @@ -15,6 +15,7 @@
> >  #include <stdbool.h>
> >  #include <glib.h>
> >  #include <getopt.h>
> > +#include <glib/gstdio.h>
> >  #ifndef _WIN32
> >  #include <syslog.h>
> >  #include <sys/wait.h>
> > @@ -30,6 +31,7 @@
> >  #include "qapi/qmp/qerror.h"
> >  #include "qapi/qmp/dispatch.h"
> >  #include "qga/channel.h"
> > +#include "qemu/bswap.h"
> >  #ifdef _WIN32
> >  #include "qga/service-win32.h"
> >  #include <windows.h>
> > @@ -53,6 +55,11 @@
> >  #endif
> >  #define QGA_SENTINEL_BYTE 0xFF
> >  
> > +typedef struct GAPersistentState {
> > +#define QGA_PSTATE_DEFAULT_FD_COUNTER 1000
> > +    int64_t fd_counter;
> > +} GAPersistentState;
> > +
> >  struct GAState {
> >      JSONMessageParser parser;
> >      GMainLoop *main_loop;
> > @@ -76,6 +83,8 @@ struct GAState {
> >  #ifdef CONFIG_FSFREEZE
> >      const char *fsfreeze_hook;
> >  #endif
> > +    const gchar *pstate_filepath;
> > +    GAPersistentState pstate;
> >  };
> >  
> >  struct GAState *ga_state;
> > @@ -724,6 +733,171 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
> >  }
> >  #endif
> >  
> > +static void set_persistent_state_defaults(GAPersistentState *pstate)
> > +{
> > +    g_assert(pstate);
> > +    pstate->fd_counter = QGA_PSTATE_DEFAULT_FD_COUNTER;
> > +}
> > +
> > +static void persistent_state_from_keyfile(GAPersistentState *pstate,
> > +                                          GKeyFile *keyfile)
> > +{
> > +    g_assert(pstate);
> > +    g_assert(keyfile);
> > +    /* if any fields are missing, either because the file was tampered with
> > +     * by agents of chaos, or because the field wasn't present at the time the
> > +     * file was created, the best we can ever do is start over with the default
> > +     * values. so load them now, and ignore any errors in accessing key-value
> > +     * pairs
> > +     */
> > +    set_persistent_state_defaults(pstate);
> > +
> > +    if (g_key_file_has_key(keyfile, "global", "fd_counter", NULL)) {
> > +        pstate->fd_counter =
> > +            g_key_file_get_int64(keyfile, "global", "fd_counter", NULL);
> > +    }
> > +}
> > +
> > +static void persistent_state_to_keyfile(const GAPersistentState *pstate,
> > +                                        GKeyFile *keyfile)
> > +{
> > +    g_assert(pstate);
> > +    g_assert(keyfile);
> > +
> > +    g_key_file_set_int64(keyfile, "global", "fd_counter", pstate->fd_counter);
> > +}
> > +
> > +static gboolean write_persistent_state(const GAPersistentState *pstate,
> > +                                       const gchar *path)
> > +{
> > +    GKeyFile *keyfile = g_key_file_new();
> > +    GError *gerr = NULL;
> > +    gboolean ret = true;
> > +    gchar *data = NULL;
> > +    gsize data_len;
> > +
> > +    g_assert(pstate);
> > +
> > +    persistent_state_to_keyfile(pstate, keyfile);
> > +    data = g_key_file_to_data(keyfile, &data_len, &gerr);
> > +    if (gerr) {
> > +        g_critical("failed to convert persistent state to string: %s",
> > +                   gerr->message);
> > +        ret = false;
> > +        goto out;
> > +    }
> > +
> > +    g_file_set_contents(path, data, data_len, &gerr);
> > +    if (gerr) {
> > +        g_critical("failed to write persistent state to %s: %s",
> > +                    path, gerr->message);
> > +        ret = false;
> > +        goto out;
> > +    }
> > +
> > +out:
> > +    if (gerr) {
> > +        g_error_free(gerr);
> > +    }
> > +    if (keyfile) {
> > +        g_key_file_free(keyfile);
> > +    }
> > +    g_free(data);
> > +    return ret;
> > +}
> > +
> > +static gboolean read_persistent_state(GAPersistentState *pstate,
> > +                                      const gchar *path, gboolean frozen)
> > +{
> > +    GKeyFile *keyfile = NULL;
> > +    GError *gerr = NULL;
> > +    struct stat st;
> > +    gboolean ret = true;
> > +
> > +    g_assert(pstate);
> > +
> > +    if (stat(path, &st) == -1) {
> > +        /* it's okay if state file doesn't exist, but any other error
> > +         * indicates a permissions issue or some other misconfiguration
> > +         * that we likely won't be able to recover from.
> > +         */
> > +        if (errno != ENOENT) {
> > +            g_critical("unable to access state file at path %s: %s",
> > +                       path, strerror(errno));
> > +            ret = false;
> > +            goto out;
> > +        }
> > +
> > +        /* file doesn't exist. initialize state to default values and
> > +         * attempt to save now. (we could wait till later when we have
> > +         * modified state we need to commit, but if there's a problem,
> > +         * such as a missing parent directory, we want to catch it now)
> > +         *
> > +         * there is a potential scenario where someone either managed to
> > +         * update the agent from a version that didn't use a key store
> > +         * while qemu-ga thought the filesystem was frozen, or
> > +         * deleted the key store prior to issuing a fsfreeze, prior
> > +         * to restarting the agent. in this case we go ahead and defer
> > +         * initial creation till we actually have modified state to
> > +         * write, otherwise fail to recover from freeze.
> > +         */
> > +        set_persistent_state_defaults(pstate);
> > +        if (!frozen) {
> > +            ret = write_persistent_state(pstate, path);
> > +            if (!ret) {
> > +                g_critical("unable to create state file at path %s", path);
> > +                ret = false;
> > +                goto out;
> > +            }
> > +        }
> > +        ret = true;
> > +        goto out;
> > +    }
> > +
> > +    keyfile = g_key_file_new();
> > +    g_key_file_load_from_file(keyfile, path, 0, &gerr);
> > +    if (gerr) {
> > +        g_critical("error loading persistent state from path: %s, %s",
> > +                   path, gerr->message);
> > +        ret = false;
> > +        goto out;
> > +    }
> > +
> > +    persistent_state_from_keyfile(pstate, keyfile);
> > +
> > +out:
> > +    if (keyfile) {
> > +        g_key_file_free(keyfile);
> > +    }
> > +    if (gerr) {
> > +        g_error_free(gerr);
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +int64_t ga_get_fd_handle(GAState *s, Error **errp)
> > +{
> > +    int64_t handle;
> > +
> > +    g_assert(s->pstate_filepath);
> > +    /* we blacklist commands and avoid operations that potentially require
> > +     * writing to disk when we're in a frozen state. this includes opening
> > +     * new files, so we should never get here in that situation
> > +     */
> > +    g_assert(!ga_is_frozen(s));
> > +
> > +    handle = s->pstate.fd_counter++;
> > +    if (s->pstate.fd_counter < 0) {
> > +        s->pstate.fd_counter = 0;
> > +    }
> 
> Is this handling the overflow case? Can't fd 0 be in use already?

It could, but it's very unlikely that an overflow/counter reset would
result in issuing still-in-use handle, since guest-file-open would need
to be called 2^63 times in the meantime.

Not impossible though I guess, but not really something we can
avoid without maintaining a list of all outstanding handles that
persists across reboot. That would allow us to make a nice statement
like "only x outstanding handles supported at a time", but that's
potentially a lot of book-keeping for each guest-file-open. I think it
can be implemented using the same key store, we'd just introduce a new
key via g_key_file_get_integer_list/g_key_file_set_integer_list and give
it priority over the fd_counter key for issuing handles if it's present.

So it's potentially doable, I'm just not sure it's worth it atm.

> 
> I'd check against fd_counter max value and return an error if that's reached.

It's meant to be a cyclical counter, otherwise we'd hit a point where
qemu-ga just stopped working forever. In retrospect, "fd_counter" was a
poor name for the field, "next_fd_handle" is really what it is.

Also, welcome back! :)

> 
> > +    if (!write_persistent_state(&s->pstate, s->pstate_filepath)) {
> > +        error_setg(errp, "failed to commit persistent state to disk");
> > +    }
> > +
> > +    return handle;
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >      const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
> > @@ -853,7 +1027,9 @@ int main(int argc, char **argv)
> >      ga_enable_logging(s);
> >      s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
> >                                                   state_dir);
> > +    s->pstate_filepath = g_strdup_printf("%s/qga.state", state_dir);
> >      s->frozen = false;
> > +
> >  #ifndef _WIN32
> >      /* check if a previous instance of qemu-ga exited with filesystems' state
> >       * marked as frozen. this could be a stale value (a non-qemu-ga process
> > @@ -910,6 +1086,14 @@ int main(int argc, char **argv)
> >          }
> >      }
> >  
> > +    /* load persistent state from disk */
> > +    if (!read_persistent_state(&s->pstate,
> > +                               s->pstate_filepath,
> > +                               ga_is_frozen(s))) {
> > +        g_critical("failed to load persistent state");
> > +        goto out_bad;
> > +    }
> > +
> >      if (blacklist) {
> >          s->blacklist = blacklist;
> >          do {
>

On Wed, 20 Mar 2013 11:03:08 -0500
mdroth <mdroth@linux.vnet.ibm.com> wrote:

> On Wed, Mar 20, 2013 at 10:54:51AM -0400, Luiz Capitulino wrote:
> > 
> > On Fri,  1 Mar 2013 11:40:27 -0600
> > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > 
> > > Hosts hold on to handles provided by guest-file-open for periods that can
> > > span beyond the life of the qemu-ga process that issued them. Since these
> > > are issued starting from 0 on every restart, we run the risk of issuing
> > > duplicate handles after restarts/reboots.
> > > 
> > > As a result, users with a stale copy of these handles may end up
> > > reading/writing corrupted data due to their existing handles effectively
> > > being re-assigned to an unexpected file or offset.
> > > 
> > > We unfortunately do not issue handles as strings, but as integers, so a
> > > solution such as using UUIDs can't be implemented without introducing a
> > > new interface.
> > > 
> > > As a workaround, we fix this by implementing a persistent key-value store
> > > that will be used to track the value of the last handle that was issued
> > > across restarts/reboots to avoid issuing duplicates.
> > > 
> > > The store is automatically written to the same directory we currently
> > > set via --statedir to track fsfreeze state, and so should be applicable
> > > for stable releases where this flag is supported.
> > 
> > Did you consider that statedir is normally set to /var/run which is
> > cleared by some distros on system reboot? Is this OK?
> 
> Yes, though it's not ideal. The statedir should be a directory that
> persists across reboots to completely fix the problem. But using a
> directory that at least persists until reboot will avoid duplicate handles
> being issued for qemu-ga restarts. So it's an improvement at least,
> which is why I think it's worth considering for stable as well.

Agreed.

> > Also, I find it unfortunate that fd_counter never goes down...
> 
> The required book-keeping makes it just not worth it, imo, but it could
> be done in the future without breaking compatibility (see below)
> 
> Not ideal certainly, but I think it's about the best we can do with
> integer handles unfortunately.
> 
> > 
> > One more comment below.
> > 
> > > 
> > > A follow-up can use this same store for handling fsfreeze state, but
> > > that change is cosmetic and left out for now.
> > > 
> > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > Cc: qemu-stable@nongnu.org
> > > ---
> > >  qga/commands-posix.c   |   25 +++++--
> > >  qga/guest-agent-core.h |    1 +
> > >  qga/main.c             |  184 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 204 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > > index 7a0202e..5d12716 100644
> > > --- a/qga/commands-posix.c
> > > +++ b/qga/commands-posix.c
> > > @@ -129,14 +129,22 @@ static struct {
> > >      QTAILQ_HEAD(, GuestFileHandle) filehandles;
> > >  } guest_file_state;
> > >  
> > > -static void guest_file_handle_add(FILE *fh)
> > > +static uint64_t guest_file_handle_add(FILE *fh, Error **errp)
> > >  {
> > >      GuestFileHandle *gfh;
> > > +    uint64_t handle;
> > > +
> > > +    handle = ga_get_fd_handle(ga_state, errp);
> > > +    if (error_is_set(errp)) {
> > > +        return 0;
> > > +    }
> > >  
> > >      gfh = g_malloc0(sizeof(GuestFileHandle));
> > > -    gfh->id = fileno(fh);
> > > +    gfh->id = handle;
> > >      gfh->fh = fh;
> > >      QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
> > > +
> > > +    return handle;
> > >  }
> > >  
> > >  static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
> > > @@ -158,7 +166,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
> > >  {
> > >      FILE *fh;
> > >      int fd;
> > > -    int64_t ret = -1;
> > > +    int64_t ret = -1, handle;
> > >  
> > >      if (!has_mode) {
> > >          mode = "r";
> > > @@ -184,9 +192,14 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
> > >          return -1;
> > >      }
> > >  
> > > -    guest_file_handle_add(fh);
> > > -    slog("guest-file-open, handle: %d", fd);
> > > -    return fd;
> > > +    handle = guest_file_handle_add(fh, err);
> > > +    if (error_is_set(err)) {
> > > +        fclose(fh);
> > > +        return -1;
> > > +    }
> > > +
> > > +    slog("guest-file-open, handle: %d", handle);
> > > +    return handle;
> > >  }
> > >  
> > >  void qmp_guest_file_close(int64_t handle, Error **err)
> > > diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> > > index 3354598..624a559 100644
> > > --- a/qga/guest-agent-core.h
> > > +++ b/qga/guest-agent-core.h
> > > @@ -35,6 +35,7 @@ bool ga_is_frozen(GAState *s);
> > >  void ga_set_frozen(GAState *s);
> > >  void ga_unset_frozen(GAState *s);
> > >  const char *ga_fsfreeze_hook(GAState *s);
> > > +int64_t ga_get_fd_handle(GAState *s, Error **errp);
> > >  
> > >  #ifndef _WIN32
> > >  void reopen_fd_to_null(int fd);
> > > diff --git a/qga/main.c b/qga/main.c
> > > index db281a5..3635430 100644
> > > --- a/qga/main.c
> > > +++ b/qga/main.c
> > > @@ -15,6 +15,7 @@
> > >  #include <stdbool.h>
> > >  #include <glib.h>
> > >  #include <getopt.h>
> > > +#include <glib/gstdio.h>
> > >  #ifndef _WIN32
> > >  #include <syslog.h>
> > >  #include <sys/wait.h>
> > > @@ -30,6 +31,7 @@
> > >  #include "qapi/qmp/qerror.h"
> > >  #include "qapi/qmp/dispatch.h"
> > >  #include "qga/channel.h"
> > > +#include "qemu/bswap.h"
> > >  #ifdef _WIN32
> > >  #include "qga/service-win32.h"
> > >  #include <windows.h>
> > > @@ -53,6 +55,11 @@
> > >  #endif
> > >  #define QGA_SENTINEL_BYTE 0xFF
> > >  
> > > +typedef struct GAPersistentState {
> > > +#define QGA_PSTATE_DEFAULT_FD_COUNTER 1000
> > > +    int64_t fd_counter;
> > > +} GAPersistentState;
> > > +
> > >  struct GAState {
> > >      JSONMessageParser parser;
> > >      GMainLoop *main_loop;
> > > @@ -76,6 +83,8 @@ struct GAState {
> > >  #ifdef CONFIG_FSFREEZE
> > >      const char *fsfreeze_hook;
> > >  #endif
> > > +    const gchar *pstate_filepath;
> > > +    GAPersistentState pstate;
> > >  };
> > >  
> > >  struct GAState *ga_state;
> > > @@ -724,6 +733,171 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
> > >  }
> > >  #endif
> > >  
> > > +static void set_persistent_state_defaults(GAPersistentState *pstate)
> > > +{
> > > +    g_assert(pstate);
> > > +    pstate->fd_counter = QGA_PSTATE_DEFAULT_FD_COUNTER;
> > > +}
> > > +
> > > +static void persistent_state_from_keyfile(GAPersistentState *pstate,
> > > +                                          GKeyFile *keyfile)
> > > +{
> > > +    g_assert(pstate);
> > > +    g_assert(keyfile);
> > > +    /* if any fields are missing, either because the file was tampered with
> > > +     * by agents of chaos, or because the field wasn't present at the time the
> > > +     * file was created, the best we can ever do is start over with the default
> > > +     * values. so load them now, and ignore any errors in accessing key-value
> > > +     * pairs
> > > +     */
> > > +    set_persistent_state_defaults(pstate);
> > > +
> > > +    if (g_key_file_has_key(keyfile, "global", "fd_counter", NULL)) {
> > > +        pstate->fd_counter =
> > > +            g_key_file_get_int64(keyfile, "global", "fd_counter", NULL);
> > > +    }
> > > +}
> > > +
> > > +static void persistent_state_to_keyfile(const GAPersistentState *pstate,
> > > +                                        GKeyFile *keyfile)
> > > +{
> > > +    g_assert(pstate);
> > > +    g_assert(keyfile);
> > > +
> > > +    g_key_file_set_int64(keyfile, "global", "fd_counter", pstate->fd_counter);
> > > +}
> > > +
> > > +static gboolean write_persistent_state(const GAPersistentState *pstate,
> > > +                                       const gchar *path)
> > > +{
> > > +    GKeyFile *keyfile = g_key_file_new();
> > > +    GError *gerr = NULL;
> > > +    gboolean ret = true;
> > > +    gchar *data = NULL;
> > > +    gsize data_len;
> > > +
> > > +    g_assert(pstate);
> > > +
> > > +    persistent_state_to_keyfile(pstate, keyfile);
> > > +    data = g_key_file_to_data(keyfile, &data_len, &gerr);
> > > +    if (gerr) {
> > > +        g_critical("failed to convert persistent state to string: %s",
> > > +                   gerr->message);
> > > +        ret = false;
> > > +        goto out;
> > > +    }
> > > +
> > > +    g_file_set_contents(path, data, data_len, &gerr);
> > > +    if (gerr) {
> > > +        g_critical("failed to write persistent state to %s: %s",
> > > +                    path, gerr->message);
> > > +        ret = false;
> > > +        goto out;
> > > +    }
> > > +
> > > +out:
> > > +    if (gerr) {
> > > +        g_error_free(gerr);
> > > +    }
> > > +    if (keyfile) {
> > > +        g_key_file_free(keyfile);
> > > +    }
> > > +    g_free(data);
> > > +    return ret;
> > > +}
> > > +
> > > +static gboolean read_persistent_state(GAPersistentState *pstate,
> > > +                                      const gchar *path, gboolean frozen)
> > > +{
> > > +    GKeyFile *keyfile = NULL;
> > > +    GError *gerr = NULL;
> > > +    struct stat st;
> > > +    gboolean ret = true;
> > > +
> > > +    g_assert(pstate);
> > > +
> > > +    if (stat(path, &st) == -1) {
> > > +        /* it's okay if state file doesn't exist, but any other error
> > > +         * indicates a permissions issue or some other misconfiguration
> > > +         * that we likely won't be able to recover from.
> > > +         */
> > > +        if (errno != ENOENT) {
> > > +            g_critical("unable to access state file at path %s: %s",
> > > +                       path, strerror(errno));
> > > +            ret = false;
> > > +            goto out;
> > > +        }
> > > +
> > > +        /* file doesn't exist. initialize state to default values and
> > > +         * attempt to save now. (we could wait till later when we have
> > > +         * modified state we need to commit, but if there's a problem,
> > > +         * such as a missing parent directory, we want to catch it now)
> > > +         *
> > > +         * there is a potential scenario where someone either managed to
> > > +         * update the agent from a version that didn't use a key store
> > > +         * while qemu-ga thought the filesystem was frozen, or
> > > +         * deleted the key store prior to issuing a fsfreeze, prior
> > > +         * to restarting the agent. in this case we go ahead and defer
> > > +         * initial creation till we actually have modified state to
> > > +         * write, otherwise fail to recover from freeze.
> > > +         */
> > > +        set_persistent_state_defaults(pstate);
> > > +        if (!frozen) {
> > > +            ret = write_persistent_state(pstate, path);
> > > +            if (!ret) {
> > > +                g_critical("unable to create state file at path %s", path);
> > > +                ret = false;
> > > +                goto out;
> > > +            }
> > > +        }
> > > +        ret = true;
> > > +        goto out;
> > > +    }
> > > +
> > > +    keyfile = g_key_file_new();
> > > +    g_key_file_load_from_file(keyfile, path, 0, &gerr);
> > > +    if (gerr) {
> > > +        g_critical("error loading persistent state from path: %s, %s",
> > > +                   path, gerr->message);
> > > +        ret = false;
> > > +        goto out;
> > > +    }
> > > +
> > > +    persistent_state_from_keyfile(pstate, keyfile);
> > > +
> > > +out:
> > > +    if (keyfile) {
> > > +        g_key_file_free(keyfile);
> > > +    }
> > > +    if (gerr) {
> > > +        g_error_free(gerr);
> > > +    }
> > > +
> > > +    return ret;
> > > +}
> > > +
> > > +int64_t ga_get_fd_handle(GAState *s, Error **errp)
> > > +{
> > > +    int64_t handle;
> > > +
> > > +    g_assert(s->pstate_filepath);
> > > +    /* we blacklist commands and avoid operations that potentially require
> > > +     * writing to disk when we're in a frozen state. this includes opening
> > > +     * new files, so we should never get here in that situation
> > > +     */
> > > +    g_assert(!ga_is_frozen(s));
> > > +
> > > +    handle = s->pstate.fd_counter++;
> > > +    if (s->pstate.fd_counter < 0) {
> > > +        s->pstate.fd_counter = 0;
> > > +    }
> > 
> > Is this handling the overflow case? Can't fd 0 be in use already?
> 
> It could, but it's very unlikely that an overflow/counter reset would
> result in issuing still-in-use handle, since guest-file-open would need
> to be called 2^63 times in the meantime.

Agreed, but as you do check for that case and as the right fix is simple
and I think it's worth it. I can send a patch myself.

On Wed, Mar 20, 2013 at 12:58:51PM -0400, Luiz Capitulino wrote:
> On Wed, 20 Mar 2013 11:03:08 -0500
> mdroth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > On Wed, Mar 20, 2013 at 10:54:51AM -0400, Luiz Capitulino wrote:
> > > 
> > > On Fri,  1 Mar 2013 11:40:27 -0600
> > > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > > 
> > > > Hosts hold on to handles provided by guest-file-open for periods that can
> > > > span beyond the life of the qemu-ga process that issued them. Since these
> > > > are issued starting from 0 on every restart, we run the risk of issuing
> > > > duplicate handles after restarts/reboots.
> > > > 
> > > > As a result, users with a stale copy of these handles may end up
> > > > reading/writing corrupted data due to their existing handles effectively
> > > > being re-assigned to an unexpected file or offset.
> > > > 
> > > > We unfortunately do not issue handles as strings, but as integers, so a
> > > > solution such as using UUIDs can't be implemented without introducing a
> > > > new interface.
> > > > 
> > > > As a workaround, we fix this by implementing a persistent key-value store
> > > > that will be used to track the value of the last handle that was issued
> > > > across restarts/reboots to avoid issuing duplicates.
> > > > 
> > > > The store is automatically written to the same directory we currently
> > > > set via --statedir to track fsfreeze state, and so should be applicable
> > > > for stable releases where this flag is supported.
> > > 
> > > Did you consider that statedir is normally set to /var/run which is
> > > cleared by some distros on system reboot? Is this OK?
> > 
> > Yes, though it's not ideal. The statedir should be a directory that
> > persists across reboots to completely fix the problem. But using a
> > directory that at least persists until reboot will avoid duplicate handles
> > being issued for qemu-ga restarts. So it's an improvement at least,
> > which is why I think it's worth considering for stable as well.
> 
> Agreed.
> 
> > > Also, I find it unfortunate that fd_counter never goes down...
> > 
> > The required book-keeping makes it just not worth it, imo, but it could
> > be done in the future without breaking compatibility (see below)
> > 
> > Not ideal certainly, but I think it's about the best we can do with
> > integer handles unfortunately.
> > 
> > > 
> > > One more comment below.
> > > 
> > > > 
> > > > A follow-up can use this same store for handling fsfreeze state, but
> > > > that change is cosmetic and left out for now.
> > > > 
> > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > Cc: qemu-stable@nongnu.org
> > > > ---
> > > >  qga/commands-posix.c   |   25 +++++--
> > > >  qga/guest-agent-core.h |    1 +
> > > >  qga/main.c             |  184 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 204 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > > > index 7a0202e..5d12716 100644
> > > > --- a/qga/commands-posix.c
> > > > +++ b/qga/commands-posix.c
> > > > @@ -129,14 +129,22 @@ static struct {
> > > >      QTAILQ_HEAD(, GuestFileHandle) filehandles;
> > > >  } guest_file_state;
> > > >  
> > > > -static void guest_file_handle_add(FILE *fh)
> > > > +static uint64_t guest_file_handle_add(FILE *fh, Error **errp)
> > > >  {
> > > >      GuestFileHandle *gfh;
> > > > +    uint64_t handle;
> > > > +
> > > > +    handle = ga_get_fd_handle(ga_state, errp);
> > > > +    if (error_is_set(errp)) {
> > > > +        return 0;
> > > > +    }
> > > >  
> > > >      gfh = g_malloc0(sizeof(GuestFileHandle));
> > > > -    gfh->id = fileno(fh);
> > > > +    gfh->id = handle;
> > > >      gfh->fh = fh;
> > > >      QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
> > > > +
> > > > +    return handle;
> > > >  }
> > > >  
> > > >  static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
> > > > @@ -158,7 +166,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
> > > >  {
> > > >      FILE *fh;
> > > >      int fd;
> > > > -    int64_t ret = -1;
> > > > +    int64_t ret = -1, handle;
> > > >  
> > > >      if (!has_mode) {
> > > >          mode = "r";
> > > > @@ -184,9 +192,14 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
> > > >          return -1;
> > > >      }
> > > >  
> > > > -    guest_file_handle_add(fh);
> > > > -    slog("guest-file-open, handle: %d", fd);
> > > > -    return fd;
> > > > +    handle = guest_file_handle_add(fh, err);
> > > > +    if (error_is_set(err)) {
> > > > +        fclose(fh);
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    slog("guest-file-open, handle: %d", handle);
> > > > +    return handle;
> > > >  }
> > > >  
> > > >  void qmp_guest_file_close(int64_t handle, Error **err)
> > > > diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> > > > index 3354598..624a559 100644
> > > > --- a/qga/guest-agent-core.h
> > > > +++ b/qga/guest-agent-core.h
> > > > @@ -35,6 +35,7 @@ bool ga_is_frozen(GAState *s);
> > > >  void ga_set_frozen(GAState *s);
> > > >  void ga_unset_frozen(GAState *s);
> > > >  const char *ga_fsfreeze_hook(GAState *s);
> > > > +int64_t ga_get_fd_handle(GAState *s, Error **errp);
> > > >  
> > > >  #ifndef _WIN32
> > > >  void reopen_fd_to_null(int fd);
> > > > diff --git a/qga/main.c b/qga/main.c
> > > > index db281a5..3635430 100644
> > > > --- a/qga/main.c
> > > > +++ b/qga/main.c
> > > > @@ -15,6 +15,7 @@
> > > >  #include <stdbool.h>
> > > >  #include <glib.h>
> > > >  #include <getopt.h>
> > > > +#include <glib/gstdio.h>
> > > >  #ifndef _WIN32
> > > >  #include <syslog.h>
> > > >  #include <sys/wait.h>
> > > > @@ -30,6 +31,7 @@
> > > >  #include "qapi/qmp/qerror.h"
> > > >  #include "qapi/qmp/dispatch.h"
> > > >  #include "qga/channel.h"
> > > > +#include "qemu/bswap.h"
> > > >  #ifdef _WIN32
> > > >  #include "qga/service-win32.h"
> > > >  #include <windows.h>
> > > > @@ -53,6 +55,11 @@
> > > >  #endif
> > > >  #define QGA_SENTINEL_BYTE 0xFF
> > > >  
> > > > +typedef struct GAPersistentState {
> > > > +#define QGA_PSTATE_DEFAULT_FD_COUNTER 1000
> > > > +    int64_t fd_counter;
> > > > +} GAPersistentState;
> > > > +
> > > >  struct GAState {
> > > >      JSONMessageParser parser;
> > > >      GMainLoop *main_loop;
> > > > @@ -76,6 +83,8 @@ struct GAState {
> > > >  #ifdef CONFIG_FSFREEZE
> > > >      const char *fsfreeze_hook;
> > > >  #endif
> > > > +    const gchar *pstate_filepath;
> > > > +    GAPersistentState pstate;
> > > >  };
> > > >  
> > > >  struct GAState *ga_state;
> > > > @@ -724,6 +733,171 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
> > > >  }
> > > >  #endif
> > > >  
> > > > +static void set_persistent_state_defaults(GAPersistentState *pstate)
> > > > +{
> > > > +    g_assert(pstate);
> > > > +    pstate->fd_counter = QGA_PSTATE_DEFAULT_FD_COUNTER;
> > > > +}
> > > > +
> > > > +static void persistent_state_from_keyfile(GAPersistentState *pstate,
> > > > +                                          GKeyFile *keyfile)
> > > > +{
> > > > +    g_assert(pstate);
> > > > +    g_assert(keyfile);
> > > > +    /* if any fields are missing, either because the file was tampered with
> > > > +     * by agents of chaos, or because the field wasn't present at the time the
> > > > +     * file was created, the best we can ever do is start over with the default
> > > > +     * values. so load them now, and ignore any errors in accessing key-value
> > > > +     * pairs
> > > > +     */
> > > > +    set_persistent_state_defaults(pstate);
> > > > +
> > > > +    if (g_key_file_has_key(keyfile, "global", "fd_counter", NULL)) {
> > > > +        pstate->fd_counter =
> > > > +            g_key_file_get_int64(keyfile, "global", "fd_counter", NULL);
> > > > +    }
> > > > +}
> > > > +
> > > > +static void persistent_state_to_keyfile(const GAPersistentState *pstate,
> > > > +                                        GKeyFile *keyfile)
> > > > +{
> > > > +    g_assert(pstate);
> > > > +    g_assert(keyfile);
> > > > +
> > > > +    g_key_file_set_int64(keyfile, "global", "fd_counter", pstate->fd_counter);
> > > > +}
> > > > +
> > > > +static gboolean write_persistent_state(const GAPersistentState *pstate,
> > > > +                                       const gchar *path)
> > > > +{
> > > > +    GKeyFile *keyfile = g_key_file_new();
> > > > +    GError *gerr = NULL;
> > > > +    gboolean ret = true;
> > > > +    gchar *data = NULL;
> > > > +    gsize data_len;
> > > > +
> > > > +    g_assert(pstate);
> > > > +
> > > > +    persistent_state_to_keyfile(pstate, keyfile);
> > > > +    data = g_key_file_to_data(keyfile, &data_len, &gerr);
> > > > +    if (gerr) {
> > > > +        g_critical("failed to convert persistent state to string: %s",
> > > > +                   gerr->message);
> > > > +        ret = false;
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    g_file_set_contents(path, data, data_len, &gerr);
> > > > +    if (gerr) {
> > > > +        g_critical("failed to write persistent state to %s: %s",
> > > > +                    path, gerr->message);
> > > > +        ret = false;
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +out:
> > > > +    if (gerr) {
> > > > +        g_error_free(gerr);
> > > > +    }
> > > > +    if (keyfile) {
> > > > +        g_key_file_free(keyfile);
> > > > +    }
> > > > +    g_free(data);
> > > > +    return ret;
> > > > +}
> > > > +
> > > > +static gboolean read_persistent_state(GAPersistentState *pstate,
> > > > +                                      const gchar *path, gboolean frozen)
> > > > +{
> > > > +    GKeyFile *keyfile = NULL;
> > > > +    GError *gerr = NULL;
> > > > +    struct stat st;
> > > > +    gboolean ret = true;
> > > > +
> > > > +    g_assert(pstate);
> > > > +
> > > > +    if (stat(path, &st) == -1) {
> > > > +        /* it's okay if state file doesn't exist, but any other error
> > > > +         * indicates a permissions issue or some other misconfiguration
> > > > +         * that we likely won't be able to recover from.
> > > > +         */
> > > > +        if (errno != ENOENT) {
> > > > +            g_critical("unable to access state file at path %s: %s",
> > > > +                       path, strerror(errno));
> > > > +            ret = false;
> > > > +            goto out;
> > > > +        }
> > > > +
> > > > +        /* file doesn't exist. initialize state to default values and
> > > > +         * attempt to save now. (we could wait till later when we have
> > > > +         * modified state we need to commit, but if there's a problem,
> > > > +         * such as a missing parent directory, we want to catch it now)
> > > > +         *
> > > > +         * there is a potential scenario where someone either managed to
> > > > +         * update the agent from a version that didn't use a key store
> > > > +         * while qemu-ga thought the filesystem was frozen, or
> > > > +         * deleted the key store prior to issuing a fsfreeze, prior
> > > > +         * to restarting the agent. in this case we go ahead and defer
> > > > +         * initial creation till we actually have modified state to
> > > > +         * write, otherwise fail to recover from freeze.
> > > > +         */
> > > > +        set_persistent_state_defaults(pstate);
> > > > +        if (!frozen) {
> > > > +            ret = write_persistent_state(pstate, path);
> > > > +            if (!ret) {
> > > > +                g_critical("unable to create state file at path %s", path);
> > > > +                ret = false;
> > > > +                goto out;
> > > > +            }
> > > > +        }
> > > > +        ret = true;
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    keyfile = g_key_file_new();
> > > > +    g_key_file_load_from_file(keyfile, path, 0, &gerr);
> > > > +    if (gerr) {
> > > > +        g_critical("error loading persistent state from path: %s, %s",
> > > > +                   path, gerr->message);
> > > > +        ret = false;
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    persistent_state_from_keyfile(pstate, keyfile);
> > > > +
> > > > +out:
> > > > +    if (keyfile) {
> > > > +        g_key_file_free(keyfile);
> > > > +    }
> > > > +    if (gerr) {
> > > > +        g_error_free(gerr);
> > > > +    }
> > > > +
> > > > +    return ret;
> > > > +}
> > > > +
> > > > +int64_t ga_get_fd_handle(GAState *s, Error **errp)
> > > > +{
> > > > +    int64_t handle;
> > > > +
> > > > +    g_assert(s->pstate_filepath);
> > > > +    /* we blacklist commands and avoid operations that potentially require
> > > > +     * writing to disk when we're in a frozen state. this includes opening
> > > > +     * new files, so we should never get here in that situation
> > > > +     */
> > > > +    g_assert(!ga_is_frozen(s));
> > > > +
> > > > +    handle = s->pstate.fd_counter++;
> > > > +    if (s->pstate.fd_counter < 0) {
> > > > +        s->pstate.fd_counter = 0;
> > > > +    }
> > > 
> > > Is this handling the overflow case? Can't fd 0 be in use already?
> > 
> > It could, but it's very unlikely that an overflow/counter reset would
> > result in issuing still-in-use handle, since guest-file-open would need
> > to be called 2^63 times in the meantime.
> 
> Agreed, but as you do check for that case and as the right fix is simple
> and I think it's worth it. I can send a patch myself.
> 

A patch to switch to tracking a list of issued handles in the keystore,
or to return an error on overflow?

If the former, sounds good, but keep in mind that we need to update on
every guest-file-open/guest-file-close, so we'll want to impose a
reasonable max on the number of outstanding FDs and document it. I would
lean toward something conservation like 1024, can consider bumping it in
the future if that proves insufficient.

On Wed, 20 Mar 2013 12:26:30 -0500
mdroth <mdroth@linux.vnet.ibm.com> wrote:

> On Wed, Mar 20, 2013 at 12:58:51PM -0400, Luiz Capitulino wrote:
> > On Wed, 20 Mar 2013 11:03:08 -0500
> > mdroth <mdroth@linux.vnet.ibm.com> wrote:
> > 
> > > On Wed, Mar 20, 2013 at 10:54:51AM -0400, Luiz Capitulino wrote:
> > > > 
> > > > On Fri,  1 Mar 2013 11:40:27 -0600
> > > > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > > > 
> > > > > Hosts hold on to handles provided by guest-file-open for periods that can
> > > > > span beyond the life of the qemu-ga process that issued them. Since these
> > > > > are issued starting from 0 on every restart, we run the risk of issuing
> > > > > duplicate handles after restarts/reboots.
> > > > > 
> > > > > As a result, users with a stale copy of these handles may end up
> > > > > reading/writing corrupted data due to their existing handles effectively
> > > > > being re-assigned to an unexpected file or offset.
> > > > > 
> > > > > We unfortunately do not issue handles as strings, but as integers, so a
> > > > > solution such as using UUIDs can't be implemented without introducing a
> > > > > new interface.
> > > > > 
> > > > > As a workaround, we fix this by implementing a persistent key-value store
> > > > > that will be used to track the value of the last handle that was issued
> > > > > across restarts/reboots to avoid issuing duplicates.
> > > > > 
> > > > > The store is automatically written to the same directory we currently
> > > > > set via --statedir to track fsfreeze state, and so should be applicable
> > > > > for stable releases where this flag is supported.
> > > > 
> > > > Did you consider that statedir is normally set to /var/run which is
> > > > cleared by some distros on system reboot? Is this OK?
> > > 
> > > Yes, though it's not ideal. The statedir should be a directory that
> > > persists across reboots to completely fix the problem. But using a
> > > directory that at least persists until reboot will avoid duplicate handles
> > > being issued for qemu-ga restarts. So it's an improvement at least,
> > > which is why I think it's worth considering for stable as well.
> > 
> > Agreed.
> > 
> > > > Also, I find it unfortunate that fd_counter never goes down...
> > > 
> > > The required book-keeping makes it just not worth it, imo, but it could
> > > be done in the future without breaking compatibility (see below)
> > > 
> > > Not ideal certainly, but I think it's about the best we can do with
> > > integer handles unfortunately.
> > > 
> > > > 
> > > > One more comment below.
> > > > 
> > > > > 
> > > > > A follow-up can use this same store for handling fsfreeze state, but
> > > > > that change is cosmetic and left out for now.
> > > > > 
> > > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > > Cc: qemu-stable@nongnu.org
> > > > > ---
> > > > >  qga/commands-posix.c   |   25 +++++--
> > > > >  qga/guest-agent-core.h |    1 +
> > > > >  qga/main.c             |  184 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 204 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > > > > index 7a0202e..5d12716 100644
> > > > > --- a/qga/commands-posix.c
> > > > > +++ b/qga/commands-posix.c
> > > > > @@ -129,14 +129,22 @@ static struct {
> > > > >      QTAILQ_HEAD(, GuestFileHandle) filehandles;
> > > > >  } guest_file_state;
> > > > >  
> > > > > -static void guest_file_handle_add(FILE *fh)
> > > > > +static uint64_t guest_file_handle_add(FILE *fh, Error **errp)
> > > > >  {
> > > > >      GuestFileHandle *gfh;
> > > > > +    uint64_t handle;
> > > > > +
> > > > > +    handle = ga_get_fd_handle(ga_state, errp);
> > > > > +    if (error_is_set(errp)) {
> > > > > +        return 0;
> > > > > +    }
> > > > >  
> > > > >      gfh = g_malloc0(sizeof(GuestFileHandle));
> > > > > -    gfh->id = fileno(fh);
> > > > > +    gfh->id = handle;
> > > > >      gfh->fh = fh;
> > > > >      QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
> > > > > +
> > > > > +    return handle;
> > > > >  }
> > > > >  
> > > > >  static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
> > > > > @@ -158,7 +166,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
> > > > >  {
> > > > >      FILE *fh;
> > > > >      int fd;
> > > > > -    int64_t ret = -1;
> > > > > +    int64_t ret = -1, handle;
> > > > >  
> > > > >      if (!has_mode) {
> > > > >          mode = "r";
> > > > > @@ -184,9 +192,14 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
> > > > >          return -1;
> > > > >      }
> > > > >  
> > > > > -    guest_file_handle_add(fh);
> > > > > -    slog("guest-file-open, handle: %d", fd);
> > > > > -    return fd;
> > > > > +    handle = guest_file_handle_add(fh, err);
> > > > > +    if (error_is_set(err)) {
> > > > > +        fclose(fh);
> > > > > +        return -1;
> > > > > +    }
> > > > > +
> > > > > +    slog("guest-file-open, handle: %d", handle);
> > > > > +    return handle;
> > > > >  }
> > > > >  
> > > > >  void qmp_guest_file_close(int64_t handle, Error **err)
> > > > > diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> > > > > index 3354598..624a559 100644
> > > > > --- a/qga/guest-agent-core.h
> > > > > +++ b/qga/guest-agent-core.h
> > > > > @@ -35,6 +35,7 @@ bool ga_is_frozen(GAState *s);
> > > > >  void ga_set_frozen(GAState *s);
> > > > >  void ga_unset_frozen(GAState *s);
> > > > >  const char *ga_fsfreeze_hook(GAState *s);
> > > > > +int64_t ga_get_fd_handle(GAState *s, Error **errp);
> > > > >  
> > > > >  #ifndef _WIN32
> > > > >  void reopen_fd_to_null(int fd);
> > > > > diff --git a/qga/main.c b/qga/main.c
> > > > > index db281a5..3635430 100644
> > > > > --- a/qga/main.c
> > > > > +++ b/qga/main.c
> > > > > @@ -15,6 +15,7 @@
> > > > >  #include <stdbool.h>
> > > > >  #include <glib.h>
> > > > >  #include <getopt.h>
> > > > > +#include <glib/gstdio.h>
> > > > >  #ifndef _WIN32
> > > > >  #include <syslog.h>
> > > > >  #include <sys/wait.h>
> > > > > @@ -30,6 +31,7 @@
> > > > >  #include "qapi/qmp/qerror.h"
> > > > >  #include "qapi/qmp/dispatch.h"
> > > > >  #include "qga/channel.h"
> > > > > +#include "qemu/bswap.h"
> > > > >  #ifdef _WIN32
> > > > >  #include "qga/service-win32.h"
> > > > >  #include <windows.h>
> > > > > @@ -53,6 +55,11 @@
> > > > >  #endif
> > > > >  #define QGA_SENTINEL_BYTE 0xFF
> > > > >  
> > > > > +typedef struct GAPersistentState {
> > > > > +#define QGA_PSTATE_DEFAULT_FD_COUNTER 1000
> > > > > +    int64_t fd_counter;
> > > > > +} GAPersistentState;
> > > > > +
> > > > >  struct GAState {
> > > > >      JSONMessageParser parser;
> > > > >      GMainLoop *main_loop;
> > > > > @@ -76,6 +83,8 @@ struct GAState {
> > > > >  #ifdef CONFIG_FSFREEZE
> > > > >      const char *fsfreeze_hook;
> > > > >  #endif
> > > > > +    const gchar *pstate_filepath;
> > > > > +    GAPersistentState pstate;
> > > > >  };
> > > > >  
> > > > >  struct GAState *ga_state;
> > > > > @@ -724,6 +733,171 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
> > > > >  }
> > > > >  #endif
> > > > >  
> > > > > +static void set_persistent_state_defaults(GAPersistentState *pstate)
> > > > > +{
> > > > > +    g_assert(pstate);
> > > > > +    pstate->fd_counter = QGA_PSTATE_DEFAULT_FD_COUNTER;
> > > > > +}
> > > > > +
> > > > > +static void persistent_state_from_keyfile(GAPersistentState *pstate,
> > > > > +                                          GKeyFile *keyfile)
> > > > > +{
> > > > > +    g_assert(pstate);
> > > > > +    g_assert(keyfile);
> > > > > +    /* if any fields are missing, either because the file was tampered with
> > > > > +     * by agents of chaos, or because the field wasn't present at the time the
> > > > > +     * file was created, the best we can ever do is start over with the default
> > > > > +     * values. so load them now, and ignore any errors in accessing key-value
> > > > > +     * pairs
> > > > > +     */
> > > > > +    set_persistent_state_defaults(pstate);
> > > > > +
> > > > > +    if (g_key_file_has_key(keyfile, "global", "fd_counter", NULL)) {
> > > > > +        pstate->fd_counter =
> > > > > +            g_key_file_get_int64(keyfile, "global", "fd_counter", NULL);
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +static void persistent_state_to_keyfile(const GAPersistentState *pstate,
> > > > > +                                        GKeyFile *keyfile)
> > > > > +{
> > > > > +    g_assert(pstate);
> > > > > +    g_assert(keyfile);
> > > > > +
> > > > > +    g_key_file_set_int64(keyfile, "global", "fd_counter", pstate->fd_counter);
> > > > > +}
> > > > > +
> > > > > +static gboolean write_persistent_state(const GAPersistentState *pstate,
> > > > > +                                       const gchar *path)
> > > > > +{
> > > > > +    GKeyFile *keyfile = g_key_file_new();
> > > > > +    GError *gerr = NULL;
> > > > > +    gboolean ret = true;
> > > > > +    gchar *data = NULL;
> > > > > +    gsize data_len;
> > > > > +
> > > > > +    g_assert(pstate);
> > > > > +
> > > > > +    persistent_state_to_keyfile(pstate, keyfile);
> > > > > +    data = g_key_file_to_data(keyfile, &data_len, &gerr);
> > > > > +    if (gerr) {
> > > > > +        g_critical("failed to convert persistent state to string: %s",
> > > > > +                   gerr->message);
> > > > > +        ret = false;
> > > > > +        goto out;
> > > > > +    }
> > > > > +
> > > > > +    g_file_set_contents(path, data, data_len, &gerr);
> > > > > +    if (gerr) {
> > > > > +        g_critical("failed to write persistent state to %s: %s",
> > > > > +                    path, gerr->message);
> > > > > +        ret = false;
> > > > > +        goto out;
> > > > > +    }
> > > > > +
> > > > > +out:
> > > > > +    if (gerr) {
> > > > > +        g_error_free(gerr);
> > > > > +    }
> > > > > +    if (keyfile) {
> > > > > +        g_key_file_free(keyfile);
> > > > > +    }
> > > > > +    g_free(data);
> > > > > +    return ret;
> > > > > +}
> > > > > +
> > > > > +static gboolean read_persistent_state(GAPersistentState *pstate,
> > > > > +                                      const gchar *path, gboolean frozen)
> > > > > +{
> > > > > +    GKeyFile *keyfile = NULL;
> > > > > +    GError *gerr = NULL;
> > > > > +    struct stat st;
> > > > > +    gboolean ret = true;
> > > > > +
> > > > > +    g_assert(pstate);
> > > > > +
> > > > > +    if (stat(path, &st) == -1) {
> > > > > +        /* it's okay if state file doesn't exist, but any other error
> > > > > +         * indicates a permissions issue or some other misconfiguration
> > > > > +         * that we likely won't be able to recover from.
> > > > > +         */
> > > > > +        if (errno != ENOENT) {
> > > > > +            g_critical("unable to access state file at path %s: %s",
> > > > > +                       path, strerror(errno));
> > > > > +            ret = false;
> > > > > +            goto out;
> > > > > +        }
> > > > > +
> > > > > +        /* file doesn't exist. initialize state to default values and
> > > > > +         * attempt to save now. (we could wait till later when we have
> > > > > +         * modified state we need to commit, but if there's a problem,
> > > > > +         * such as a missing parent directory, we want to catch it now)
> > > > > +         *
> > > > > +         * there is a potential scenario where someone either managed to
> > > > > +         * update the agent from a version that didn't use a key store
> > > > > +         * while qemu-ga thought the filesystem was frozen, or
> > > > > +         * deleted the key store prior to issuing a fsfreeze, prior
> > > > > +         * to restarting the agent. in this case we go ahead and defer
> > > > > +         * initial creation till we actually have modified state to
> > > > > +         * write, otherwise fail to recover from freeze.
> > > > > +         */
> > > > > +        set_persistent_state_defaults(pstate);
> > > > > +        if (!frozen) {
> > > > > +            ret = write_persistent_state(pstate, path);
> > > > > +            if (!ret) {
> > > > > +                g_critical("unable to create state file at path %s", path);
> > > > > +                ret = false;
> > > > > +                goto out;
> > > > > +            }
> > > > > +        }
> > > > > +        ret = true;
> > > > > +        goto out;
> > > > > +    }
> > > > > +
> > > > > +    keyfile = g_key_file_new();
> > > > > +    g_key_file_load_from_file(keyfile, path, 0, &gerr);
> > > > > +    if (gerr) {
> > > > > +        g_critical("error loading persistent state from path: %s, %s",
> > > > > +                   path, gerr->message);
> > > > > +        ret = false;
> > > > > +        goto out;
> > > > > +    }
> > > > > +
> > > > > +    persistent_state_from_keyfile(pstate, keyfile);
> > > > > +
> > > > > +out:
> > > > > +    if (keyfile) {
> > > > > +        g_key_file_free(keyfile);
> > > > > +    }
> > > > > +    if (gerr) {
> > > > > +        g_error_free(gerr);
> > > > > +    }
> > > > > +
> > > > > +    return ret;
> > > > > +}
> > > > > +
> > > > > +int64_t ga_get_fd_handle(GAState *s, Error **errp)
> > > > > +{
> > > > > +    int64_t handle;
> > > > > +
> > > > > +    g_assert(s->pstate_filepath);
> > > > > +    /* we blacklist commands and avoid operations that potentially require
> > > > > +     * writing to disk when we're in a frozen state. this includes opening
> > > > > +     * new files, so we should never get here in that situation
> > > > > +     */
> > > > > +    g_assert(!ga_is_frozen(s));
> > > > > +
> > > > > +    handle = s->pstate.fd_counter++;
> > > > > +    if (s->pstate.fd_counter < 0) {
> > > > > +        s->pstate.fd_counter = 0;
> > > > > +    }
> > > > 
> > > > Is this handling the overflow case? Can't fd 0 be in use already?
> > > 
> > > It could, but it's very unlikely that an overflow/counter reset would
> > > result in issuing still-in-use handle, since guest-file-open would need
> > > to be called 2^63 times in the meantime.
> > 
> > Agreed, but as you do check for that case and as the right fix is simple
> > and I think it's worth it. I can send a patch myself.
> > 
> 
> A patch to switch to tracking a list of issued handles in the keystore,
> or to return an error on overflow?

To return an error on overflow. Minor, but if we do handle it let's do it
right. Or, we could just add an assert like:

assert(s->pstate.fd_counter >= 0);

> If the former, sounds good, but keep in mind that we need to update on
> every guest-file-open/guest-file-close, so we'll want to impose a
> reasonable max on the number of outstanding FDs and document it. I would
> lean toward something conservation like 1024, can consider bumping it in
> the future if that proves insufficient.

Maybe in the near future :)

On Wed, Mar 20, 2013 at 01:40:56PM -0400, Luiz Capitulino wrote:
> On Wed, 20 Mar 2013 12:26:30 -0500
> mdroth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > On Wed, Mar 20, 2013 at 12:58:51PM -0400, Luiz Capitulino wrote:
> > > On Wed, 20 Mar 2013 11:03:08 -0500
> > > mdroth <mdroth@linux.vnet.ibm.com> wrote:
> > > 
> > > > On Wed, Mar 20, 2013 at 10:54:51AM -0400, Luiz Capitulino wrote:
> > > > > 
> > > > > On Fri,  1 Mar 2013 11:40:27 -0600
> > > > > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > > > > 
> > > > > > Hosts hold on to handles provided by guest-file-open for periods that can
> > > > > > span beyond the life of the qemu-ga process that issued them. Since these
> > > > > > are issued starting from 0 on every restart, we run the risk of issuing
> > > > > > duplicate handles after restarts/reboots.
> > > > > > 
> > > > > > As a result, users with a stale copy of these handles may end up
> > > > > > reading/writing corrupted data due to their existing handles effectively
> > > > > > being re-assigned to an unexpected file or offset.
> > > > > > 
> > > > > > We unfortunately do not issue handles as strings, but as integers, so a
> > > > > > solution such as using UUIDs can't be implemented without introducing a
> > > > > > new interface.
> > > > > > 
> > > > > > As a workaround, we fix this by implementing a persistent key-value store
> > > > > > that will be used to track the value of the last handle that was issued
> > > > > > across restarts/reboots to avoid issuing duplicates.
> > > > > > 
> > > > > > The store is automatically written to the same directory we currently
> > > > > > set via --statedir to track fsfreeze state, and so should be applicable
> > > > > > for stable releases where this flag is supported.
> > > > > 
> > > > > Did you consider that statedir is normally set to /var/run which is
> > > > > cleared by some distros on system reboot? Is this OK?
> > > > 
> > > > Yes, though it's not ideal. The statedir should be a directory that
> > > > persists across reboots to completely fix the problem. But using a
> > > > directory that at least persists until reboot will avoid duplicate handles
> > > > being issued for qemu-ga restarts. So it's an improvement at least,
> > > > which is why I think it's worth considering for stable as well.
> > > 
> > > Agreed.
> > > 
> > > > > Also, I find it unfortunate that fd_counter never goes down...
> > > > 
> > > > The required book-keeping makes it just not worth it, imo, but it could
> > > > be done in the future without breaking compatibility (see below)
> > > > 
> > > > Not ideal certainly, but I think it's about the best we can do with
> > > > integer handles unfortunately.
> > > > 
> > > > > 
> > > > > One more comment below.
> > > > > 
> > > > > > 
> > > > > > A follow-up can use this same store for handling fsfreeze state, but
> > > > > > that change is cosmetic and left out for now.
> > > > > > 
> > > > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > > > Cc: qemu-stable@nongnu.org
> > > > > > ---
> > > > > >  qga/commands-posix.c   |   25 +++++--
> > > > > >  qga/guest-agent-core.h |    1 +
> > > > > >  qga/main.c             |  184 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  3 files changed, 204 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > > > > > index 7a0202e..5d12716 100644
> > > > > > --- a/qga/commands-posix.c
> > > > > > +++ b/qga/commands-posix.c
> > > > > > @@ -129,14 +129,22 @@ static struct {
> > > > > >      QTAILQ_HEAD(, GuestFileHandle) filehandles;
> > > > > >  } guest_file_state;
> > > > > >  
> > > > > > -static void guest_file_handle_add(FILE *fh)
> > > > > > +static uint64_t guest_file_handle_add(FILE *fh, Error **errp)
> > > > > >  {
> > > > > >      GuestFileHandle *gfh;
> > > > > > +    uint64_t handle;
> > > > > > +
> > > > > > +    handle = ga_get_fd_handle(ga_state, errp);
> > > > > > +    if (error_is_set(errp)) {
> > > > > > +        return 0;
> > > > > > +    }
> > > > > >  
> > > > > >      gfh = g_malloc0(sizeof(GuestFileHandle));
> > > > > > -    gfh->id = fileno(fh);
> > > > > > +    gfh->id = handle;
> > > > > >      gfh->fh = fh;
> > > > > >      QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
> > > > > > +
> > > > > > +    return handle;
> > > > > >  }
> > > > > >  
> > > > > >  static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
> > > > > > @@ -158,7 +166,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
> > > > > >  {
> > > > > >      FILE *fh;
> > > > > >      int fd;
> > > > > > -    int64_t ret = -1;
> > > > > > +    int64_t ret = -1, handle;
> > > > > >  
> > > > > >      if (!has_mode) {
> > > > > >          mode = "r";
> > > > > > @@ -184,9 +192,14 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, E
> > > > > >          return -1;
> > > > > >      }
> > > > > >  
> > > > > > -    guest_file_handle_add(fh);
> > > > > > -    slog("guest-file-open, handle: %d", fd);
> > > > > > -    return fd;
> > > > > > +    handle = guest_file_handle_add(fh, err);
> > > > > > +    if (error_is_set(err)) {
> > > > > > +        fclose(fh);
> > > > > > +        return -1;
> > > > > > +    }
> > > > > > +
> > > > > > +    slog("guest-file-open, handle: %d", handle);
> > > > > > +    return handle;
> > > > > >  }
> > > > > >  
> > > > > >  void qmp_guest_file_close(int64_t handle, Error **err)
> > > > > > diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> > > > > > index 3354598..624a559 100644
> > > > > > --- a/qga/guest-agent-core.h
> > > > > > +++ b/qga/guest-agent-core.h
> > > > > > @@ -35,6 +35,7 @@ bool ga_is_frozen(GAState *s);
> > > > > >  void ga_set_frozen(GAState *s);
> > > > > >  void ga_unset_frozen(GAState *s);
> > > > > >  const char *ga_fsfreeze_hook(GAState *s);
> > > > > > +int64_t ga_get_fd_handle(GAState *s, Error **errp);
> > > > > >  
> > > > > >  #ifndef _WIN32
> > > > > >  void reopen_fd_to_null(int fd);
> > > > > > diff --git a/qga/main.c b/qga/main.c
> > > > > > index db281a5..3635430 100644
> > > > > > --- a/qga/main.c
> > > > > > +++ b/qga/main.c
> > > > > > @@ -15,6 +15,7 @@
> > > > > >  #include <stdbool.h>
> > > > > >  #include <glib.h>
> > > > > >  #include <getopt.h>
> > > > > > +#include <glib/gstdio.h>
> > > > > >  #ifndef _WIN32
> > > > > >  #include <syslog.h>
> > > > > >  #include <sys/wait.h>
> > > > > > @@ -30,6 +31,7 @@
> > > > > >  #include "qapi/qmp/qerror.h"
> > > > > >  #include "qapi/qmp/dispatch.h"
> > > > > >  #include "qga/channel.h"
> > > > > > +#include "qemu/bswap.h"
> > > > > >  #ifdef _WIN32
> > > > > >  #include "qga/service-win32.h"
> > > > > >  #include <windows.h>
> > > > > > @@ -53,6 +55,11 @@
> > > > > >  #endif
> > > > > >  #define QGA_SENTINEL_BYTE 0xFF
> > > > > >  
> > > > > > +typedef struct GAPersistentState {
> > > > > > +#define QGA_PSTATE_DEFAULT_FD_COUNTER 1000
> > > > > > +    int64_t fd_counter;
> > > > > > +} GAPersistentState;
> > > > > > +
> > > > > >  struct GAState {
> > > > > >      JSONMessageParser parser;
> > > > > >      GMainLoop *main_loop;
> > > > > > @@ -76,6 +83,8 @@ struct GAState {
> > > > > >  #ifdef CONFIG_FSFREEZE
> > > > > >      const char *fsfreeze_hook;
> > > > > >  #endif
> > > > > > +    const gchar *pstate_filepath;
> > > > > > +    GAPersistentState pstate;
> > > > > >  };
> > > > > >  
> > > > > >  struct GAState *ga_state;
> > > > > > @@ -724,6 +733,171 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
> > > > > >  }
> > > > > >  #endif
> > > > > >  
> > > > > > +static void set_persistent_state_defaults(GAPersistentState *pstate)
> > > > > > +{
> > > > > > +    g_assert(pstate);
> > > > > > +    pstate->fd_counter = QGA_PSTATE_DEFAULT_FD_COUNTER;
> > > > > > +}
> > > > > > +
> > > > > > +static void persistent_state_from_keyfile(GAPersistentState *pstate,
> > > > > > +                                          GKeyFile *keyfile)
> > > > > > +{
> > > > > > +    g_assert(pstate);
> > > > > > +    g_assert(keyfile);
> > > > > > +    /* if any fields are missing, either because the file was tampered with
> > > > > > +     * by agents of chaos, or because the field wasn't present at the time the
> > > > > > +     * file was created, the best we can ever do is start over with the default
> > > > > > +     * values. so load them now, and ignore any errors in accessing key-value
> > > > > > +     * pairs
> > > > > > +     */
> > > > > > +    set_persistent_state_defaults(pstate);
> > > > > > +
> > > > > > +    if (g_key_file_has_key(keyfile, "global", "fd_counter", NULL)) {
> > > > > > +        pstate->fd_counter =
> > > > > > +            g_key_file_get_int64(keyfile, "global", "fd_counter", NULL);
> > > > > > +    }
> > > > > > +}
> > > > > > +
> > > > > > +static void persistent_state_to_keyfile(const GAPersistentState *pstate,
> > > > > > +                                        GKeyFile *keyfile)
> > > > > > +{
> > > > > > +    g_assert(pstate);
> > > > > > +    g_assert(keyfile);
> > > > > > +
> > > > > > +    g_key_file_set_int64(keyfile, "global", "fd_counter", pstate->fd_counter);
> > > > > > +}
> > > > > > +
> > > > > > +static gboolean write_persistent_state(const GAPersistentState *pstate,
> > > > > > +                                       const gchar *path)
> > > > > > +{
> > > > > > +    GKeyFile *keyfile = g_key_file_new();
> > > > > > +    GError *gerr = NULL;
> > > > > > +    gboolean ret = true;
> > > > > > +    gchar *data = NULL;
> > > > > > +    gsize data_len;
> > > > > > +
> > > > > > +    g_assert(pstate);
> > > > > > +
> > > > > > +    persistent_state_to_keyfile(pstate, keyfile);
> > > > > > +    data = g_key_file_to_data(keyfile, &data_len, &gerr);
> > > > > > +    if (gerr) {
> > > > > > +        g_critical("failed to convert persistent state to string: %s",
> > > > > > +                   gerr->message);
> > > > > > +        ret = false;
> > > > > > +        goto out;
> > > > > > +    }
> > > > > > +
> > > > > > +    g_file_set_contents(path, data, data_len, &gerr);
> > > > > > +    if (gerr) {
> > > > > > +        g_critical("failed to write persistent state to %s: %s",
> > > > > > +                    path, gerr->message);
> > > > > > +        ret = false;
> > > > > > +        goto out;
> > > > > > +    }
> > > > > > +
> > > > > > +out:
> > > > > > +    if (gerr) {
> > > > > > +        g_error_free(gerr);
> > > > > > +    }
> > > > > > +    if (keyfile) {
> > > > > > +        g_key_file_free(keyfile);
> > > > > > +    }
> > > > > > +    g_free(data);
> > > > > > +    return ret;
> > > > > > +}
> > > > > > +
> > > > > > +static gboolean read_persistent_state(GAPersistentState *pstate,
> > > > > > +                                      const gchar *path, gboolean frozen)
> > > > > > +{
> > > > > > +    GKeyFile *keyfile = NULL;
> > > > > > +    GError *gerr = NULL;
> > > > > > +    struct stat st;
> > > > > > +    gboolean ret = true;
> > > > > > +
> > > > > > +    g_assert(pstate);
> > > > > > +
> > > > > > +    if (stat(path, &st) == -1) {
> > > > > > +        /* it's okay if state file doesn't exist, but any other error
> > > > > > +         * indicates a permissions issue or some other misconfiguration
> > > > > > +         * that we likely won't be able to recover from.
> > > > > > +         */
> > > > > > +        if (errno != ENOENT) {
> > > > > > +            g_critical("unable to access state file at path %s: %s",
> > > > > > +                       path, strerror(errno));
> > > > > > +            ret = false;
> > > > > > +            goto out;
> > > > > > +        }
> > > > > > +
> > > > > > +        /* file doesn't exist. initialize state to default values and
> > > > > > +         * attempt to save now. (we could wait till later when we have
> > > > > > +         * modified state we need to commit, but if there's a problem,
> > > > > > +         * such as a missing parent directory, we want to catch it now)
> > > > > > +         *
> > > > > > +         * there is a potential scenario where someone either managed to
> > > > > > +         * update the agent from a version that didn't use a key store
> > > > > > +         * while qemu-ga thought the filesystem was frozen, or
> > > > > > +         * deleted the key store prior to issuing a fsfreeze, prior
> > > > > > +         * to restarting the agent. in this case we go ahead and defer
> > > > > > +         * initial creation till we actually have modified state to
> > > > > > +         * write, otherwise fail to recover from freeze.
> > > > > > +         */
> > > > > > +        set_persistent_state_defaults(pstate);
> > > > > > +        if (!frozen) {
> > > > > > +            ret = write_persistent_state(pstate, path);
> > > > > > +            if (!ret) {
> > > > > > +                g_critical("unable to create state file at path %s", path);
> > > > > > +                ret = false;
> > > > > > +                goto out;
> > > > > > +            }
> > > > > > +        }
> > > > > > +        ret = true;
> > > > > > +        goto out;
> > > > > > +    }
> > > > > > +
> > > > > > +    keyfile = g_key_file_new();
> > > > > > +    g_key_file_load_from_file(keyfile, path, 0, &gerr);
> > > > > > +    if (gerr) {
> > > > > > +        g_critical("error loading persistent state from path: %s, %s",
> > > > > > +                   path, gerr->message);
> > > > > > +        ret = false;
> > > > > > +        goto out;
> > > > > > +    }
> > > > > > +
> > > > > > +    persistent_state_from_keyfile(pstate, keyfile);
> > > > > > +
> > > > > > +out:
> > > > > > +    if (keyfile) {
> > > > > > +        g_key_file_free(keyfile);
> > > > > > +    }
> > > > > > +    if (gerr) {
> > > > > > +        g_error_free(gerr);
> > > > > > +    }
> > > > > > +
> > > > > > +    return ret;
> > > > > > +}
> > > > > > +
> > > > > > +int64_t ga_get_fd_handle(GAState *s, Error **errp)
> > > > > > +{
> > > > > > +    int64_t handle;
> > > > > > +
> > > > > > +    g_assert(s->pstate_filepath);
> > > > > > +    /* we blacklist commands and avoid operations that potentially require
> > > > > > +     * writing to disk when we're in a frozen state. this includes opening
> > > > > > +     * new files, so we should never get here in that situation
> > > > > > +     */
> > > > > > +    g_assert(!ga_is_frozen(s));
> > > > > > +
> > > > > > +    handle = s->pstate.fd_counter++;
> > > > > > +    if (s->pstate.fd_counter < 0) {
> > > > > > +        s->pstate.fd_counter = 0;
> > > > > > +    }
> > > > > 
> > > > > Is this handling the overflow case? Can't fd 0 be in use already?
> > > > 
> > > > It could, but it's very unlikely that an overflow/counter reset would
> > > > result in issuing still-in-use handle, since guest-file-open would need
> > > > to be called 2^63 times in the meantime.
> > > 
> > > Agreed, but as you do check for that case and as the right fix is simple
> > > and I think it's worth it. I can send a patch myself.
> > > 
> > 
> > A patch to switch to tracking a list of issued handles in the keystore,
> > or to return an error on overflow?
> 
> To return an error on overflow. Minor, but if we do handle it let's do it
> right. Or, we could just add an assert like:
> 
> assert(s->pstate.fd_counter >= 0);

Ah, well, I'm not sure I understand then. You mean dropping:

    if (s->pstate.fd_counter < 0) {
        s->pstate.fd_counter = 0;
    }

And replacing it with an error or assertion?

If so, the overflow is actually expected: once we dish out handle MAX_INT64,
we should restart at 0. I initially made fd_counter a uint64_t so
overflow/reset would happen more naturally, but since we issue handles as
int64_t this would've caused other complications.

Something like this might be more clear about the intent though:

    handle = s->pstate.fd_counter;
    if (s->pstate.fd_counter == MAX_INT64) {
        s->pstate.fd_counter = 0;
    } else {
        s->pstate.fd_counter++;
    }

> 
> > If the former, sounds good, but keep in mind that we need to update on
> > every guest-file-open/guest-file-close, so we'll want to impose a
> > reasonable max on the number of outstanding FDs and document it. I would
> > lean toward something conservation like 1024, can consider bumping it in
> > the future if that proves insufficient.
> 
> Maybe in the near future :)
>

On Wed, 20 Mar 2013 13:14:21 -0500
mdroth <mdroth@linux.vnet.ibm.com> wrote:

> > > > > > > +    handle = s->pstate.fd_counter++;
> > > > > > > +    if (s->pstate.fd_counter < 0) {
> > > > > > > +        s->pstate.fd_counter = 0;
> > > > > > > +    }
> > > > > > 
> > > > > > Is this handling the overflow case? Can't fd 0 be in use already?
> > > > > 
> > > > > It could, but it's very unlikely that an overflow/counter reset would
> > > > > result in issuing still-in-use handle, since guest-file-open would need
> > > > > to be called 2^63 times in the meantime.
> > > > 
> > > > Agreed, but as you do check for that case and as the right fix is simple
> > > > and I think it's worth it. I can send a patch myself.
> > > > 
> > > 
> > > A patch to switch to tracking a list of issued handles in the keystore,
> > > or to return an error on overflow?
> > 
> > To return an error on overflow. Minor, but if we do handle it let's do it
> > right. Or, we could just add an assert like:
> > 
> > assert(s->pstate.fd_counter >= 0);
> 
> Ah, well, I'm not sure I understand then. You mean dropping:
> 
>     if (s->pstate.fd_counter < 0) {
>         s->pstate.fd_counter = 0;
>     }
> 
> And replacing it with an error or assertion?

Yes, because I had understood you meant that this is very unlikely to be
triggered because we'd need guest-file-open to be called 2^63. This is
what I agreed above, although thinking more about it there's also the
possibility of a malicious client doing this on purpose.

But now I see that what you really meant is that it's unlikely for fd 0
to be in use after 2^63 guest-file-open calls. Am I right? If yes, then
I disagree because there's no way to guarantee when a certain fd will be
in use or not, unless we allow fds to be returned.

> If so, the overflow is actually expected: once we dish out handle MAX_INT64,
> we should restart at 0. I initially made fd_counter a uint64_t so
> overflow/reset would happen more naturally, but since we issue handles as
> int64_t this would've caused other complications.
> 
> Something like this might be more clear about the intent though:
> 
>     handle = s->pstate.fd_counter;
>     if (s->pstate.fd_counter == MAX_INT64) {
>         s->pstate.fd_counter = 0;
>     } else {
>         s->pstate.fd_counter++;
>     }

I disagree about restarting to zero as I have explained above. You seem to
not like returning an error, is it because we'll make guest-file-open
useless after the limit is reached?

Let's review our options:

 1. When fd_count reaches MAX_INT64 we reset it to zero

    Pros: simple and guest-file-open always work
    Cons: fd 0 might be in use by a client

 2. When fd_count reaches MAX_INT64 we return an error

    Pros: simple and we fix 'cons' from item 1
    Cons: guest-file-open will have a usage count limit

 3. Allow fds to be returned by clients on guest-file-close and do 2 on top

    Pros: solve problems discussed in items 1 and 2
    Cons: not trivial and the usage limit problem from item 2 can still
          happen if the client ends up not calling guest-file-close
          (although I do think we'll reach the OS limit here)

Do you see other options? Am I overcomplicating?

On Wed, Mar 20, 2013 at 02:38:35PM -0400, Luiz Capitulino wrote:
> On Wed, 20 Mar 2013 13:14:21 -0500
> mdroth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > > > > > > > +    handle = s->pstate.fd_counter++;
> > > > > > > > +    if (s->pstate.fd_counter < 0) {
> > > > > > > > +        s->pstate.fd_counter = 0;
> > > > > > > > +    }
> > > > > > > 
> > > > > > > Is this handling the overflow case? Can't fd 0 be in use already?
> > > > > > 
> > > > > > It could, but it's very unlikely that an overflow/counter reset would
> > > > > > result in issuing still-in-use handle, since guest-file-open would need
> > > > > > to be called 2^63 times in the meantime.
> > > > > 
> > > > > Agreed, but as you do check for that case and as the right fix is simple
> > > > > and I think it's worth it. I can send a patch myself.
> > > > > 
> > > > 
> > > > A patch to switch to tracking a list of issued handles in the keystore,
> > > > or to return an error on overflow?
> > > 
> > > To return an error on overflow. Minor, but if we do handle it let's do it
> > > right. Or, we could just add an assert like:
> > > 
> > > assert(s->pstate.fd_counter >= 0);
> > 
> > Ah, well, I'm not sure I understand then. You mean dropping:
> > 
> >     if (s->pstate.fd_counter < 0) {
> >         s->pstate.fd_counter = 0;
> >     }
> > 
> > And replacing it with an error or assertion?
> 
> Yes, because I had understood you meant that this is very unlikely to be
> triggered because we'd need guest-file-open to be called 2^63. This is
> what I agreed above, although thinking more about it there's also the
> possibility of a malicious client doing this on purpose.
> 
> But now I see that what you really meant is that it's unlikely for fd 0
> to be in use after 2^63 guest-file-open calls. Am I right? If yes, then

Yup, that's the scenario I was referring to.

> I disagree because there's no way to guarantee when a certain fd will be
> in use or not, unless we allow fds to be returned.

Yes, it's a real scenario that can indeed occur under "normal" usage, but in
my head it's similar to assumptions we make about clocks not overflowing within
a timeframe that anyone cares about in terms of severity. To quantify it
more:

Given RPC latency there's really no way to run up the fd counter faster than
once per nanosecond (more like millisecond atm), so you'd have to keep a handle
open and constantly called guest-file-open for a period of 292 years
before a duplicate handle gets issued.

> 
> > If so, the overflow is actually expected: once we dish out handle MAX_INT64,
> > we should restart at 0. I initially made fd_counter a uint64_t so
> > overflow/reset would happen more naturally, but since we issue handles as
> > int64_t this would've caused other complications.
> > 
> > Something like this might be more clear about the intent though:
> > 
> >     handle = s->pstate.fd_counter;
> >     if (s->pstate.fd_counter == MAX_INT64) {
> >         s->pstate.fd_counter = 0;
> >     } else {
> >         s->pstate.fd_counter++;
> >     }
> 
> I disagree about restarting to zero as I have explained above. You seem to
> not like returning an error, is it because we'll make guest-file-open
> useless after the limit is reached?

Yes. But, on second thought, given the above I guess I don't mind returning an
error as a failsafe for now. Although I think it should be an assert()
with a FIXME:, since it really should be fixed before anyone ever manages
to trigger it.

> 
> Let's review our options:
> 
>  1. When fd_count reaches MAX_INT64 we reset it to zero
> 
>     Pros: simple and guest-file-open always work
>     Cons: fd 0 might be in use by a client
> 
>  2. When fd_count reaches MAX_INT64 we return an error
> 
>     Pros: simple and we fix 'cons' from item 1
>     Cons: guest-file-open will have a usage count limit
> 
>  3. Allow fds to be returned by clients on guest-file-close and do 2 on top
> 
>     Pros: solve problems discussed in items 1 and 2
>     Cons: not trivial and the usage limit problem from item 2 can still
>           happen if the client ends up not calling guest-file-close
>           (although I do think we'll reach the OS limit here)
> 
> Do you see other options? Am I overcomplicating?
> 

No, I think that about sums it up. Doing 3, paired with a limit on the
number of outstanding FDs as in 2 is the full solution. The only
complication that is that the higher the limit we impose, the more I/O
and look-up overhead will be incured for every
guest-file-open/guest-file-close, because those operations will become
O(fd_limit).

So if we do it we'll need to impose a reasonable limit like 16k outstanding
FDs at a time or something (maybe even that's too much, but I'm thinking an
extra 64k read/write with every fopen()/fclose() isn't that big a deal).

But to complicate things somewhat more:

This whole reason for this keystore thing is to patch up the existing
interfaces so they continue functioning without clients needing to
update. So if we're considering something that requires imposing a fairly
small limit on the number of outstanding handles, they'd need to update
their implementations eventually anyway to be correct.

So I wonder if, rather than pursuing option 3, we just introduce an
interface that does what we really want and returns handles as UUIDs,
then mark the existing interfaces as deprecated (and then remove them
within the next 300 years so our assert never gets hit :)

On Wed, 20 Mar 2013 14:39:55 -0500
mdroth <mdroth@linux.vnet.ibm.com> wrote:

> So I wonder if, rather than pursuing option 3, we just introduce an
> interface that does what we really want and returns handles as UUIDs,
> then mark the existing interfaces as deprecated (and then remove them
> within the next 300 years so our assert never gets hit :)

This seems reasonable to me. It should be an assert() if it should never
happen. If we feel like doing option 3, we introduce a new interface
that handles UUIDs instead.

On Wed, Mar 20, 2013 at 03:56:18PM -0400, Luiz Capitulino wrote:
> On Wed, 20 Mar 2013 14:39:55 -0500
> mdroth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > So I wonder if, rather than pursuing option 3, we just introduce an
> > interface that does what we really want and returns handles as UUIDs,
> > then mark the existing interfaces as deprecated (and then remove them
> > within the next 300 years so our assert never gets hit :)
> 
> This seems reasonable to me. It should be an assert() if it should never
> happen. If we feel like doing option 3, we introduce a new interface
> that handles UUIDs instead.
> 

Ok, I think it's a plan then. I have a partial implementation for
guest-file-* commands on windows, and a posix/w32 guest-exec-async
implementation that re-uses them, so maybe those should be introduced
around the new interfaces to avoid churn. Need to generate uuid handles
for guest-exec handles anyway.

mdroth <mdroth@linux.vnet.ibm.com> writes:

> On Wed, Mar 20, 2013 at 02:38:35PM -0400, Luiz Capitulino wrote:
>> On Wed, 20 Mar 2013 13:14:21 -0500
>> mdroth <mdroth@linux.vnet.ibm.com> wrote:
>> 
>> > > > > > > > +    handle = s->pstate.fd_counter++;
>> > > > > > > > +    if (s->pstate.fd_counter < 0) {
>> > > > > > > > +        s->pstate.fd_counter = 0;
>> > > > > > > > +    }
>> > > > > > > 
>> > > > > > > Is this handling the overflow case? Can't fd 0 be in use already?
>> > > > > > 
>> > > > > > It could, but it's very unlikely that an overflow/counter
>> > > > > > reset would
>> > > > > > result in issuing still-in-use handle, since
>> > > > > > guest-file-open would need
>> > > > > > to be called 2^63 times in the meantime.
>> > > > > 
>> > > > > Agreed, but as you do check for that case and as the right
>> > > > > fix is simple
>> > > > > and I think it's worth it. I can send a patch myself.
>> > > > > 
>> > > > 
>> > > > A patch to switch to tracking a list of issued handles in the keystore,
>> > > > or to return an error on overflow?
>> > > 
>> > > To return an error on overflow. Minor, but if we do handle it let's do it
>> > > right. Or, we could just add an assert like:
>> > > 
>> > > assert(s->pstate.fd_counter >= 0);
>> > 
>> > Ah, well, I'm not sure I understand then. You mean dropping:
>> > 
>> >     if (s->pstate.fd_counter < 0) {
>> >         s->pstate.fd_counter = 0;
>> >     }
>> > 
>> > And replacing it with an error or assertion?
>> 
>> Yes, because I had understood you meant that this is very unlikely to be
>> triggered because we'd need guest-file-open to be called 2^63. This is
>> what I agreed above, although thinking more about it there's also the
>> possibility of a malicious client doing this on purpose.
>> 
>> But now I see that what you really meant is that it's unlikely for fd 0
>> to be in use after 2^63 guest-file-open calls. Am I right? If yes, then
>
> Yup, that's the scenario I was referring to.
>
>> I disagree because there's no way to guarantee when a certain fd will be
>> in use or not, unless we allow fds to be returned.
>
> Yes, it's a real scenario that can indeed occur under "normal" usage, but in
> my head it's similar to assumptions we make about clocks not overflowing within
> a timeframe that anyone cares about in terms of severity. To quantify it
> more:
>
> Given RPC latency there's really no way to run up the fd counter faster than
> once per nanosecond (more like millisecond atm), so you'd have to keep a handle
> open and constantly called guest-file-open for a period of 292 years
> before a duplicate handle gets issued.

In other words, it's a complete non-issue :)

>> > If so, the overflow is actually expected: once we dish out handle MAX_INT64,
>> > we should restart at 0. I initially made fd_counter a uint64_t so
>> > overflow/reset would happen more naturally, but since we issue handles as
>> > int64_t this would've caused other complications.
>> > 
>> > Something like this might be more clear about the intent though:
>> > 
>> >     handle = s->pstate.fd_counter;
>> >     if (s->pstate.fd_counter == MAX_INT64) {
>> >         s->pstate.fd_counter = 0;
>> >     } else {
>> >         s->pstate.fd_counter++;
>> >     }
>> 
>> I disagree about restarting to zero as I have explained above. You seem to
>> not like returning an error, is it because we'll make guest-file-open
>> useless after the limit is reached?
>
> Yes. But, on second thought, given the above I guess I don't mind returning an
> error as a failsafe for now. Although I think it should be an assert()
> with a FIXME:, since it really should be fixed before anyone ever manages
> to trigger it.
>
>> 
>> Let's review our options:
>> 
>>  1. When fd_count reaches MAX_INT64 we reset it to zero
>> 
>>     Pros: simple and guest-file-open always work
>>     Cons: fd 0 might be in use by a client
>> 
>>  2. When fd_count reaches MAX_INT64 we return an error
>> 
>>     Pros: simple and we fix 'cons' from item 1
>>     Cons: guest-file-open will have a usage count limit

             error path unreachable in practice

Such errors require special hackery to test.  The test needs to include
the error's consumer.

Waste of time if you ask me.

>>  3. Allow fds to be returned by clients on guest-file-close and do 2 on top
>> 
>>     Pros: solve problems discussed in items 1 and 2
>>     Cons: not trivial and the usage limit problem from item 2 can still
>>           happen if the client ends up not calling guest-file-close
>>           (although I do think we'll reach the OS limit here)

The OS limit on file descriptors is many orders of magnitude lower than
2^63.

>> Do you see other options? Am I overcomplicating?
>> 
>
> No, I think that about sums it up. Doing 3, paired with a limit on the
> number of outstanding FDs as in 2 is the full solution. The only
> complication that is that the higher the limit we impose, the more I/O
> and look-up overhead will be incured for every
> guest-file-open/guest-file-close, because those operations will become
> O(fd_limit).

Cure much worse than the disease.

> So if we do it we'll need to impose a reasonable limit like 16k outstanding
> FDs at a time or something (maybe even that's too much, but I'm thinking an
> extra 64k read/write with every fopen()/fclose() isn't that big a deal).

I guess the limit should be in the order of the OS's limit on open fds.

Instead of a purely theoretical limit, we get a real limit.  Stupid.

> But to complicate things somewhat more:
>
> This whole reason for this keystore thing is to patch up the existing
> interfaces so they continue functioning without clients needing to
> update. So if we're considering something that requires imposing a fairly
> small limit on the number of outstanding handles, they'd need to update
> their implementations eventually anyway to be correct.
>
> So I wonder if, rather than pursuing option 3, we just introduce an
> interface that does what we really want and returns handles as UUIDs,
> then mark the existing interfaces as deprecated (and then remove them
> within the next 300 years so our assert never gets hit :)

Looks like you guys have no *practical* problems to solve.  Congrats!
Take a vacation!  Please report back no later than 275 years from now,
to make sure this 64 bit fd counter overflow problem gets taken care of
in time.  ;-P

On Thu, Mar 21, 2013 at 08:03:11AM +0100, Markus Armbruster wrote:
> mdroth <mdroth@linux.vnet.ibm.com> writes:
> 
> > On Wed, Mar 20, 2013 at 02:38:35PM -0400, Luiz Capitulino wrote:
> >> On Wed, 20 Mar 2013 13:14:21 -0500
> >> mdroth <mdroth@linux.vnet.ibm.com> wrote:
> >> 
> >> > > > > > > > +    handle = s->pstate.fd_counter++;
> >> > > > > > > > +    if (s->pstate.fd_counter < 0) {
> >> > > > > > > > +        s->pstate.fd_counter = 0;
> >> > > > > > > > +    }
> >> > > > > > > 
> >> > > > > > > Is this handling the overflow case? Can't fd 0 be in use already?
> >> > > > > > 
> >> > > > > > It could, but it's very unlikely that an overflow/counter
> >> > > > > > reset would
> >> > > > > > result in issuing still-in-use handle, since
> >> > > > > > guest-file-open would need
> >> > > > > > to be called 2^63 times in the meantime.
> >> > > > > 
> >> > > > > Agreed, but as you do check for that case and as the right
> >> > > > > fix is simple
> >> > > > > and I think it's worth it. I can send a patch myself.
> >> > > > > 
> >> > > > 
> >> > > > A patch to switch to tracking a list of issued handles in the keystore,
> >> > > > or to return an error on overflow?
> >> > > 
> >> > > To return an error on overflow. Minor, but if we do handle it let's do it
> >> > > right. Or, we could just add an assert like:
> >> > > 
> >> > > assert(s->pstate.fd_counter >= 0);
> >> > 
> >> > Ah, well, I'm not sure I understand then. You mean dropping:
> >> > 
> >> >     if (s->pstate.fd_counter < 0) {
> >> >         s->pstate.fd_counter = 0;
> >> >     }
> >> > 
> >> > And replacing it with an error or assertion?
> >> 
> >> Yes, because I had understood you meant that this is very unlikely to be
> >> triggered because we'd need guest-file-open to be called 2^63. This is
> >> what I agreed above, although thinking more about it there's also the
> >> possibility of a malicious client doing this on purpose.
> >> 
> >> But now I see that what you really meant is that it's unlikely for fd 0
> >> to be in use after 2^63 guest-file-open calls. Am I right? If yes, then
> >
> > Yup, that's the scenario I was referring to.
> >
> >> I disagree because there's no way to guarantee when a certain fd will be
> >> in use or not, unless we allow fds to be returned.
> >
> > Yes, it's a real scenario that can indeed occur under "normal" usage, but in
> > my head it's similar to assumptions we make about clocks not overflowing within
> > a timeframe that anyone cares about in terms of severity. To quantify it
> > more:
> >
> > Given RPC latency there's really no way to run up the fd counter faster than
> > once per nanosecond (more like millisecond atm), so you'd have to keep a handle
> > open and constantly called guest-file-open for a period of 292 years
> > before a duplicate handle gets issued.
> 
> In other words, it's a complete non-issue :)
> 
> >> > If so, the overflow is actually expected: once we dish out handle MAX_INT64,
> >> > we should restart at 0. I initially made fd_counter a uint64_t so
> >> > overflow/reset would happen more naturally, but since we issue handles as
> >> > int64_t this would've caused other complications.
> >> > 
> >> > Something like this might be more clear about the intent though:
> >> > 
> >> >     handle = s->pstate.fd_counter;
> >> >     if (s->pstate.fd_counter == MAX_INT64) {
> >> >         s->pstate.fd_counter = 0;
> >> >     } else {
> >> >         s->pstate.fd_counter++;
> >> >     }
> >> 
> >> I disagree about restarting to zero as I have explained above. You seem to
> >> not like returning an error, is it because we'll make guest-file-open
> >> useless after the limit is reached?
> >
> > Yes. But, on second thought, given the above I guess I don't mind returning an
> > error as a failsafe for now. Although I think it should be an assert()
> > with a FIXME:, since it really should be fixed before anyone ever manages
> > to trigger it.
> >
> >> 
> >> Let's review our options:
> >> 
> >>  1. When fd_count reaches MAX_INT64 we reset it to zero
> >> 
> >>     Pros: simple and guest-file-open always work
> >>     Cons: fd 0 might be in use by a client
> >> 
> >>  2. When fd_count reaches MAX_INT64 we return an error
> >> 
> >>     Pros: simple and we fix 'cons' from item 1
> >>     Cons: guest-file-open will have a usage count limit
> 
>              error path unreachable in practice
> 
> Such errors require special hackery to test.  The test needs to include
> the error's consumer.
> 
> Waste of time if you ask me.
> 
> >>  3. Allow fds to be returned by clients on guest-file-close and do 2 on top
> >> 
> >>     Pros: solve problems discussed in items 1 and 2
> >>     Cons: not trivial and the usage limit problem from item 2 can still
> >>           happen if the client ends up not calling guest-file-close
> >>           (although I do think we'll reach the OS limit here)
> 
> The OS limit on file descriptors is many orders of magnitude lower than
> 2^63.
> 
> >> Do you see other options? Am I overcomplicating?
> >> 
> >
> > No, I think that about sums it up. Doing 3, paired with a limit on the
> > number of outstanding FDs as in 2 is the full solution. The only
> > complication that is that the higher the limit we impose, the more I/O
> > and look-up overhead will be incured for every
> > guest-file-open/guest-file-close, because those operations will become
> > O(fd_limit).
> 
> Cure much worse than the disease.
> 
> > So if we do it we'll need to impose a reasonable limit like 16k outstanding
> > FDs at a time or something (maybe even that's too much, but I'm thinking an
> > extra 64k read/write with every fopen()/fclose() isn't that big a deal).
> 
> I guess the limit should be in the order of the OS's limit on open fds.
> 
> Instead of a purely theoretical limit, we get a real limit.  Stupid.
> 
> > But to complicate things somewhat more:
> >
> > This whole reason for this keystore thing is to patch up the existing
> > interfaces so they continue functioning without clients needing to
> > update. So if we're considering something that requires imposing a fairly
> > small limit on the number of outstanding handles, they'd need to update
> > their implementations eventually anyway to be correct.
> >
> > So I wonder if, rather than pursuing option 3, we just introduce an
> > interface that does what we really want and returns handles as UUIDs,
> > then mark the existing interfaces as deprecated (and then remove them
> > within the next 300 years so our assert never gets hit :)
> 
> Looks like you guys have no *practical* problems to solve.  Congrats!
> Take a vacation!  Please report back no later than 275 years from now,
> to make sure this 64 bit fd counter overflow problem gets taken care of
> in time.  ;-P
> 

Haha, well, I didn't want to be that one lazy developer who brings about
the downfall of future human civilization... but if it's a really big
deal they'll probably send someone back from the future to let me know,
so maybe I'm jumping the gun a bit :)

I just didn't want to introduce a new interface that relied on
interfaces that were planned for deprecation in the *long*-term, but i
think you're right, it's too much hassle for current users for too
little gain, and there's plenty of time to do it in the future so I'll
hold off on it for now.

On Thu, 21 Mar 2013 11:13:13 -0500
mdroth <mdroth@linux.vnet.ibm.com> wrote:

> > Looks like you guys have no *practical* problems to solve.  Congrats!
> > Take a vacation!  Please report back no later than 275 years from now,
> > to make sure this 64 bit fd counter overflow problem gets taken care of
> > in time.  ;-P
> > 
> 
> Haha, well, I didn't want to be that one lazy developer who brings about
> the downfall of future human civilization... but if it's a really big
> deal they'll probably send someone back from the future to let me know,
> so maybe I'm jumping the gun a bit :)

I *am* that guy, but I was afraid to tell :)

> I just didn't want to introduce a new interface that relied on
> interfaces that were planned for deprecation in the *long*-term, but i
> think you're right, it's too much hassle for current users for too
> little gain, and there's plenty of time to do it in the future so I'll
> hold off on it for now.

Let me clarify it: when I read the code I didn't realize fd_counter
would never wrap. I think this discussion is settled now. However, I
still think that having an assert there is good practice.

I can post a patch myself.

On Thu, Mar 21, 2013 at 02:24:56PM -0400, Luiz Capitulino wrote:
> On Thu, 21 Mar 2013 11:13:13 -0500
> mdroth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > > Looks like you guys have no *practical* problems to solve.  Congrats!
> > > Take a vacation!  Please report back no later than 275 years from now,
> > > to make sure this 64 bit fd counter overflow problem gets taken care of
> > > in time.  ;-P
> > > 
> > 
> > Haha, well, I didn't want to be that one lazy developer who brings about
> > the downfall of future human civilization... but if it's a really big
> > deal they'll probably send someone back from the future to let me know,
> > so maybe I'm jumping the gun a bit :)
> 
> I *am* that guy, but I was afraid to tell :)
> 
> > I just didn't want to introduce a new interface that relied on
> > interfaces that were planned for deprecation in the *long*-term, but i
> > think you're right, it's too much hassle for current users for too
> > little gain, and there's plenty of time to do it in the future so I'll
> > hold off on it for now.
> 
> Let me clarify it: when I read the code I didn't realize fd_counter
> would never wrap. I think this discussion is settled now. However, I
> still think that having an assert there is good practice.
> 
> I can post a patch myself.
> 

Sounds good :)

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 21 Mar 2013 11:13:13 -0500
> mdroth <mdroth@linux.vnet.ibm.com> wrote:
>
>> > Looks like you guys have no *practical* problems to solve.  Congrats!
>> > Take a vacation!  Please report back no later than 275 years from now,
>> > to make sure this 64 bit fd counter overflow problem gets taken care of
>> > in time.  ;-P
>> > 
>> 
>> Haha, well, I didn't want to be that one lazy developer who brings about
>> the downfall of future human civilization... but if it's a really big
>> deal they'll probably send someone back from the future to let me know,
>> so maybe I'm jumping the gun a bit :)
>
> I *am* that guy, but I was afraid to tell :)
>
>> I just didn't want to introduce a new interface that relied on
>> interfaces that were planned for deprecation in the *long*-term, but i
>> think you're right, it's too much hassle for current users for too
>> little gain, and there's plenty of time to do it in the future so I'll
>> hold off on it for now.
>
> Let me clarify it: when I read the code I didn't realize fd_counter
> would never wrap. I think this discussion is settled now. However, I
> still think that having an assert there is good practice.
>
> I can post a patch myself.

Asserting the counter doesn't wrap makes sense.