Re: [PATCH for-4.14] tools: fix error path of xendevicemodel_open()

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Andrew Cooper writes ("Re: [PATCH for-4.14] tools: fix error path of 
xendevicemodel_open()"):
> There is still the crash described below which needs some form of
> figuring out and fixing.
...
> >> Failure to create the logger will still hit the NULL deference, in all of 
> >> the
> >> stable libs, not just devicemodel.

Are you sure ?

I think you mean this sequence of events:

  xencall_open(logger=NULL, open_flags=0)
     xcall->logger = NULL; /* from logger */
     xcall->logger_tofree = NULL;
     if (1) {
       xtl_createlogger_stdiostream => NULL
       /* so */ goto err;
     }

   err:
     xentoolcore__deregister_active_handle(&xcall->tc_ah); /* " */
     osdep_xencall_close(xcall);
     xencall_close(dmod->xcall);
     xtl_logger_destroy(xcall->logger_tofree /* NULL */); // <- crash?
     free(xcall);

But xtl_logger_destroy(NULL) is a safe no-op.

However,

> >> Also, unless I'd triple checked the history, I was about to reintroduce the
> >> deadlock from c/s 9976f3874d4

These comments made me look again at 9976f3874d4 "tools:
xentoolcore_restrict_all: Do deregistration before close".

Just now I wrote:

   I notice that the tail of xendevicemodel_open is now identical to
   xendevicemodel_close.  I think this is expected, and that it would be
   better to combine the two sets of code.  If they hadn't been separate
   then we might have avoided this bug...

But in fact this is not true.  xendevicemodel_close has them in the
right order, but xendevicemodel_open's err block has them in the wrong
order.

Now that I look at xencall, I discover tht xencall_open's err block is
not identical to xencall_close.  xencall close calls
buffer_release_cache and xencall_open's err block does not.  Looking
more closely I think this happens not to be a memory leak because
xcall->buffer* don't contain any malloc'd stuff until they are used.

But I think this is poor practice and another example of teardown code
duplication being a bad idea.

> >> because it totally counterintuitive wrong to
> >> expect setup and teardown in opposite orders.

Are you sure you wrote what you meant ?  To my mind it is usual for
setup and teardown to proceed in precisely the opposite order.

The need to call xentoolcore__deregister_active_handle before closing
the fd is to my mind unusual and counterintuitive.  The reasons are
explained in the commit message for 9976f3874d4cca82.

Does that all make sense ?

Perhaps we should at least delete the wrong err path of
xendevicemodel_open with a call to xendevicemodel_close ?

Ian.