zfs code - Sep 2008 - Limitation on only one filesystem per command invocation.

In the current design of zfs(1m), nearly all of the operations are limited
to working on a single filesystem at a time. This places some limitations
on the usability of zfs(1m) when using it with scripts. For example, you
cannot do:

# zfs list -H -o origin | egrep -v ''^\-$'' | xargs zfs destroy

Instead, you need to do this:

# zfs list -H -o origin | egrep -v ''^\-$'' | sh -c
''while read i; do zfs
destroy "$i"; done''

(because not everyone uses sh as their shell...)

As an experiment, I modified zfs_main.c to allow destroy to work in
a manner that is conducive to being used with xargs - patches attached.

It''s a relatively simple change but there are important questions, such
as does it fail on the first error or continue on, etc.

But I''m sure there was a reason why it was designed and implemented
the way it is rather than with what my patch suggests?

Darren


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: zfs_main.c.patch
URL:
<http://mail.opensolaris.org/pipermail/zfs-code/attachments/20080908/30bdf0e2/attachment.ksh>

On 8 Sep 2008, at 20:55, Darren Reed <Darren.Reed at Sun.COM> wrote:
> In the current design of zfs(1m), nearly all of the operations are  
> limited
> to working on a single filesystem at a time. This places some  
> limitations
> on the usability of zfs(1m) when using it with scripts. For example,  
> you
> cannot do:
>
> # zfs list -H -o origin | egrep -v ''^\-$'' | xargs zfs
destroy
That''s why ''xargs -l'' exists. :-)


However, I see nothing wrong with what you''re suggesting.







