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