[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [PATCH V2 1/2] NEW API:ext:mke2fs



On Mon, Sep 03, 2012 at 01:50:58PM +0800, Wanlong Gao wrote:
> New api mke2fs for full configuration of filesystem.
> 
> Signed-off-by: Wanlong Gao <gaowanlong cn fujitsu com>
> ---
>  daemon/ext2.c        | 452 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  generator/actions.ml |  18 ++
>  gobject/Makefile.inc |   6 +-
>  src/MAX_PROC_NR      |   2 +-
>  4 files changed, 475 insertions(+), 3 deletions(-)
> 
> diff --git a/daemon/ext2.c b/daemon/ext2.c
> index 40b36d2..7108547 100644
> --- a/daemon/ext2.c
> +++ b/daemon/ext2.c
> @@ -811,3 +811,455 @@ do_set_e2generation (const char *filename, int64_t generation)
>  
>    return 0;
>  }
> +
> +int
> +do_mke2fs (const char *device,
> +           int checkbadblock, const char *badblockfile,
> +           int64_t blocksize, int directwrite,
> +           int64_t fragsize, int forcecreate,
> +           int64_t blockspergroup, int64_t numberofgroups,
> +           int64_t bytesperinode, int64_t inodesize,
> +           int withjournal, int64_t journalsize,
> +           const char *journaldevice, const char *newvolumelabel,
> +           int reservedblockspercentage, const char *lastmounteddir,
> +           int64_t numberofinodes, const char *creatoros,
> +           int writesbandgrouponly,
> +           const char *fstype, const char *usagetype,
> +           const char *fsuuid, int mmpupdateinterval,
> +           int64_t stridesize, int64_t stripewidth,
> +           int64_t maxonlineresize, int lazyitableinit,
> +           int lazyjournalinit, int testfs,
> +           int discard, int quotatype,
> +           int dirindex, int extent, int filetype,
> +           int flexbg, int hasjournal, int journaldev,
> +           int largefile, int quota, int resizeinode,
> +           int sparsesuper, int uninitbg,
> +           int64_t blockscount)
> +{
> +  int r;
> +  char *err = NULL;
> +  const char *argv[MAX_ARGS];

I bet there is a scenario where we exceed MAX_ARGS (64?) ...

> +  char blocksize_s[64];
> +  char fragsize_s[64];
> +  char blockspergroup_s[64];
> +  char numberofgroups_s[64];
> +  char bytesperinode_s[64];
> +  char inodesize_s[64];
> +  char journalsize_s[64];
> +  char journaldevice_s[256];
> +  char reservedblockspercentage_s[64];
> +  char numberofinodes_s[64];
> +  char mmpupdateinterval_s[84];
> +  char stridesize_s[74];
> +  char stripewidth_s[84];
> +  char maxonlineresize_s[74];
> +  char blockscount_s[64];
> +  size_t i = 0;
> +  int feature = 0;
> +  char features[256];

Does mke2fs let you use multiple -O options?  If so, we don't need
this fixed size "features" array, or the horrible and possibly unsafe
use of strncat later on.

> +  ADD_ARG (argv, i, str_mke2fs);
> +
> +  if (optargs_bitmask & GUESTFS_MKE2FS_CHECKBADBLOCK_BITMASK) {
> +    if (checkbadblock)
> +      ADD_ARG (argv, i, "-c");
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_BADBLOCKFILE_BITMASK) {
> +    if (badblockfile) {
> +      ADD_ARG (argv, i, "-l");
> +      ADD_ARG (argv, i, badblockfile);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_BLOCKSIZE_BITMASK) {
> +    if (blocksize < 0) {
> +      reply_with_error ("blocksize must be >= 0");
> +      goto error;
> +    }
> +    snprintf (blocksize_s, sizeof blocksize_s, "%" PRIi64, blocksize);
> +    ADD_ARG (argv, i, "-b");
> +    ADD_ARG (argv, i, blocksize_s);
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_DIRECTWRITE_BITMASK) {
> +    if (directwrite)
> +      ADD_ARG (argv, i, "-D");
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_FRAGSIZE_BITMASK) {
> +    if (fragsize < 0) {
> +      reply_with_error ("fragsize must be >= 0");
> +      goto error;
> +    }
> +    snprintf (fragsize_s, sizeof fragsize_s, "%" PRIi64, fragsize);
> +    ADD_ARG (argv, i, "-f");
> +    ADD_ARG (argv, i, fragsize_s);
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_FORCECREATE_BITMASK) {
> +    if (forcecreate)
> +      ADD_ARG (argv, i, "-F");
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_BLOCKSPERGROUP_BITMASK) {
> +    if (blockspergroup < 0) {
> +      reply_with_error ("blockspergroup must be >= 0");
> +      goto error;
> +    }
> +    snprintf (blockspergroup_s, sizeof blockspergroup_s,
> +              "%" PRIi64, blockspergroup);
> +    ADD_ARG (argv, i, "-g");
> +    ADD_ARG (argv, i, blockspergroup_s);
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_NUMBEROFGROUPS_BITMASK) {
> +    if (numberofgroups < 0) {
> +      reply_with_error ("numberofgroups must be >= 0");
> +      goto error;
> +    }
> +    snprintf (numberofgroups_s, sizeof numberofgroups_s,
> +              "%" PRIi64, numberofgroups);
> +    ADD_ARG (argv, i, "-G");
> +    ADD_ARG (argv, i, numberofgroups_s);
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_BYTESPERINODE_BITMASK) {
> +    if (bytesperinode < 0) {
> +      reply_with_error ("bytesperinode must be >= 0");
> +      goto error;
> +    }
> +    snprintf (bytesperinode_s, sizeof bytesperinode_s, "%" PRIi64, bytesperinode);
> +    ADD_ARG (argv, i, "-i");
> +    ADD_ARG (argv, i, bytesperinode_s);
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_INODESIZE_BITMASK) {
> +    if (inodesize < 0) {
> +      reply_with_error ("inodesize must be >= 0");
> +      goto error;
> +    }
> +    snprintf (inodesize_s, sizeof inodesize_s, "%" PRIi64, inodesize);
> +    ADD_ARG (argv, i, "-I");
> +    ADD_ARG (argv, i, inodesize_s);
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_WITHJOURNAL_BITMASK) {
> +    if (withjournal)
> +      ADD_ARG (argv, i, "-j");
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_JOURNALSIZE_BITMASK) {
> +    if (journalsize < 0) {
> +      reply_with_error ("journalsize must be >= 0");
> +      goto error;
> +    }
> +    snprintf (journalsize_s, sizeof journalsize_s,
> +              "size=" "%" PRIi64, journalsize);
> +    ADD_ARG (argv, i, "-J");
> +    ADD_ARG (argv, i, journalsize_s);
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_JOURNALDEVICE_BITMASK) {
> +    if (journaldevice) {
> +      snprintf (journaldevice_s, sizeof journaldevice_s,
> +                "device=%s", journaldevice);
> +      ADD_ARG (argv, i, "-J");
> +      ADD_ARG (argv, i, journaldevice_s);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_NEWVOLUMELABEL_BITMASK) {
> +    if (newvolumelabel) {
> +      ADD_ARG (argv, i, "-L");
> +      ADD_ARG (argv, i, newvolumelabel);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_RESERVEDBLOCKSPERCENTAGE_BITMASK) {
> +    if (reservedblockspercentage < 0) {
> +      reply_with_error ("reservedblockspercentage must be >= 0");
> +      goto error;
> +    }
> +    snprintf (reservedblockspercentage_s, sizeof reservedblockspercentage_s,
> +              "%" PRIi32, reservedblockspercentage);
> +    ADD_ARG (argv, i, "-m");
> +    ADD_ARG (argv, i, reservedblockspercentage_s);
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_LASTMOUNTEDDIR_BITMASK) {
> +    if (lastmounteddir) {
> +      ADD_ARG (argv, i, "-M");
> +      ADD_ARG (argv, i, lastmounteddir);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_NUMBEROFINODES_BITMASK) {
> +    if (numberofinodes < 0) {
> +      reply_with_error ("numberofinodes must be >= 0");
> +      goto error;
> +    }
> +    snprintf (numberofinodes_s, sizeof numberofinodes_s,
> +              "%" PRIi64, numberofinodes);
> +    ADD_ARG (argv, i, "-N");
> +    ADD_ARG (argv, i, numberofinodes_s);
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_CREATOROS_BITMASK) {
> +    if (creatoros) {
> +      ADD_ARG (argv, i, "-o");
> +      ADD_ARG (argv, i, creatoros);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_WRITESBANDGROUPONLY_BITMASK) {
> +    if (writesbandgrouponly)
> +      ADD_ARG (argv, i, "-S");
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_FSTYPE_BITMASK) {
> +    if (fstype) {
> +      ADD_ARG (argv, i, "-t");
> +      ADD_ARG (argv, i, fstype);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_USAGETYPE_BITMASK) {
> +    if (usagetype) {
> +      ADD_ARG (argv, i, "-T");
> +      ADD_ARG (argv, i, usagetype);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_FSUUID_BITMASK) {
> +    if (fsuuid) {
> +      ADD_ARG (argv, i, "-U");
> +      ADD_ARG (argv, i, fsuuid);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_MMPUPDATEINTERVAL_BITMASK) {
> +    if (mmpupdateinterval < 0) {
> +      reply_with_error ("mmpupdateinterval must be >= 0");
> +      goto error;
> +    }
> +    snprintf (mmpupdateinterval_s, sizeof mmpupdateinterval_s,
> +              "mmp_update_interval=" "%" PRIi32, mmpupdateinterval);
> +    ADD_ARG (argv, i, "-E");
> +    ADD_ARG (argv, i, mmpupdateinterval_s);
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_STRIDESIZE_BITMASK) {
> +    if (stridesize < 0) {
> +      reply_with_error ("stridesize must be >= 0");
> +      goto error;
> +    }
> +    snprintf (stridesize_s, sizeof stridesize_s,
> +              "stride=" "%" PRIi64, stridesize);
> +    ADD_ARG (argv, i, "-E");
> +    ADD_ARG (argv, i, stridesize_s);
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_STRIPEWIDTH_BITMASK) {
> +    if (stripewidth< 0) {
> +      reply_with_error ("stripewidth must be >= 0");
> +      goto error;
> +    }
> +    snprintf (stripewidth_s, sizeof stripewidth_s,
> +              "stripe_width=" "%" PRIi64, stripewidth);
> +    ADD_ARG (argv, i, "-E");
> +    ADD_ARG (argv, i, stripewidth_s);
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_MAXONLINERESIZE_BITMASK) {
> +    if (maxonlineresize < 0) {
> +      reply_with_error ("maxonlineresize must be >= 0");
> +      goto error;
> +    }
> +    snprintf (maxonlineresize_s, sizeof maxonlineresize_s,
> +              "resize=" "%" PRIi64, maxonlineresize);
> +    ADD_ARG (argv, i, "-E");
> +    ADD_ARG (argv, i, maxonlineresize_s);
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_LAZYITABLEINIT_BITMASK) {
> +    ADD_ARG (argv, i, "-E");
> +    if (lazyitableinit)
> +      ADD_ARG (argv, i, "lazy_itable_init=1");
> +    else
> +      ADD_ARG (argv, i, "lazy_itable_init=0");
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_LAZYJOURNALINIT_BITMASK) {
> +    ADD_ARG (argv, i, "-E");
> +    if (lazyjournalinit)
> +      ADD_ARG (argv, i, "lazy_journal_init=1");
> +    else
> +      ADD_ARG (argv, i, "lazy_journal_init=0");
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_TESTFS_BITMASK) {
> +    if (testfs) {
> +      ADD_ARG (argv, i, "-E");
> +      ADD_ARG (argv, i, "test_fs");
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_DISCARD_BITMASK) {
> +    ADD_ARG (argv, i, "-E");
> +    if (discard)
> +      ADD_ARG (argv, i, "discard");
> +    else
> +      ADD_ARG (argv, i, "nodiscard");
> +  }
> +
> +  if (optargs_bitmask & GUESTFS_MKE2FS_DIRINDEX_BITMASK) {
> +    if (dirindex)
> +      strncat (features, "dir_index", 9);
> +    else
> +      strncat (features, "^dir_index", 10);
> +    feature++;

Is there a reason to use ^dir_index?  Surely it can just
be omitted?

> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_EXTENT_BITMASK) {
> +    if (feature == 0) {
> +      if (extent)
> +        strncat (features, "extent", 6);
> +      else
> +        strncat (features, "^extent", 7);
> +      feature++;
> +    } else {
> +      if (extent)
> +        strncat (features, ",extent", 7);
> +      else
> +        strncat (features, ",^extent", 8);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_FILETYPE_BITMASK) {
> +    if (feature == 0) {
> +      if (filetype)
> +        strncat (features, "filetype", 8);
> +      else
> +        strncat (features, "^filetype", 9);
> +      feature++;
> +    } else {
> +      if (filetype)
> +        strncat (features, ",filetype", 9);
> +      else
> +        strncat (features, ",^filetype", 10);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_FLEXBG_BITMASK) {
> +    if (feature == 0) {
> +      if (flexbg)
> +        strncat (features, "flex_bg", 7);
> +      else
> +        strncat (features, "^flex_bg", 8);
> +      feature++;
> +    } else {
> +      if (flexbg)
> +        strncat (features, ",flex_bg", 8);
> +      else
> +        strncat (features, ",^flex_bg", 9);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_HASJOURNAL_BITMASK) {
> +    if (feature == 0) {
> +      if (hasjournal)
> +        strncat (features, "has_journal", 11);
> +      else
> +        strncat (features, "^has_journal", 12);
> +      feature++;
> +    } else {
> +      if (hasjournal)
> +        strncat (features, ",has_journal", 12);
> +      else
> +        strncat (features, ",^has_journal", 13);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_JOURNALDEV_BITMASK) {
> +    if (feature == 0) {
> +      if (journaldev)
> +        strncat (features, "journal_dev", 11);
> +      else
> +        strncat (features, "^journal_dev", 12);
> +      feature++;
> +    } else {
> +      if (journaldev)
> +        strncat (features, ",journal_dev", 12);
> +      else
> +        strncat (features, ",^journal_dev", 13);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_LARGEFILE_BITMASK) {
> +    if (feature == 0) {
> +      if (largefile)
> +        strncat (features, "large_file", 10);
> +      else
> +        strncat (features, "^large_file", 11);
> +      feature++;
> +    } else {
> +      if (largefile)
> +        strncat (features, ",large_file", 11);
> +      else
> +        strncat (features, ",^large_file", 12);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_QUOTA_BITMASK) {
> +    if (feature == 0) {
> +      if (quota)
> +        strncat (features, "quota", 5);
> +      else
> +        strncat (features, "^quota", 6);
> +      feature++;
> +    } else {
> +      if (quota)
> +        strncat (features, ",quota", 6);
> +      else
> +        strncat (features, ",^quota", 7);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_RESIZEINODE_BITMASK) {
> +    if (feature == 0) {
> +      if (resizeinode)
> +        strncat (features, "resize_inode", 12);
> +      else
> +        strncat (features, "^resize_inode", 13);
> +      feature++;
> +    } else {
> +      if (resizeinode)
> +        strncat (features, ",resize_inode", 13);
> +      else
> +        strncat (features, ",^resize_inode", 14);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_SPARSESUPER_BITMASK) {
> +    if (feature == 0) {
> +      if (sparsesuper)
> +        strncat (features, "sparse_super", 12);
> +      else
> +        strncat (features, "^sparse_super", 13);
> +      feature++;
> +    } else {
> +      if (sparsesuper)
> +        strncat (features, ",sparse_super", 13);
> +      else
> +        strncat (features, ",^sparse_super", 14);
> +    }
> +  }
> +  if (optargs_bitmask & GUESTFS_MKE2FS_UNINITBG_BITMASK) {
> +    if (feature == 0) {
> +      if (uninitbg)
> +        strncat (features, "uninit_bg", 9);
> +      else
> +        strncat (features, "^uninit_bg", 10);
> +      feature++;
> +    } else {
> +      if (uninitbg)
> +        strncat (features, ",uninit_bg", 10);
> +      else
> +        strncat (features, ",^uninit_bg", 11);
> +    }
> +  }
> +
> +  if (feature != 0) {
> +    ADD_ARG (argv, i, "-O");
> +    ADD_ARG (argv, i, features);
> +  }
> +
> +  ADD_ARG (argv, i, device);
> +
> +  if (optargs_bitmask & GUESTFS_MKE2FS_BLOCKSCOUNT_BITMASK) {
> +    if (blockscount < 0) {
> +      reply_with_error ("blockscount must be >= 0");
> +      goto error;
> +    }
> +    snprintf (blockscount_s, sizeof blockscount_s, "%" PRIi64, blockscount);
> +    ADD_ARG (argv, i, blockscount_s);
> +  }
> +
> +  ADD_ARG (argv, i, NULL);
> +
> +  r = commandv (NULL, &err, argv);
> +  if (r == -1) {
> +    reply_with_error ("%s: %s", device, err);
> +    goto error;
> +  }
> +
> +  free (err);
> +  return 0;
> +
> +error:
> +  if (err) free (err);
> +  return -1;
> +}
> diff --git a/generator/actions.ml b/generator/actions.ml
> index 65012ad..cfd5198 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -9710,6 +9710,24 @@ the resulting filesystem may be inconsistent or corrupt.
>  The returned status indicates whether filesystem corruption was
>  detected (returns C<1>) or was not detected (returns C<0>)." };
>  
> +  { defaults with
> +    name = "mke2fs";

> + style = RErr, [Device "device"],

> [OBool "checkbadblock"; OString
> "badblockfile";

Bad blocks aren't relevant on virtual disks.  I would
omit anything related to bad blocks unless someone comes
up with a compelling reason for them.

> OInt64 "blocksize";
> OBool "directwrite";

I would omit "directwrite".  It appears to be a very specialized
corner-case, and if not it should probably be enabled by default.

> OInt64 "fragsize"; OBool "forcecreate"; OInt64 "blockspergroup"; OInt64
> "numberofgroups"; OInt64 "bytesperinode"; OInt64 "inodesize"; OBool
> "withjournal"; OInt64 "journalsize"; OString "journaldevice";
> OString "newvolumelabel";

Elsewhere (eg. mkswap) we have called this just "label".

> OInt "reservedblockspercentage";
> OString "lastmounteddir"; OInt64 "numberofinodes"; OString "creatoros";
> OBool "writesbandgrouponly"; OString "fstype";
> OString "usagetype";
> OString "fsuuid";

Elsewhere we have called this just "uuid".

> OInt "mmpupdateinterval"; OInt64 "stridesize";
> OInt64 "stripewidth"; OInt64 "maxonlineresize"; OBool
> "lazyitableinit"; OBool "lazyjournalinit"; OBool "testfs"; OBool
> "discard"; OBool "quotatype"; OBool "dirindex"; OBool "extent";
> OBool "filetype"; OBool "flexbg";
> OBool "hasjournal";

I think -O has-journal is the same as the -j option, so probably
this one can be omitted.

> OBool "journaldev"; OBool "largefile"; OBool "quota"; OBool "resizeinode";
> OBool "sparsesuper"; OBool "uninitbg";

I was waiting for ...

> OInt64 "blockscount"];

I think blockscount should be the first in the optional argument list.

> +    proc_nr = Some 367;
> +    tests = [
> +      InitEmpty, IfAvailable "ext2", TestRun (
> +        [["part_disk"; "/dev/sda"; "mbr"];
> +         ["mke2fs"; "/dev/sda1"; ""; "NOARG"; ""; ""; ""; ""; ""; ""; ""; ""; "true"; ""; "NOARG"; "NOARG"; ""; "NOARG"; ""; "NOARG"; ""; "ext4"; "NOARG"; "NOARG"; ""; ""; ""; ""; ""; ""; ""; ""; ""; ""; ""; ""; ""; ""; ""; ""; ""; ""; ""; ""; ""];
> +      ])
> +    ];

This needs to add deprecated_by for any existing API which
can now be done using this call.  There are several of them.
AND it needs to add the tests for all those other APIs too
(while leaving the tests of the other APIs intact).
See mkswap (f23a5c468e959f86df89e2a4e8aa9160eca532f8) for an
example of what I mean.

> +    shortdesc = "create an ext2/ext3/ext4 filesystem on C<device>";

You can't use POD (C<> etc) in shortdesc.

> +    longdesc = "\
> +C<mke2fs> is used to create an ext2, ext3, or ext4 filesystem,
> +usually in a disk partition. C<device> is the special file corresponding
> +to the device (e.g /dev/sdXX). C<blockscount> is the number of blocks
> +on C<device>. If omitted, C<mke2fs> automagically figures the file system
> +size." };
> +
>  ]

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]