Re: [PATCH 1/2] tools/libs/evtchn: decouple more from mini-os

[Juergen Gross <jgross@xxxxxxxx>]

On 10.01.22 19:51, Andrew Cooper wrote:
> On 10/01/2022 12:49, Juergen Gross wrote:
> > On 10.01.22 13:25, Andrew Cooper wrote:
> > > On 07/01/2022 10:35, Juergen Gross wrote:
> > > > @@ -75,12 +86,25 @@ static void port_dealloc(struct evtchn_port_info
> > > > *port_info)
> > > >     */
> > > >    int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int flags)
> > > >    {
> > > > -    int fd = alloc_fd(FTYPE_EVTCHN);
> > > > +    int fd;
> > > > +    struct file *file;
> > > > +    struct port_list *list;
> > > > +
> > > > +    list = malloc(sizeof(*list));
> > > > +    if ( !list )
> > > > +        return -1;
> > > > +
> > > > +    fd = alloc_fd(FTYPE_EVTCHN);
> > > > +    file = get_file_from_fd(fd);
> > > >    -    if ( fd == -1 )
> > > > +    if ( !file )
> > > > +    {
> > > > +        free(list);
> > > >            return -1;
> > > > +    }
> > > This wants rearranging to keep alloc_fd() ahead of malloc().  With that,
> > > there is no need for free(list) in this error path.
> > Yeah, but the error path of malloc() having failed is quite nasty then.
> Oh yeah.  This is ugly, but I guess it is better this way around.
Please define "this way around". Do you mean like it is in my patch, or
with the malloc() after alloc_fd()?

With your suggestion I'm basically having an error path with close() in
it, while with my variant I'm having one with free() in it. I'd rather
have a local handling doing free(), than to use another external call to
close() for a half opened file.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature