Re: [Libguestfs] [PATCH] builder, edit, fish: use copy-attributes

Richard W.M. Jones
2014-01-14T12:07:50Z

On Tue, Jan 14, 2014 at 11:16:40AM +0100, Pino Toscano
wrote:
> Make use of the new copy-attributes command to properly copy all file
> attributes from a file to the new version of it.
> ---
>  builder/perl_edit.ml | 29 +---------------------------
>  edit/edit.c          | 49 ++---------------------------------------------
>  fish/edit.c          | 54
++--------------------------------------------------
>  3 files changed, 5 insertions(+), 127 deletions(-)
>
> diff --git a/builder/perl_edit.ml b/builder/perl_edit.ml
> index aa4c2e6..28e5dea 100644
> --- a/builder/perl_edit.ml
> +++ b/builder/perl_edit.ml
> @@ -42,7 +42,7 @@ let rec edit_file ~debug (g : Guestfs.guestfs) file expr
>    g#upload tmpfile file;
>
>    (* However like virt-edit we do need to copy attributes. *)
> -  copy_attributes g file_old file;
> +  g#copy_attributes ~all:true file_old file;
>    g#rm file_old
>
>  and do_perl_edit ~debug g file expr > @@ -76,30 +76,3 @@ and
do_perl_edit ~debug g file expr >    );
>
>    Unix.rename (file ^ ".out") file
> -
> -and copy_attributes g src dest > -  let has_linuxxattrs =
g#feature_available [|"linuxxattrs"|] in
> -
> -  (* Get the mode. *)
> -  let stat = g#stat src in
> -
> -  (* Get the SELinux context.  XXX Should we copy over other extended
> -   * attributes too?
> -   *)
> -  let selinux_context > -    if has_linuxxattrs then (
> -      try Some (g#getxattr src "security.selinux") with _ ->
None
> -    ) else None in
> -
> -  (* Set the permissions (inc. sticky and set*id bits), UID, GID. *)
> -  let mode = Int64.to_int stat.G.mode
> -  and uid = Int64.to_int stat.G.uid and gid = Int64.to_int stat.G.gid in
> -  g#chmod (mode land 0o7777) dest;
> -  g#chown uid gid dest;
> -
> -  (* Set the SELinux context. *)
> -  match selinux_context with
> -  | None -> ()
> -  | Some selinux_context ->
> -    g#setxattr "security.selinux"
> -      selinux_context (String.length selinux_context) dest
> diff --git a/edit/edit.c b/edit/edit.c
> index e5e3a29..07790be 100644
> --- a/edit/edit.c
> +++ b/edit/edit.c
> @@ -56,7 +56,6 @@ static void edit_files (int argc, char *argv[]);
>  static void edit (const char *filename, const char *root);
>  static char *edit_interactively (const char *tmpfile);
>  static char *edit_non_interactively (const char *tmpfile);
> -static int copy_attributes (const char *src, const char *dest);
>  static int is_windows (guestfs_h *g, const char *root);
>  static char *windows_path (guestfs_h *g, const char *root, const char
*filename);
>  static char *generate_random_name (const char *filename);
> @@ -361,7 +360,8 @@ edit (const char *filename, const char *root)
>      /* Set the permissions, UID, GID and SELinux context of the new
>       * file to match the old file (RHBZ#788641).
>       */
> -    if (copy_attributes (filename, newname) == -1)
> +    if (guestfs_copy_attributes (g, filename, newname,
> +        GUESTFS_COPY_ATTRIBUTES_ALL, 1, -1) == -1)
>        goto error;
>
>      /* Backup or overwrite the file. */
> @@ -510,51 +510,6 @@ edit_non_interactively (const char *tmpfile)
>  }
>
>  static int
> -copy_attributes (const char *src, const char *dest)
> -{
> -  CLEANUP_FREE_STAT struct guestfs_stat *stat = NULL;
> -  const char *linuxxattrs[] = { "linuxxattrs", NULL };
> -  int has_linuxxattrs;
> -  CLEANUP_FREE char *selinux_context = NULL;
> -  size_t selinux_context_size;
> -
> -  has_linuxxattrs = guestfs_feature_available (g, (char **) linuxxattrs);
> -
> -  /* Get the mode. */
> -  stat = guestfs_stat (g, src);
> -  if (stat == NULL)
> -    return -1;
> -
> -  /* Get the SELinux context.  XXX Should we copy over other extended
> -   * attributes too?
> -   */
> -  if (has_linuxxattrs) {
> -    guestfs_push_error_handler (g, NULL, NULL);
> -
> -    selinux_context = guestfs_getxattr (g, src,
"security.selinux",
> -                                        &selinux_context_size);
> -    /* selinux_context could be NULL.  This isn't an error. */
> -
> -    guestfs_pop_error_handler (g);
> -  }
> -
> -  /* Set the permissions (inc. sticky and set*id bits), UID, GID. */
> -  if (guestfs_chmod (g, stat->mode & 07777, dest) == -1)
> -    return -1;
> -  if (guestfs_chown (g, stat->uid, stat->gid, dest) == -1)
> -    return -1;
> -
> -  /* Set the SELinux context. */
> -  if (has_linuxxattrs && selinux_context) {
> -    if (guestfs_setxattr (g, "security.selinux",
selinux_context,
> -                          (int) selinux_context_size, dest) == -1)
> -      return -1;
> -  }
> -
> -  return 0;
> -}
> -
> -static int
>  is_windows (guestfs_h *g, const char *root)
>  {
>    int w;
> diff --git a/fish/edit.c b/fish/edit.c
> index 754a34a..bd02f4b 100644
> --- a/fish/edit.c
> +++ b/fish/edit.c
> @@ -32,7 +32,6 @@
>  #include "fish.h"
>
>  static char *generate_random_name (const char *filename);
> -static int copy_attributes (const char *src, const char *dest);
>
>  /* guestfish edit command, suggested by Ján Ondrej, implemented by RWMJ */
>
> @@ -135,7 +134,8 @@ run_edit (const char *cmd, size_t argc, char *argv[])
>    /* Set the permissions, UID, GID and SELinux context of the new
>     * file to match the old file (RHBZ#788641).
>     */
> -  if (copy_attributes (remotefilename, newname) == -1)
> +  if (guestfs_copy_attributes (g, remotefilename, newname,
> +      GUESTFS_COPY_ATTRIBUTES_ALL, 1, -1) == -1)
>      return -1;
>
>    if (guestfs_mv (g, newname, remotefilename) == -1)
> @@ -177,53 +177,3 @@ generate_random_name (const char *filename)
>
>    return ret; /* caller will free */
>  }
> -
> -static int
> -copy_attributes (const char *src, const char *dest)
> -{
> -  struct guestfs_stat *stat;
> -  const char *linuxxattrs[] = { "linuxxattrs", NULL };
> -  int has_linuxxattrs;
> -  CLEANUP_FREE char *selinux_context = NULL;
> -  size_t selinux_context_size;
> -
> -  has_linuxxattrs = guestfs_feature_available (g, (char **) linuxxattrs);
> -
> -  /* Get the mode. */
> -  stat = guestfs_stat (g, src);
> -  if (stat == NULL)
> -    return -1;
> -
> -  /* Get the SELinux context.  XXX Should we copy over other extended
> -   * attributes too?
> -   */
> -  if (has_linuxxattrs) {
> -    guestfs_push_error_handler (g, NULL, NULL);
> -
> -    selinux_context = guestfs_getxattr (g, src,
"security.selinux",
> -                                        &selinux_context_size);
> -    /* selinux_context could be NULL.  This isn't an error. */
> -
> -    guestfs_pop_error_handler (g);
> -  }
> -
> -  /* Set the permissions (inc. sticky and set*id bits), UID, GID. */
> -  if (guestfs_chmod (g, stat->mode & 07777, dest) == -1) {
> -    guestfs_free_stat (stat);
> -    return -1;
> -  }
> -  if (guestfs_chown (g, stat->uid, stat->gid, dest) == -1) {
> -    guestfs_free_stat (stat);
> -    return -1;
> -  }
> -  guestfs_free_stat (stat);
> -
> -  /* Set the SELinux context. */
> -  if (has_linuxxattrs && selinux_context) {
> -    if (guestfs_setxattr (g, "security.selinux",
selinux_context,
> -                          (int) selinux_context_size, dest) == -1)
> -      return -1;
> -  }
> -
> -  return 0;
> -}
> --
> 1.8.3.1

Looks good, ACK.

Rich.

--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)