[Libguestfs] [PATCH] lib: Add optional 'append' parameter to copy-(device|file)-to-file APIs.

[Richard W.M. Jones]

On Tue, Jun 23, 2015 at 07:40:57PM +0200, Pino Toscano wrote:
> In data martedì 23 giugno 2015 13:25:28, Richard W.M. Jones ha scritto:
> > This allows you to append one file to another:
> > 
> >   copy-file-to-file /input.txt /output.txt append:true
> > 
> > will append the contents of /input.txt to /output.txt.
> > ---
> >  daemon/copy.c        | 38 +++++++++++++++++++++++++++++++-------
> >  generator/actions.ml | 29 +++++++++++++++++++++--------
> >  2 files changed, 52 insertions(+), 15 deletions(-)
> > 
> > diff --git a/daemon/copy.c b/daemon/copy.c
> > index bab00fe..6f3e844 100644
> > --- a/daemon/copy.c
> > +++ b/daemon/copy.c
> > @@ -30,7 +30,6 @@
> >  #include "actions.h"
> >  
> >  /* wrflags */
> > -#define DEST_FILE_FLAGS O_WRONLY|O_CREAT|O_TRUNC|O_NOCTTY|O_CLOEXEC, 0666
> >  #define DEST_DEVICE_FLAGS O_WRONLY|O_CLOEXEC, 0
> >  
> >  /* flags */
> > @@ -210,8 +209,13 @@ copy (const char *src, const char *src_display,
> >  int
> >  do_copy_device_to_device (const char *src, const char *dest,
> >                            int64_t srcoffset, int64_t destoffset, int64_t size,
> > -                          int sparse)
> > +                          int sparse, int append)
> >  {
> > +  if ((optargs_bitmask & GUESTFS_COPY_DEVICE_TO_DEVICE_APPEND_BITMASK) &&
> > +      append) {
> > +    reply_with_error ("the append flag cannot be set for this call");
> > +    return -1;
> > +  }
> >    return copy (src, src, dest, dest, DEST_DEVICE_FLAGS, 0,
> >                 srcoffset, destoffset, size, sparse);
> >  }
> > @@ -219,23 +223,30 @@ do_copy_device_to_device (const char *src, const char *dest,
> >  int
> >  do_copy_device_to_file (const char *src, const char *dest,
> >                          int64_t srcoffset, int64_t destoffset, int64_t size,
> > -                        int sparse)
> > +                        int sparse, int append)
> >  {
> >    CLEANUP_FREE char *dest_buf = sysroot_path (dest);
> > +  int wrflags = O_WRONLY|O_CREAT|O_NOCTTY|O_CLOEXEC;
> 
> Cannot DEST_FILE_FLAGS (without O_TRUNC, of course) be used here (and
> below) to avoid copying&pasting these attributes?

That was my original version.  I just questioned whether the code was
really any easier to read versus defining the set of O_ flags that
we're all pretty familiar with.

> >      tests = [
> >        InitScratchFS, Always, TestResult (
> >          [["mkdir"; "/copyff"];
> >           ["write"; "/copyff/src"; "hello, world"];
> > -         ["copy_file_to_file"; "/copyff/src"; "/copyff/dest"; ""; ""; ""; ""];
> > +         ["copy_file_to_file"; "/copyff/src"; "/copyff/dest"; ""; ""; ""; ""; "false"];
> >           ["read_file"; "/copyff/dest"]],
> >          "compare_buffers (ret, size, \"hello, world\", 12) == 0"), [];
> > -      let size = 1024 * 1024 in
> >        InitScratchFS, Always, TestResultTrue (
> > +        let size = 1024 * 1024 in
> 
> This seems unused? Or is it supposed to be used and its usage has been
> forgotten?
> 
> >          [["mkdir"; "/copyff2"];
> >           ["fill"; "0"; string_of_int size; "/copyff2/src"];
> >           ["touch"; "/copyff2/dest"];
> >           ["truncate_size"; "/copyff2/dest"; string_of_int size];

It's used here ^^

[...]

> It would seem okay for me, although maybe we should decouple the enums
> for flags, so there's no need to add non-functional append flags to the
> *_to_device variants.

This is an assumption of the current code, if you look at:

https://github.com/libguestfs/libguestfs/blob/master/daemon/copy.c#L39

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v