Falcon back-end patch
Pete Zaitcev
zaitcev at redhat.com
Mon Mar 28 16:36:55 UTC 2011
On Mon, 28 Mar 2011 15:53:01 +0200
Jim Meyering <jim at meyering.net> wrote:
> Thanks for the patch, and especially for taking the
> trouble to write a test case for it.
No trouble at all, I basically copy-pasted existing cases like a monkey.
> backend.c:1601:11: error: assignment discards 'const' qualifier from\
> pointer target type [-Werror]
Hmm... I suppose there's something to your stock build flags. It used
to be that I caught something that you didn't, but this time it's
the opposite. Thanks for that patch, I applied it to the tree in case.
> Also, I noticed some hard-coded argv[12] limits, with no protection
> against overflow if someone adds new command line arguments,
> so I added an assertion [likewise in two other places]:
>
> const char *cmd = "dc-register-image";
> argv[argc++] = cmd;
> argv[argc++] = ms->bucket;
.....
> argv[argc] = NULL;
> assert (argc < ARRAY_CARDINALITY (argv));
Yeah, nasty. I was thinking about some kind of inline function that
would catch it before the scribbing, but this is good, or better than
it was for sure.
> > @@ -1350,8 +1352,7 @@ fs_rhevm_register (my_state *ms, const provider_t *prov, const char *next,
> > char *nfs_dir;
> > const char *conf_name = NULL;
> > char *conf_text;
> > - int ret = MHD_HTTP_BAD_REQUEST;
> > - int rc;
> > + int rc = MHD_HTTP_BAD_REQUEST;
>
> At first I wondered if this was a bug fix,
> but then saw that it's just a consistency thing,
> using "rc" in place of "ret". []
It was a bug, the ret was not set in places.
> while (fgets(buf,sizeof(buf)-1,fp)) {
> if ((s = strchr(buf, '\n')) != NULL)
> *s = '\0';
> if (regexec(&s3_success_pat,buf,2,match,0) == 0) {
> buf[match[1].rm_eo] = '\0';
> DPRINTF("found image UUID: %s\n",buf+match[1].rm_so);
> sprintf(ami_id_buf,"OK %.60s",buf+match[1].rm_so);
> rc = MHD_HTTP_OK;
> }
> else if (strcmp(buf,"ERROR") == 0) {
> DPRINTF("found err marker: %s\n",buf+sizeof("ERROR"));
> sprintf(ami_id_buf,"failed %.56s",buf+sizeof("ERROR"));
> rc = MHD_HTTP_INTERNAL_SERVER_ERROR;
> }
> else {
> DPRINTF("ignoring line: <%s>\n",buf);
> }
> }
>
> Note how finding an error marker sets rc to indicate the failure,
> but if the next line looks like success, we reset rc to MHD_HTTP_OK,
> effectively forgetting the previous failure.
> Is that desirable?
That's a good question... Both of these should not occur, but if
they do, we should treat it as an error.
Note that after we talked about it in context of copy_file_preserving(),
I decided that we have to be more robust about it and primarily rely
on the error code from the exit, and not the messages. This works in
case of /bin/cp failing and such. The loop in Falcon part is accordingly
different from the legacy loop above.
> > + sprintf(ami_id_buf,"pending %lld",(long long)time(NULL));
> > + DPRINTF("temporary ami-id = \"%s\"\n",ami_id_buf);
> > + (void)meta_set_value(ms->bucket,ms->key,"ami-id",ami_id_buf);
>
> I know it's done elsewhere, but why ignore failure here?
Dunno.
> > +if [ "$bucket" = "" ]; then
> > + echo "dc-falcon-image: No bucket" >&2
> > + exit 1
> > +fi
>
> Rather than repeating the literal program name
> and the "echo ... >&2" syntax, how about using
> code like this:
>
> ME=$(expr "./$0" : '.*/\(.*\)$')
> warn () { printf '%s\n' "$@" >&2; }
> die () { warn "$ME: $@"; exit 1; }
This looks nice, thanks. Do we need export ME for it to be visible
in a function?
> > +# The API defines staging/ for a good reason: if anything goes wrong,
> > +# a clean-up process can collect pieces even if we crash. So, create temp
> > +# in staging/, but be polite and set a trap anyway.
> > +#
> > +tmpdir=$(mktemp -d -p $nfsdir/staging) || exit 1
> > +trap "rm -rf $tmpdir" EXIT
>
> Might as well also trap also on interrupt, etc.
> This copies code from init.sh:
>
> # This trap statement, along with a trap on 0 below, ensure that the
> # temporary directory, $test_dir_, is removed upon exit as well as
> # upon receipt of any of the listed signals.
> for sig_ in 1 2 3 13 15; do
> eval "trap 'Exit $(expr $sig_ + 128)' $sig_"
> done
I was wondering about this... When administrator is starting to
kill these things, he might not want clean-ups to happen if they
get stuck somehow (on NFS). But then it all gets to -9 eventually.
> tmpdir=
> trap 'rm -rf $tmpdir' 0
> tmpdir=$(mktemp -d -p $nfsdir/staging) || exit 1
>
> Finally, note that I used "trap ... 0", rather than
> the more readable trap ... EXIT. Unfortunately,
> there are still a few mainstream shells for which
> the names don't work.
But dc-falcon-image specifies bash in #!/bin/bash. It does not use
the same init.sh mechanism that tests use, for speed and because
we're on Linux anyway.
> > +cp "$bucket/$object" "$tmpdir/$object" || exit 1
> > +spitdesc $tmpdir $object || exit 1
> > +
> > +# great, we didn't run out of disk space, now rename and declare victory.
> > +mv $tmpdir/$object "$nfsdir"
> > +mv $tmpdir/$object.xml "$nfsdir"
>
> Might as well move both at once:
>
> mv $tmpdir/$object $tmpdir/$object.xml "$nfsdir"
Hmm, good idea.
> > +mkdir -p reg/mongod reg/_fs reg/falcon/staging || framework_failure_ mkdir failed
> > +
> > +port=9094
> > +m_port=27019
>
> Using those values makes at least auto and basic tests fail
> when running parallel "make check", since you're using the same
> ports that they use for those purposes (one of each).
I see what's going on. I noticed that ports were spread out, but
forgot about make -j. I thought it was something about ports stuck
in TIME_WAIT or whatnot. At least we have reg/ subdirectory by
accident (I wanted to preserve it from cleaning sometimes).
> This is too subtle and hard to manage, so
> I've just assigned myself the task of automating
> selection of those two port numbers in each test.
Hail has that... Its servers have a mode when they select a random
port (kernel selects), and write the eventual port into a specified file.
Then, test scripts use $(cat tabled.port) etc. This was done so that
parallel builds in Koji do not conflict.
> > +PATH=$PATH:${abs_top_srcdir} iwhd -v -p $port -c iwhd_reg.conf -d localhost:$m_port &
>
> Please split long lines.
ok
-- Pete
More information about the iwhd-devel
mailing list