[libvirt] [PATCH] tests: handle backspace-newline pairs in test input files

[Eric Blake]

On 01/31/2011 05:24 AM, Haefliger, Juerg wrote:
>>From 04856f4c830a5f40d44bb67e908a1cbf88c18ce5 Mon Sep 17 00:00:00 2001
> From: Juerg Haefliger <juerg.haefliger at hp.com>
> Date: Mon, 31 Jan 2011 06:42:57 -0500
> Subject: [PATCH] tests: handle backspace-newline pairs in test input files
> 
> This patch teaches testutil how to read multi-line input files with
> backspace-newline line continuation markers.

Thanks for tackling this!

> 
> The patch also breaks up all the single-line arguments test input files into
> multi-line files with lines shorther than 80 characters.

s/shorther/shorter/

> ---
> 
> Not sure if my mailer handles the long lines properly so I'm attaching the
> patch file as well, just in case.

Probably wise (I used just the attachment, given that the whole point of
this exercise is getting rid of mailer problems, but they aren't gone
until after this patch is in :)

Your patch failed 'make syntax-check':

TAB_in_indentation
tests/testutils.c:211:	    /* advance the temporary buffer pointer */
maint.mk: use spaces, not TAB, for indentation in C, sh, and RNG schemas

Also, I have a (minor) concern that there may be cases where we WANT to
preserve a literal backslash-newline in some test files, rather than
flattening it for all clients.  That is, would it be better to add a new
function virtTestLoadFileFlags with a flag argument that requests
whether to do \\-\n flattening, and adjust callers that need it while
keeping the existing virtTestLoadFile as a simple wrapper to
virtTestLoadFileFlags(,0)?  But as none of tests failed, that means none
of our tests are affected by this, so I'm okay with the patch as-is (and
in fact, that means if we add a test in the future that _does_ care
about literal \\-\n, then we would would add virtTestLoadFileFlags with
a non-zero to preserve literals, and not have to change any of the
existing callers that get flattening by default).

> @@ -197,17 +199,28 @@ int virtTestLoadFile(const char *file,
>          return -1;
>      }
> 
> +    (*buf)[0] = '\0';
>      if (st.st_size) {
> -        if (fread(*buf, st.st_size, 1, fp) != 1) {
> +        /* read the file line by line */
> +        while (fgets(tmp, tmplen, fp) != NULL) {
> +            len = strlen(tmp);
> +            /* remove trailing backslash-newline pair */
> +            if (len >= 2 && tmp[len-2] == '\\' && tmp[len-1] == '\n') {
> +                len -= 2;
> +            }
> +           /* advance the temporary buffer pointer */
> +            tmp += len;
> +            tmplen -= len;
> +        }
> +        if (ferror(fp)) {
>              fprintf (stderr, "%s: read failed: %s\n", file, strerror(errno));
>              VIR_FORCE_FCLOSE(fp);
>              return -1;
>          }
>      }
> -    (*buf)[st.st_size] = '\0';
> 
>      VIR_FORCE_FCLOSE(fp);
> -    return st.st_size;
> +    return strlen(*buf);

Mishandles any embedded NUL bytes, but again, none of our tests
currently use embedded NULs, so we could add a flag in the future to
force being more careful with a binary file if we have a reason to slurp
in a binary file.  Mishandles files that end in the two bytes backslash
and newline (you back up the pointer by two, but the next fgets()
returns NULL because it is at EOF without writing a NUL at the start
location) - but that's trivially solvable (I squashed in tmp[len] = '\0'
just after the len -= 2 line).  And not optimal - it counts the string
length twice (once while reading, and once at the end), but that's still
O(n) and not worth the hassle of refactoring things just to keep an
accurate running total while reading lines.  I like it enough as is,
that I'm comfortable giving:

ACK with the syntax-check nit fixed

So I pushed it.  Thanks again for taking on my request:
https://www.redhat.com/archives/libvir-list/2011-January/msg01149.html

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110131/c9caafa6/attachment-0001.sig>