Merge lp:~zyga/checkbox/attachment-fixes into lp:checkbox

[Zygmunt Krynicki]

284c6e6 plainbox:providers:stubbox: disable strict validation
82ac810 plainbox:providers:stubbox: add test attachment jobs
46b8e41 plainbox:excmd: don't expose non-blocking pipes to children

[lp:~zyga/checkbox/attachment-fixes]

3501. By Zygmunt Krynicki on 2014-12-12

plainbox:providers:stubbox: add test attachment jobs

This patch adds a collection of stubbox jobs for testing attachments.

    stub/text-attachment: prints (and captures) a small UTF-8 encoded
    piece of text. It can catch errors in handling output encoding

    stub/large-text-attachment: prints (and captures) a large UTF-8
    encoded piece of text. It can catch errors related to buffering.

    stub/binary-attachment: prints (and captures) a
    specifically-difficult-to-handle binary data consisting of all 256
    bytes

    stub/large-binary-attachment: prints (and captures) a large binary
    blob to test handling of memory efficiency.

Signed-off-by: Zygmunt Krynicki <email address hidden>

3502. By Zygmunt Krynicki on 2014-12-12

plainbox:excmd: don't expose non-blocking pipes to children

This patch fixes a bug in the glibc-based extcmd implementation.

The communication pipes that are created to pass stdout and stderr data
were created in non-blocking mode and that non-blocking pipe was exposed
to the child process. A child process writing fast enough could fill the
pipe and get EAGAIN. Since applications are not generally designed to
handle that it would cause random failures.

The fix based on creating normal (blocking) pipes and only set the
non-blocking mode in the parent process that has an event loop.

Fixes: https://bugs.launchpad.net/plainbox/+bug/1401886

Signed-off-by: Zygmunt Krynicki <email address hidden>

[Sylvain Pineau (sylvain-pineau)]

+1, thanks for the quick fix

review: Approve

[Daniel Manrique (roadmr)]

+1 on the fix, I have a couple of minor questions but I'd be OK with the test definitions as well.

review: Needs Information

[Zygmunt Krynicki (zyga)]

I've selected Polish out of selfishnesss, it's equally good as any other non-ASCII sequence.

I've used /dev/zero instead of a hardcoded gzipped file full of zeros (it's just silly to have that file in the tree in retrospective). The purpose of the test is to see how we handle *large* files, not if we can handle them at all (there, an existing, more comprehensive test checks *each* possible byte)

=== added directory 'plainbox/plainbox/impl/providers/stubbox/data'
=== added file 'plainbox/plainbox/impl/providers/stubbox/data/all-bytes'
Binary files plainbox/plainbox/impl/providers/stubbox/data/all-bytes	1970-01-01 00:00:00 +0000 and plainbox/plainbox/impl/providers/stubbox/data/all-bytes	2014-12-12 14:51:09 +0000 differ
=== modified file 'plainbox/plainbox/impl/providers/stubbox/manage.py'
--- plainbox/plainbox/impl/providers/stubbox/manage.py	2014-04-04 09:41:22 +0000
+++ plainbox/plainbox/impl/providers/stubbox/manage.py	2014-12-12 14:51:09 +0000
@@ -75,5 +75,6 @@
         name=stubbox_def.name,
         version=stubbox_def.version,
         description=stubbox_def.description,
-        gettext_domain=stubbox_def.gettext_domain
+        gettext_domain=stubbox_def.gettext_domain,
+        strict=False,
     )

=== modified file 'plainbox/plainbox/impl/providers/stubbox/units/jobs/stub.pxu'
--- plainbox/plainbox/impl/providers/stubbox/units/jobs/stub.pxu	2014-11-25 11:33:53 +0000
+++ plainbox/plainbox/impl/providers/stubbox/units/jobs/stub.pxu	2014-12-12 14:51:09 +0000
@@ -352,3 +352,62 @@
 command: test $(id -u) -eq 0
 estimated_duration: 0.1
 category_id: superuser
+
+id: stub/text-attachment
+_summary: A job that attaches a plain text file
+_description:
+ This job attaches a simple, fixed, piece of UTF-8 encoded text as attachment
+plugin: attachment
+flags: preserve-locale
+# The subsequent polish text is a typical 'the quick brown fox...' text that
+# is used just because it's likely to expose any non-ASCII text handling bugs.
+command:
+ echo "zazółć gęślą jaźń"
+estimated_duration: 0.1
+category_id: plugin-representative
+
+id: stub/binary-attachment
+_summary: A job that attaches representative binary data
+_description:
+ This job generates bytes 0 through 255 to test handling of bytes that may
+ occur but be mishandled by our I/O processing engine.
+plugin: attachment
+flags: preserve-locale
+# The all-bytes file can be generated with the following piece of bash but
+# I wanted to avoid reliance on the obscure escape processing for
+# portability:
+# for i in $(seq 0 255); do
+#  echo -n -e "\x$(printf %x $i)"
+# done
+command:
+ cat $PLAINBOX_PROVIDER_DATA/all-bytes
+
+id: stub/large-text-attachment
+_summary: A job that attaches a plain text file
+_description:
+ This job attaches a large, repeated sequence of UTF-8 encoded text as
+ attachment. It helps to stress the I/O handling code that might not happen in
+ a trivial (short / small) attachment.
+plugin: attachment
+flags: preserve-locale
+# The subsequent polish text is a typical 'the quick brown fox...' text that
+# is used just because it's likely to expose any non-ASCII text handling bugs.
+command:
+ for i in $(seq 100000); do
+    echo "$i: zazółć gęślą jaźń"
+ done
+estimated_duration: 0.1
+category_id: stress
+
+id: stub/large-binary-attachment
+_summary: A job that attaches representative binary data
+_description:
+ This job attaches 16GBs of zeros to see if we can handle (mostly on the memory
+ front) such types of attachments, e.g. someone attaching a swap file, or
+ something equally unexpected and very large.
+plugin: attachment
+flags: preserve-locale
+command:
+ dd if=/dev/zero bs=1M count=16384
+estimated_duration: 750
+category_id: stress

=== modified file 'plainbox/plainbox/vendor/extcmd/glibc.py'
--- plainbox/plainbox/vendor/extcmd/glibc.py	2014-12-05 12:07:05 +0000
+++ plainbox/plainbox/vendor/extcmd/glibc.py	2014-12-12 14:51:09 +0000
@@ -24,6 +24,7 @@
 """
 import contextlib
 import errno
+import fcntl
 import logging
 import os
 import signal
@@ -110,9 +111,9 @@
         _logger.debug("Obtained: %r", sfd)
         sigmask = pthread_sigmask(signal_list)
         _logger.debug("Obtained: %r", sigmask)
-        stdout_pair = pipe2(O_CLOEXEC | O_NONBLOCK)
+        stdout_pair = pipe2(O_CLOEXEC)
         _logger.debug("Obtained: %r", stdout_pair)
-        stderr_pair = pipe2(O_CLOEXEC | O_NONBLOCK)
+        stderr_pair = pipe2(O_CLOEXEC)
         _logger.debug("Obtained: %r", stderr_pair)
         key = selector.register(stdout_pair[0], EVENT_READ, 'stdout')
         _logger.debug("Registered key with selector: %r", key)
@@ -171,6 +172,15 @@
                 os.exit(-1)
             else:
                 _logger.debug("Forked child process: %r", pid)
+                # Make all pipes non-blocking as the loop below cannot work
+                # with any blocking I/O.
+                flags = fcntl.fcntl(stdout_pair[0], fcntl.F_GETFL)
+                flags |= O_NONBLOCK
+                fcntl.fcntl(stdout_pair[0], fcntl.F_SETFL, flags)
+                flags = fcntl.fcntl(stderr_pair[0], fcntl.F_GETFL)
+                flags |= O_NONBLOCK
+                fcntl.fcntl(stderr_pair[0], fcntl.F_SETFL, O_NONBLOCK)
+                # Close the write sides of the pipes
                 os.close(stdout_pair[1])
                 os.close(stderr_pair[1])
                 return self._loop(selector, pid)