From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1cGNE3-0003XA-Ue for mharc-grub-devel@gnu.org; Mon, 12 Dec 2016 04:48:51 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60013) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cGNCo-0003AI-6S for grub-devel@gnu.org; Mon, 12 Dec 2016 04:47:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cGNCl-00065j-2g for grub-devel@gnu.org; Mon, 12 Dec 2016 04:47:34 -0500 Received: from userp1040.oracle.com ([156.151.31.81]:18198) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cGNCk-00065K-Pa for grub-devel@gnu.org; Mon, 12 Dec 2016 04:47:30 -0500 Received: from userv0022.oracle.com (userv0022.oracle.com [156.151.31.74]) by userp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id uBC9lO4m006527 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Mon, 12 Dec 2016 09:47:27 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userv0022.oracle.com (8.14.4/8.14.4) with ESMTP id uBC9lOSS006563 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 12 Dec 2016 09:47:24 GMT Received: from abhmp0008.oracle.com (abhmp0008.oracle.com [141.146.116.14]) by aserv0121.oracle.com (8.13.8/8.13.8) with ESMTP id uBC9lLn5017246 for ; Mon, 12 Dec 2016 09:47:23 GMT Received: from [10.162.81.165] (/10.162.81.165) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Mon, 12 Dec 2016 01:47:21 -0800 Subject: Re: [PATCH V2 2/4] ofnet: move the allocation of the transmit buffer into a function To: The development of GNU GRUB References: <1480691405-15118-1-git-send-email-stanislav.kholmanskikh@oracle.com> <1480691405-15118-3-git-send-email-stanislav.kholmanskikh@oracle.com> <20161205155204.GC6024@router-fw-old.local.net-space.pl> Cc: vasily.isaenko@oracle.com From: Stanislav Kholmanskikh Message-ID: <584E7225.1020500@oracle.com> Date: Mon, 12 Dec 2016 12:47:17 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <20161205155204.GC6024@router-fw-old.local.net-space.pl> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Source-IP: userv0022.oracle.com [156.151.31.74] X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.4.x-2.6.x [generic] [fuzzy] X-Received-From: 156.151.31.81 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Dec 2016 09:47:35 -0000 On 12/05/2016 06:52 PM, Daniel Kiper wrote: > On Fri, Dec 02, 2016 at 06:10:03PM +0300, Stanislav Kholmanskikh wrote: >> In the current code search_net_devices() uses the "alloc-mem" command >> from the IEEE1275 User Interface for allocation of the transmit buffer >> for the case when GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN is set. >> >> I don't have hardware where this flag is set to verify if this >> workaround is still needed. However, further changes to ofnet will >> require to execute this workaround one more time. Therefore, to >> avoid possible duplication of code I'm moving this piece of >> code into a function. >> >> Signed-off-by: Stanislav Kholmanskikh >> --- >> grub-core/net/drivers/ieee1275/ofnet.c | 71 ++++++++++++++++++++------------ >> 1 files changed, 44 insertions(+), 27 deletions(-) >> >> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c >> index 8332d34..25559c8 100644 >> --- a/grub-core/net/drivers/ieee1275/ofnet.c >> +++ b/grub-core/net/drivers/ieee1275/ofnet.c >> @@ -298,6 +298,48 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path, >> } >> } >> >> +/* Allocate memory with alloc-mem */ >> +static void * >> +grub_ieee1275_alloc_mem (grub_size_t len) >> +{ >> + struct alloc_args >> + { >> + struct grub_ieee1275_common_hdr common; >> + grub_ieee1275_cell_t method; >> + grub_ieee1275_cell_t len; >> + grub_ieee1275_cell_t catch; >> + grub_ieee1275_cell_t result; >> + } >> + args; >> + >> + if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_CANNOT_INTERPRET)) >> + { >> + grub_error (GRUB_ERR_UNKNOWN_COMMAND, N_("interpret is not supported")); >> + return NULL; >> + } >> + >> + INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2); >> + args.len = len; >> + args.method = (grub_ieee1275_cell_t) "alloc-mem"; >> + >> + if (IEEE1275_CALL_ENTRY_FN (&args) == -1 || args.catch) >> + { >> + grub_error (GRUB_ERR_INVALID_COMMAND, N_("alloc-mem failed")); >> + return NULL; >> + } >> + else >> + return (void *)args.result; >> +} >> + >> +static void * >> +ofnet_alloc_netbuf (grub_size_t len) >> +{ >> + if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN)) >> + return grub_ieee1275_alloc_mem (len); >> + else >> + return grub_zalloc (len); >> +} >> + >> static int >> search_net_devices (struct grub_ieee1275_devalias *alias) >> { >> @@ -414,39 +456,14 @@ search_net_devices (struct grub_ieee1275_devalias *alias) >> >> card->txbufsize = ALIGN_UP (card->mtu, 64) + 256; >> >> - if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN)) >> - { >> - struct alloc_args >> - { >> - struct grub_ieee1275_common_hdr common; >> - grub_ieee1275_cell_t method; >> - grub_ieee1275_cell_t len; >> - grub_ieee1275_cell_t catch; >> - grub_ieee1275_cell_t result; >> - } >> - args; >> - INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2); >> - args.len = card->txbufsize; >> - args.method = (grub_ieee1275_cell_t) "alloc-mem"; >> - >> - if (IEEE1275_CALL_ENTRY_FN (&args) == -1 >> - || args.catch) >> - { >> - card->txbuf = 0; >> - grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory")); >> - } >> - else >> - card->txbuf = (void *) args.result; >> - } >> - else >> - card->txbuf = grub_zalloc (card->txbufsize); >> + card->txbuf = ofnet_alloc_netbuf (card->txbufsize); >> if (!card->txbuf) >> { >> grub_free (ofdata->path); >> grub_free (ofdata); >> grub_free (card); >> grub_print_error (); >> - return 0; >> + return 1; > > Hmmm... This looks like an error... Look, here is what I see. The search_net_devices() function is called from grub_ofnet_findcards() as: grub_ieee1275_devices_iterate (search_net_devices); The grub_ieee1275_devices_iterate(hook) function is defined in grub-core/kern/ieee1275/openfw.c and what it does is basically calling the hook for each IEEE1275 device in the tree until: a) there are no more devices b) the hook returns a value != 1 So if search_net_devices() returns 1 it means that further probing for network cards is stopped. I think that stopping further probes when a memory allocation function fails is fine and it aligns with the existing code at the top of the function, i.e. handling of the cases when allocating memory for 'card' and 'ofdata' fails. If I'm not mistaken, we may also need to update the block: if (need_suffix) ofdata->path = grub_malloc (grub_strlen (alias->path) + sizeof (SUFFIX)); else ofdata->path = grub_malloc (grub_strlen (alias->path) + 1); if (!ofdata->path) { grub_print_error (); return 0; } and add a 'return 1' + free some memory there. As for the other block: pprop = (grub_uint8_t *) ∝ if (grub_ieee1275_get_property (devhandle, "mac-address", pprop, sizeof(prop), &prop_size) && grub_ieee1275_get_property (devhandle, "local-mac-address", pprop, sizeof(prop), &prop_size)) { grub_error (GRUB_ERR_IO, "Couldn't retrieve mac address."); grub_print_error (); return 0; } it seems we need to add free memory procedures here as well, but I'm not sure we need to return 1 here. > > Daniel > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel >