On 12/12/2016 01:27 PM, Andrei Borzenkov wrote: > While I agree with your reasons, it belongs to separate commit, and > definitely is out of place if you say in commit message "I'm moving > piece of code". Actually, it is not related to this patch series, so > feel free to send this cleanup as separate patch.
Thank you. I'll stick to 'return 0' for this series. > > On Mon, Dec 12, 2016 at 12:47 PM, Stanislav Kholmanskikh > <[email protected]> wrote: >> >> >> 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 <[email protected]> >>>> --- >>>> 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 >>> [email protected] >>> https://lists.gnu.org/mailman/listinfo/grub-devel >>> >> >> _______________________________________________ >> Grub-devel mailing list >> [email protected] >> https://lists.gnu.org/mailman/listinfo/grub-devel > > _______________________________________________ > Grub-devel mailing list > [email protected] > https://lists.gnu.org/mailman/listinfo/grub-devel > _______________________________________________ Grub-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
