Return to Revisions

1 of 2

answered Oct 24, 2013 at 14:37

50.1k
3
130
211

There's no documentation. Even a comment at the top of the script summarizing what it does would be better than nothing.
There's no error handling. What if the user forgets the argument?
This script will break if the argument $1 contains a space. You need to write "$1" in several places.
You don't need to put ; at the end of lines like this:
```
 echo "$line is a directory";
```

This code is really unclear:

 length=${#1}
 const=4
 count=$((length-const))
 DIREC=$(echo $1 | cut -b 1-$count)

First, the variable names are poorly chosen: length of what? why const? count of what? Second, this assumes that $1 ends with .zip and will go wrong if the file has any other name. Third, Bash has a feature ${parameter%pattern} for removing a suffix from a string, so you can remove the extension from $1 like this:

    DIREC=${1%.*}

But fourth and most importantly, how do you know that the ZIP file will unpack into a directory with the same name as the ZIP file? In the example below I create a ZIP file called b.zip that unzips into the directory a:

    $ mkdir a
    $ touch a/a
    $ zip b a/a
      adding: a/a (stored 0%)
    $ rm -r a
    $ unzip b
    Archive:  b.zip
     extracting: a/a

What you should do instead (that is, if you must unzip $1) is to use the -d option to unzip to extract it to a known place.

    $ unzip -d b b
    Archive:  b.zip
     extracting: b/a/a

Why is OUTPUTFIL misspelled? Why not spell it OUTPUT_FILE?
It's a bad idea to put temporary files like $OUTPUTFIL in the current directory. There might already be a file called out.xml in the current directory and then you would end up overwriting it. You should use the mktemp program to create a unique temporary file name or directory, for example like this:
```
 OUTPUT_FILE=$(mktemp -t unzip-XXXXXX.xml)
```

These lines have several problems:

 find $DIREC -exec echo {} \; >tmp.txt
 exec<tmp.txt
 while read line

First, this will go wrong if $DIREC contains a space: you need to use quotes. Second, this will go wrong if $DIREC starts with a - (it will be interpreted as an option): you need to use the -f option to find. Third, there's no need to run a separate echo command for each file: find has a -print option. Fourth, you don't need to write the output to a temporary file in order to read it using while read line; you can pipe the output of find into the while loop. Like this:

    find -f "$DIREC" -- -print | while read line; do

Inside your while loop you use [ -f ... ] to ensure that you only process plain files (not directories). But you could use the -type f argument to find to ensure that it only outputs plain files:
```
 find -f "$DIREC" -- -type f -print | while read line; do
```
This line will go wrong if $line contains a space or starts with a -:
```
 BASE=$(base64 $line);
```

You should write:

    BASE=$(base64 -i "$line")

It's not a good idea to read the output of the base64 program into a variable like this: the output might be very large and cause Bash to use large amounts of memory. It is better to let base64 write its output directly to your output file, so that it does not have to be stored in a variable in Bash. Instead of:
```
 BASE=$(base64 $line);
 {
     echo "<inpu>${BASE}</inpu>"
 } > ${OUTPUTFIL}
```

you should write:

    {
        echo "<inpu>"
        base64 -i "$line"
        echo "</inpu>"
    } > "${OUTPUT_FILE}"

However, there is no need for an OUTPUT_FILE at all here, because curl accepts the filename - meaning "standard input", so you can write:
```
 {
     echo "<inpu>"
     base64 -i "$line"
     echo "</inpu>"
 } | curl -u admin:secret --data-binary @- http://dp1-l2.com
```
<inpu> looks like a typo to me. Are you sure you don't mean <input>? If <inpu> is correct, you should probably add a comment to explain why.
It would be better to put configuration settings like admin:secret and http://dp1-l2.com in named variables at the top of the file. (It might be even better to accept them as command-line arguments.)
The way you have written this code means that each individual file in the archive gets uploaded as a separate XML file to the server. Is this really what you want? It seems more likely to me that you would want to upload the whole archive as one XML file. Otherwise, what's the point of using XML?
As you suspected, there is in fact no need to unzip the file, because you can use zipinfo -1 to get a listing of the contents, and then use the -p option to unzip to extract a file to standard output.

Putting all this together, I would write the script line this:

#!/bin/bash
# upload-zip.sh -- encode contents of ZIP archive as base64
# strings in an XML document, and upload it.

# Destination and credentials for upload.
UPLOAD_URL=http://dp1-l2.com
USER=admin
PASSWORD=secret

if [ "$#" != 1 ]; then
    echo "Usage: $0 zipfile"
    exit 1
fi
ZIPFILE=$1

zipinfo -1 "$ZIPFILE" | while read FILE; do
    echo "<input>"
    unzip -p "$ZIPFILE" "$FILE" | base64
    echo "</input>"
done | curl -u "$USER:$PASSWORD" --data-binary @- "$UPLOAD_URL"

answered Oct 24, 2013 at 14:37

Gareth Rees

50.1k
3
130
211