Re: [PATCH 24/29] libvchan: handle libxc evtchn failures properly in init functions

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 10/30/2013 06:12 AM, Matthew Daley wrote:
> On Wed, Oct 30, 2013 at 8:52 PM, Matthew Daley <mattjd@gmail.com> wrote:
>> Also, remove now-unnecessary check from close function.
>>
>> Coverity-ID: 1055609
>> Coverity-ID: 1055610
>> Coverity-ID: 1055611
>> Signed-off-by: Matthew Daley <mattjd@gmail.com>
>
> I should clarify: the base reason for this patch is that
> ctrl->event_port is a uint32_t (ie. unsigned), so the current checks
> on it for negative error results, non-negative port presence etc. are
> incorrect.
>
> I can spin a v2 with this mentioned if desired.
>
> - Matthew

I agree the clarification would be useful in the commit message. If you're
spinning a v2, you may want to use evtchn_port_or_error_t in place of int
since that is the actual typedef used in the return.

Either way,
Reviewed-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

>
>> ---
>>   tools/libvchan/init.c |   47 +++++++++++++++++++++++++++++++++++++++--------
>>   tools/libvchan/io.c   |    2 +-
>>   2 files changed, 40 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
>> index 0c7cff6..314b1f6 100644
>> --- a/tools/libvchan/init.c
>> +++ b/tools/libvchan/init.c
>> @@ -215,15 +215,30 @@ static int init_gnt_cli(struct libxenvchan *ctrl, int domain, uint32_t ring_ref)
>>
>>   static int init_evt_srv(struct libxenvchan *ctrl, int domain, xentoollog_logger *logger)
>>   {
>> +       int port;
>> +
>>          ctrl->event = xc_evtchn_open(logger, 0);
>>          if (!ctrl->event)
>>                  return -1;
>> -       ctrl->event_port = xc_evtchn_bind_unbound_port(ctrl->event, domain);
>> -       if (ctrl->event_port < 0)
>> -               return -1;
>> +
>> +       port = xc_evtchn_bind_unbound_port(ctrl->event, domain);
>> +       if (port < 0)
>> +               goto fail;
>> +       ctrl->event_port = port;
>> +
>>          if (xc_evtchn_unmask(ctrl->event, ctrl->event_port))
>> -               return -1;
>> +               goto fail;
>> +
>>          return 0;
>> +
>> +fail:
>> +       if (port >= 0)
>> +               xc_evtchn_unbind(ctrl->event, port);
>> +
>> +       xc_evtchn_close(ctrl->event);
>> +       ctrl->event = NULL;
>> +
>> +       return -1;
>>   }
>>
>>   static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char* xs_base, int ring_ref)
>> @@ -330,15 +345,31 @@ out:
>>
>>   static int init_evt_cli(struct libxenvchan *ctrl, int domain, xentoollog_logger *logger)
>>   {
>> +       int port;
>> +
>>          ctrl->event = xc_evtchn_open(logger, 0);
>>          if (!ctrl->event)
>>                  return -1;
>> -       ctrl->event_port = xc_evtchn_bind_interdomain(ctrl->event,
>> +
>> +       port = xc_evtchn_bind_interdomain(ctrl->event,
>>                  domain, ctrl->event_port);
>> -       if (ctrl->event_port < 0)
>> -               return -1;
>> -       xc_evtchn_unmask(ctrl->event, ctrl->event_port);
>> +       if (port < 0)
>> +               goto fail;
>> +       ctrl->event_port = port;
>> +
>> +       if (xc_evtchn_unmask(ctrl->event, ctrl->event_port))
>> +               goto fail;
>> +
>>          return 0;
>> +
>> +fail:
>> +       if (port >= 0)
>> +               xc_evtchn_unbind(ctrl->event, port);
>> +
>> +       xc_evtchn_close(ctrl->event);
>> +       ctrl->event = NULL;
>> +
>> +       return -1;
>>   }
>>
>>
>> diff --git a/tools/libvchan/io.c b/tools/libvchan/io.c
>> index 3c8d236..2383364 100644
>> --- a/tools/libvchan/io.c
>> +++ b/tools/libvchan/io.c
>> @@ -337,7 +337,7 @@ void libxenvchan_close(struct libxenvchan *ctrl)
>>                  }
>>          }
>>          if (ctrl->event) {
>> -               if (ctrl->event_port >= 0 && ctrl->ring)
>> +               if (ctrl->ring)
>>                          xc_evtchn_notify(ctrl->event, ctrl->event_port);
>>                  xc_evtchn_close(ctrl->event);
>>          }
>> --
>> 1.7.10.4
>>
>


-- 
Daniel De Graaf
National Security Agency