From: Jeff Hostetler <git@jeffhostetler.com>
Date: Thu, 23 Sep 2021 17:58:48 GMT
Subject: Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command
Message-ID: <2df757dc-bb19-555f-767e-6a4de9852bdb@jeffhostetler.com>
In-Reply-To: <87r1df497s.fsf@evledraar.gmail.com>



On 9/23/21 11:03 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote:
> 
>> +	switch (sbgr) {
>> +	case SBGR_READY:
>> +		return 0;
>>   
>> -		else if (pid_seen == pid_child) {
>> -			/*
>> -			 * The new child daemon process shutdown while
>> -			 * it was starting up, so it is not listening
>> -			 * on the socket.
>> -			 *
>> -			 * Try to ping the socket in the odd chance
>> -			 * that another daemon started (or was already
>> -			 * running) while our child was starting.
>> -			 *
>> -			 * Again, we don't care who services the socket.
>> -			 */
>> -			s = ipc_get_active_state(cl_args.path);
>> -			if (s == IPC_STATE__LISTENING)
>> -				return 0;
>> +	default:
>> +	case SBGR_ERROR:
>> +	case SBGR_CB_ERROR:
>> +		return error("daemon failed to start");
> 
> There was a discussion on v1 about the "default" being redundant here
> and hiding future compiler checks, this is another "not sure what you
> thought of that" case (per [1]).
> 
> Interestingly in this case if I drop the "default" my local gcc
> uncharacteristically complains about a missing "return" in this
> function, but clang doesn't. I needed to add a BUG() to shut up the
> former. Maybe I'm wrong, but perhaps it's a sign of some deeper
> trouble. This is with gcc/clang 10.2.1-6/11.0.1-2.

The issue of whether C needs a "default" case in switch statements
on an enum is one I didn't have bandwidth to think about (and is
completely independent of my series).

I was thinking that as a later task, someone could investigate which
compilers do and do not generate errors for missing enum values in
the switch.  Perhaps that leads to a macro in config.mak.uname on
a system-by-system basis that "does the right thing".

Then one could have something like:

     switch (e) {
     DEFAULT_HANDLER;
     case e1: ...
     case e2: ...
     }


> 
> 1. https://lore.kernel.org/git/87v92r49mt.fsf@evledraar.gmail.com/
> 
> I played with the diff below on top of this, I can't remember if it was
> noted already, but the way you declare function ptrs and use them isn't
> the usual style:
> 
> -- >8 --
> diff --git a/run-command.c b/run-command.c
> index 76bbef9d96d..5c831545201 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1903,7 +1903,7 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)
>   }
>   
>   enum start_bg_result start_bg_command(struct child_process *cmd,
> -				      start_bg_wait_cb *wait_cb,
> +				      start_bg_wait_cb wait_cb,
>   				      void *cb_data,
>   				      unsigned int timeout_sec)
>   {
> diff --git a/run-command.h b/run-command.h
> index 17b1b80c3d7..e8a637d1967 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -527,7 +527,7 @@ enum start_bg_result {
>    * Returns 0 if child is "ready".
>    * Returns -1 on any error and cause start_bg_command() to also error out.
>    */
> -typedef int(start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);
> +typedef int (*start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);

By defining the typedef WITHOUT the star, we get a function type.

We can then use it for forward function declarations.

But additionally, when declare a function that takes a function
pointer or when we define a vtable of function pointers, they look
like pointers.

start_bg_wait_cb *pfn = my_cb;

int foo(struct child_process *cmd, start_bg_wait_cb *cb);

Or if we look a the target vtable in Trace2.  This looks like
a structure of (function) pointers.

struct tr2_tgt {
	tr2_tgt_init_t                          *pfn_init;
	tr2_tgt_term_t                          *pfn_term;
	...
};

So I prefer to leave the star out of function typedef and then
we can use the typedef in both contexts.

Jeff