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

On Wed, Aug 22, 2012 at 02:41:55PM +0800, Wanlong Gao wrote: 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. 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 */ } 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 ... 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