Re: [wip patch] new zsh/attr module

[Peter Stephenson <pws@xxxxxxx>]

On Thu, 26 Feb 2009 22:55:47 +0100 (CET)
Mikael Magnusson <mikachu@xxxxxxxxx> wrote:
> So I cobbled together this module to make three builtins, 
> zgetattr, zsetattr and zdelattr.
> 
> #usage, *(e:fattr name:) or *(e:fattr name value:)
> function fattr() {
>    local val
>    zgetattr $REPLY user.$1 val 2>/dev/null
>    [[ -n "$val" && ( -z "$2" || "$val" =~ "$2" ) ]]
> }

This looks useful...

> I'm not sure if I should mention it being copied from cap.c since pretty 
> much only the skeleton remains.

I think not.

> I guess I would have to write some documentation too.

Yes.

> The builtins should probably handle more than one file

Right, then you'd need to use arrays to return values.  There's
an argument for an option to put name/value pairs into associative arrays;
that doesn't give you anything fundamentally new but it does prevent you
having to loop over a file name and an attribute array at the same time
which is potentially slow.

> and parse options in a better way.

I think the optional parameter argument is OK; however, adding options is
trivial since you're going through the standard builtin handling code.

> A builtin for listing attrs on a file
> would be useful too (could at least be used for completion of the second
> argument :) ).

Yes, and in this case a missing parameter should just output to standard
output.  I'm in two minds as to whether that would be better with zgetattr
than defaulting to REPLY; it is more general since you can always specify
REPLY if you want to use it (and with more than one file it would be
"reply" anyway), and if the listing command does this it might be more
consistent for the get command to do so.

> Maybe the module should be called xattr instead of just attr?

I'm not particularly bothered either way.  Too many x's and z's is a bit
ugly.  However, there are multiple attribute systems and there's an
argument for being specific.

> I also didn't bother checking what happens when the system doesn't
> support xattrs or doesn't have the includes. I guess something similar to
> what db_gdbm.mdd does is needed? I noticed just now that I was lazy and
> used the ?: extension so that's something to fix too.

Yes, you need to conditionalise linking by putting some configure
(i.e. portable shell code) tests into link=`...` in the .mdd file.
You should probably test both that the function and the header exist.

> Do I need to nul terminate strings I give to metafy() and/or setsparam()?

To metafy(), no, since there's an explicit length and the pre-metafied
input may include NULs within the string (that's one of the points of
metafication); to setsparam(), yes, since that's a metafied NUL-terminated
string.

> I think the AC_CHECK_LIB isn't exactly right either, it adds a second -lc 
> to $LIBS.

You should just add the getxattr test to the AC_CHECK_FUNCS lib.


> It seems these *argv and the ones below need to be wrapped in unmeta()
> to work with utf-8 filenames/values.

Yes, indeed:  internal values are nearly always passed around metafied (and
exceptions should be clearly documented, but I bet they aren't), but system
calls don't know anything about metafication.

> Obviously I can't use unmeta()
> twice in one function call though, can I call unmetafy() on the argv
> values or will something be sad then?

Yes, that should be fine.  argv is off the heap and commonly used as
workspace.

> Can the length grow when I unmetafy so I would need to alloc more space?

No, it will only ever remove Meta bytes (and xor the following one with
32).

> I also note the cap.c file doesn't unmeta(fy) its arguments so it
> probably also doesn't work, but I don't have cap stuff so can't test.

Yes, that's entirely possible; it probably hasn't been well tested with
new-fangled file names.

> Also, in unmeta() would putting a break; in the initial loop help? ie
>     for (t = file_name; *t; t++) {
> 	if (*t == Meta) {
> 	    meta = 1;
> +           break;
>         }
>     }

No, we need "t" to point to the end of the file name in this case for the
size check.

> My impression from looking at the code is that only metafy()ing can
> grow the string, so it should be safe to just unmetafy() the strings,
> assuming nothing breaks from me modifying the argv strings?

That's right.  The place to be wary about is if you later need to pass the
same strings to something else internally, but you can probably get away
without that.

-- 
Peter Stephenson <pws@xxxxxxx>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070