[Libguestfs] [PATCH V2] NEW API:xfs:xfs_repair

[Richard W.M. Jones]

On Wed, Aug 22, 2012 at 02:41:55PM +0800, Wanlong Gao wrote:
> Add a new api xfs_repair for repairing an XFS filesystem.
> +  ADD_ARG (argv, i, "xfs_repair");
> +

As a matter of style, it might be better to write:

  if (optargs_bitmask & GUESTFS_XFS_REPAIR_FORCELOGZERO_BITMASK) {
    if (forcelogzero)
      ADD_ARG (argv, i, "-L");
  }

It just keeps all the logic in a single place.

> +  if (!(optargs_bitmask & GUESTFS_XFS_REPAIR_IMGFILE_BITMASK))
> +    imgfile = 0;

This xfs_repair -f option is annoying!  Also the way you've defined
the "device" parameter (as type Device) means it won't work -- the
caller would never be able to use a non-device as a parameter.

Instead, can we check for the input being file or device and add the
option automatically?  It should be sufficient to change the code to
something like:

  ... Dev_or_path "device" ...

  if (STRPREFIX (device, "/dev/"))
    is_device = 1;

  if (!is_device) {
    /* do the sysroot adjustment, and add -f parameter */
  } else {
    /* just add device parameter */
  }

> +  if (!(optargs_bitmask & GUESTFS_XFS_REPAIR_NOMODIFY_BITMASK))
> +    nomodify = 0;

OK, but, the return value from xfs_repair is effectively thrown away,
so this parameter would not be useful.  I think you need to use
'commandrv' and do something useful with the return code.  Look at
what the fsck APIs do ...

> +  { defaults with
> +    name = "xfs_repair";
> +    style = RErr, [Device "device"], [OBool "imgfile"; OBool "forcelogzero"; OBool "dangerous"; OBool "nomodify"; OBool "noprefetch"; OBool "forcegeometry"; OInt64 "maxmem"; OInt64 "ihashsize"; OInt64 "bhashsize"; OInt64 "agstride"; OString "logdev"; OString "rtdev"];
> +    proc_nr = Some 350;
> +    optional = Some "xfs";
> +    tests = [
> +      InitEmpty, IfAvailable "xfs", TestRun (
> +        [["part_disk"; "/dev/sda"; "mbr"];
> +         ["mkfs"; "xfs"; "/dev/sda1"; ""; "NOARG"; ""; ""];
> +         ["xfs_repair"; "/dev/sda1"; ""; ""; ""; "true"; ""; ""; ""; ""; ""; ""; "NOARG"; "NOARG"]
> +      ])

A bit light on testing, but the API seems reasonable.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v