>
>
> Instead, you need to do this:
>
> # zfs list -H -o origin | egrep -v ''^\-$'' | sh -c
''while read i; do
> zfs destroy "$i"; done''
>
> (because not everyone uses sh as their shell...)
>
> As an experiment, I modified zfs_main.c to allow destroy to work in
> a manner that is conducive to being used with xargs - patches  
> attached.
>
> It''s a relatively simple change but there are important questions,
> such
> as does it fail on the first error or continue on, etc.
>
> But I''m sure there was a reason why it was designed and
implemented
> the way it is rather than with what my patch suggests?
>
> Darren
>
>
> diff -r 340e8fdced81 usr/src/cmd/zfs/zfs_main.c
> --- a/usr/src/cmd/zfs/zfs_main.c    Thu Sep 04 14:02:34 2008 -0700
> +++ b/usr/src/cmd/zfs/zfs_main.c    Mon Sep 08 17:45:36 2008 -0700
> @@ -77,6 +77,20 @@
> static int zfs_do_promote(int argc, char **argv);
> static int zfs_do_allow(int argc, char **argv);
> static int zfs_do_unallow(int argc, char **argv);
> +
> +typedef struct destroy_cbdata {
> +    boolean_t    cb_first;
> +    int        cb_force;
> +    int        cb_recurse;
> +    int        cb_error;
> +    int        cb_needforce;
> +    int        cb_doclones;
> +    boolean_t    cb_closezhp;
> +    zfs_handle_t    *cb_target;
> +    char        *cb_snapname;
> +} destroy_cbdata_t;
> +
> +static int zfs_destroy_filesystem(destroy_cbdata_t *cbp, char  
> *fsname);
>
> /*
>  * Enable a reasonable set of defaults for libumem debugging on  
> DEBUG builds.
> @@ -746,17 +760,6 @@
>  * and refuse to destroy a dataset that has any dependents.  A  
> dependent can
>  * either be a child, or a clone of a child.
>  */
> -typedef struct destroy_cbdata {
> -    boolean_t    cb_first;
> -    int        cb_force;
> -    int        cb_recurse;
> -    int        cb_error;
> -    int        cb_needforce;
> -    int        cb_doclones;
> -    boolean_t    cb_closezhp;
> -    zfs_handle_t    *cb_target;
> -    char        *cb_snapname;
> -} destroy_cbdata_t;
>
> /*
>  * Check for any dependents based on the ''-r'' or
''-R'' flags.
> @@ -886,8 +889,7 @@
> {
>    destroy_cbdata_t cb = { 0 };
>    int c;
> -    zfs_handle_t *zhp;
> -    char *cp;
> +    int idx;
>
>    /* check options */
>    while ((c = getopt(argc, argv, "frR")) != -1) {
> @@ -918,27 +920,36 @@
>        (void) fprintf(stderr, gettext("missing path
argument\n"));
>        usage(B_FALSE);
>    }
> -    if (argc > 1) {
> -        (void) fprintf(stderr, gettext("too many arguments\n"));
> -        usage(B_FALSE);
> -    }
>
>    /*
>     * If we are doing recursive destroy of a snapshot, then the
>     * named snapshot may not exist.  Go straight to libzfs.
>     */
> -    if (cb.cb_recurse && (cp = strchr(argv[0],
''@''))) {
> +    for (idx = 0; idx < argc; idx++)
> +        if (zfs_destroy_filesystem(&cb, argv[idx]))
> +            return (1);
> +
> +    return (0);
> +}
> +
> +static int
> +zfs_destroy_filesystem(destroy_cbdata_t *cbp, char *fsname)
> +{
> +    char *cp;
> +    zfs_handle_t *zhp;
> +
> +    if (cbp->cb_recurse && (cp = strchr(fsname,
''@''))) {
>        int ret;
>
>        *cp = ''\0'';
> -        if ((zhp = zfs_open(g_zfs, argv[0], ZFS_TYPE_DATASET)) ==  
> NULL)
> +        if ((zhp = zfs_open(g_zfs, fsname, ZFS_TYPE_DATASET)) ==  
> NULL)
>            return (1);
>        *cp = ''@'';
>        cp++;
>
> -        if (cb.cb_doclones) {
> -            cb.cb_snapname = cp;
> -            if (destroy_snap_clones(zhp, &cb) != 0) {
> +        if (cbp->cb_doclones) {
> +            cbp->cb_snapname = cp;
> +            if (destroy_snap_clones(zhp, cbp) != 0) {
>                zfs_close(zhp);
>                return (1);
>            }
> @@ -955,15 +966,15 @@
>
>
>    /* Open the given dataset */
> -    if ((zhp = zfs_open(g_zfs, argv[0], ZFS_TYPE_DATASET)) == NULL)
> -        return (1);
> -
> -    cb.cb_target = zhp;
> +    if ((zhp = zfs_open(g_zfs, fsname, ZFS_TYPE_DATASET)) == NULL)
> +        return (1);
> +
> +    cbp->cb_target = zhp;
>
>    /*
>     * Perform an explicit check for pools before going any further.
>     */
> -    if (!cb.cb_recurse && strchr(zfs_get_name(zhp),
''/'') == NULL &&
> +    if (!cbp->cb_recurse && strchr(zfs_get_name(zhp),
''/'') == NULL &&
>        zfs_get_type(zhp) == ZFS_TYPE_FILESYSTEM) {
>        (void) fprintf(stderr, gettext("cannot destroy
''%s'': "
>            "operation does not apply to pools\n"),
> @@ -980,16 +991,16 @@
>    /*
>     * Check for any dependents and/or clones.
>     */
> -    cb.cb_first = B_TRUE;
> -    if (!cb.cb_doclones &&
> +    cbp->cb_first = B_TRUE;
> +    if (!cbp->cb_doclones &&
>        zfs_iter_dependents(zhp, B_TRUE, destroy_check_dependent,
> -        &cb) != 0) {
> -        zfs_close(zhp);
> -        return (1);
> -    }
> -
> -    if (cb.cb_error ||
> -        zfs_iter_dependents(zhp, B_FALSE, destroy_callback, &cb) !=  
> 0) {
> +        cbp) != 0) {
> +        zfs_close(zhp);
> +        return (1);
> +    }
> +
> +    if (cbp->cb_error ||
> +        zfs_iter_dependents(zhp, B_FALSE, destroy_callback, cbp) !=  
> 0) {
>        zfs_close(zhp);
>        return (1);
>    }
> @@ -999,9 +1010,8 @@
>     * whether it succeeds or not.
>     */
>
> -    if (destroy_callback(zhp, &cb) != 0)
> -        return (1);
> -
> +    if (destroy_callback(zhp, cbp) != 0)
> +        return (1);
>
>    return (0);
> }
> _______________________________________________
> zfs-code mailing list
> zfs-code at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/zfs-code