Return-Path: Received: from mx142.netapp.com ([216.240.21.19]:39181 "EHLO mx142.netapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757805AbcEEQnZ convert rfc822-to-8bit (ORCPT ); Thu, 5 May 2016 12:43:25 -0400 From: "Kornievskaia, Olga" To: Steve Dickson CC: "Kornievskaia, Olga" , "linux-nfs@vger.kernel.org" Subject: Re: [PATCH 2/2] gssd: move read of upcall into main thread Date: Thu, 5 May 2016 16:43:17 +0000 Message-ID: <2D606FA4-2777-48B6-979C-C27F4BADA43A@netapp.com> References: <1462286792-8889-1-git-send-email-kolga@netapp.com> <1462286792-8889-2-git-send-email-kolga@netapp.com> <50edf6a8-4d4c-07d5-af0a-363ac66dbfa5@RedHat.com> <574d004f-4b42-d4aa-60f6-607ffae6dd1b@RedHat.com> <222b4f6c-6f7a-2951-6d4e-75a522cde905@RedHat.com> In-Reply-To: <222b4f6c-6f7a-2951-6d4e-75a522cde905@RedHat.com> Content-Type: text/plain; charset="Windows-1252" MIME-Version: 1.0 Sender: linux-nfs-owner@vger.kernel.org List-ID: > On May 5, 2016, at 12:40 PM, Steve Dickson wrote: > > > > On 05/05/2016 11:40 AM, Kornievskaia, Olga wrote: >> >>> On May 5, 2016, at 11:35 AM, Kornievskaia, Olga > wrote: >>> >>>> >>>> On May 5, 2016, at 10:17 AM, Steve Dickson > wrote: >>>> >>>> Hello, >>>> >>>> On 05/04/2016 10:15 AM, Kornievskaia, Olga wrote: >>>>> I?m having a hard time picturing what you have in mind. I don?t see how you can allocate, >>>>> populate and free in one routine. The logic is we allocate memory, then read the upcall >>>>> and only then we create the thread. If either upcall or creation of the thread >>>>> fails we need to free the allocated memory. >>>> Something like this: >>>> >>>> gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which), void *data) >>>> { >>>> struct clnt_info *clp = data; >>>> - pthread_t th; >>>> - int ret; >>>> + struct clnt_upcall_info *info; >>>> >>>> - ret = pthread_create(&th, NULL, (void *)handle_gssd_upcall, >>>> - (void *)clp); >>>> - if (ret != 0) { >>>> - printerr(0, "ERROR: pthread_create failed: ret %d: %s\n", >>>> - ret, strerror(errno)); >>>> + info = alloc_upcall_info(clp); >>>> + if (info == NULL) >>>> + return; >>>> + >>>> + info->lbuflen = read(clp->gssd_fd, info->lbuf, sizeof(info->lbuf)); >>>> + if (info->lbuflen <= 0 || info->lbuf[info->lbuflen-1] != '\n') { >>>> + printerr(0, "WARNING: %s: failed reading request\n", __func__); >>>> + free(info); >>>> return; >>>> } >>>> - wait_for_child_and_detach(th); >>>> + info->lbuf[info->lbuflen-1] = 0; >>>> + >>>> + start_upcall_thread(handle_gssd_upcall, info); >>>> + >>>> + free(info); >>>> + return; >>> >>> Gotcha! Sounds good! You can make the modifications. Or just in case since you are traveling, >> I?ll just send another patch. Please also add any other lines like Modified-by or Signed-off-by that are need. >> >> I?m sorry I spoke too soon. ?info? is allocated and given to the thread >> to process so the main thread can?t free it. > I'm curious as to why? The main thread allocated it, is handing allocated memory > to a thread something special? What am I missing? The main thread allocates memory, gives it to the the child thread and goes to the next libevent. We detach the thread so if we free the memory in the main thread, the child thread would segfault.