Fix need_fullpath value in libretro API

[garbear]

This core assumes a full path is provided and does not handle the NULL indicator properly: https://github.com/libretro/beetle-gba-libretro/blob/f41ee5c/libretro.cpp#L3794

[garbear]

I think some other Beetle cores need this fix. I'll investigate the others when I get time.

[inactive123]

I was told by @retro-wertz that this breaks the core. So I will revert this for now.

[andres-asm]

just making it true doesn't make it magically work.
I think work has to be done so the core can work with the blob of the rom instead of just a path to the rom

[garbear]

just making it true doesn't make it magically work

The flag is poorly worded - in this case, having it set to false was the magical wishing. Un-setting it to true disables the unimplemented feature (loading from a memory blob).

I think work has to be done so the core can work with the blob of the rom instead of just a path to the rom

Yes, this is the correct path forward. However, until this is finished, IMO the core shouldn't instruct the frontend to perform behavior that leads to a segfault.

[andres-asm]

Well on the other frontend.. it's fine. I gotta be honest, there are some API problems that have not been fixed properly, more like worked around in the frontend side, this may be one such case.

…

On Thu, Jul 5, 2018 at 3:31 PM Garrett Brown ***@***.***> wrote: just making it true doesn't make it magically work The flag is poorly worded - in this case, having it set to false was the magical wishing. Un-setting it to true disables the unimplemented feature (loading from a memory blob). I think work has to be done so the core can work with the blob of the rom instead of just a path to the rom Yes, this is the correct path forward. However, until this is finished, IMO the core shouldn't instruct the frontend to perform behavior that leads to a segfault. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#33 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABpC0NyMRDadz0GttaD95wSNtFDfgvaVks5uDnengaJpZM4VC2fH> .

[garbear]

Doxy for need_fullpath says:

This is typically set to true for libretro implementations that must load from file.

This core currently must load from a file, so it should set need_fullpath to true.

[garbear]

Is there a correct way forward? This change is still needed to prevent the core from crashing when loaded without a path

[andres-asm]

Not sure what you mean with load without a path?
you need to load content only cores that have SUPPORTS_NO_GAME can be loaded without

[garbear]

libretro says

If false, ::data and ::size are guaranteed to be valid, but ::path might not be valid.

beetle gba says "false" and then proceeds to assume the path is valid.

[andres-asm]

Ah you're completely right lol!
it declares fullpath as false and then internally use the path to derive some stuff.

You are completely right, this should be fixed or at least amended in the docs I think RA always populates info->path but the API should be set in stone by now

@twinaphex I think that set_basename call is bogus and probably just left over code?
I don't see it used anywhere

[garbear]

should I reopen the PR then?

[andres-asm]

We're not sure actually.
It seems the base path is actually being used.

The problem is the might wording... the RA codepath always fills the path and it's actually used in more than one case (FDS in nestopia for instance), basically the path is used to infer the save name when the core doesn't support libretro memory saving. It's usually not a problem for needs_fullpath cores (because you have the path)

So I think it should be re-worded in the documentation. But I can't make a decision here.

[garbear]

I'm ok with an API change, but as the API is currently worded the core needs to assume path could be invalid unless this change is applied.

[andres-asm]

I asked but I didn't get a straight reply.
@Alcaro @twinaphex @Themaister

[andres-asm]

@garbear as you can see, no-one other than people like me or mark who don't really call the shots gives a damn, so I dunno what to tell you.

I've said what I had to say, I hope you find a solution (this core is not really all that important but the API issue is).

I'm unsubscribing from this conversation.

¯_(ツ)_/¯

[garbear]

@markwkidd thanks for clarifying the documentation. It now states:

    * If need_fullpath is false and retro_load_game() is called:
    *    - retro_game_info::path may be NULL
    *    - retro_game_info::data and retro_game_info::size are guaranteed
    *      to be valid

Currently, Beetle GBA sets need_fullpath is set to false, which clearly violates the libretro API, because it discards the condition path may be NULL.

@retro-wertz or @twinaphex can you explain why this change breaks the core?

[ghost]

https://github.com/libretro/beetle-gba-libretro/blob/master/libretro.cpp#L3798

[garbear]

Previous line accesses path: https://github.com/libretro/beetle-gba-libretro/blob/master/libretro.cpp#L3794

[ghost]

if you look at what set_basename() does, it has nothing to do with loading the rom. its only there to fetch the base filename, which for the most, the path is valid whether need_fullpath is set to true of false.

…

On Sun, Jul 29, 2018 at 8:29 AM, Garrett Brown ***@***.***> wrote: Previous line accesses path: https://github.com/libretro/ beetle-gba-libretro/blob/master/libretro.cpp#L3794 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#33 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWPDtlMasd1LXDq8_wdkL2XvDLAAi6Szks5uLQH_gaJpZM4VC2fH> .

[garbear]

It doesn't matter what set_basename() does, it can't be passed an invalid parameter. For the most is fine, but Kodi is an edge case that conforms to the libretro API.

[andres-asm]

Exactly, @garbear is following the docs, RA just does differently...

Honestly retro_game_info.path should be mandated to be not null because the filename is required for stuff like... saves in many cases.

If we can't change the API to reflect that then what should happen is that cores like this that don't use retro_get_memory_size/data should check if the path is empty, and if it is NULL they should come with a fallback mechanism to name their saves, maybe CRC the memory blob or whatever.

This function uses info->path

static void set_basename(const char *path)
{
   const char *base = strrchr(path, '/');
   if (!base)
      base = strrchr(path, '\\');

   if (base)
      retro_base_name = base + 1;
   else
      retro_base_name = path;

   retro_base_name = retro_base_name.substr(0, retro_base_name.find_last_of('.'));
}

And later on retro_base_name is used to determine the save filename

   switch (type)
   {
      case MDFNMKF_SAV:
         ret = retro_save_directory +slash + retro_base_name + std::string(".") +

So I guess in a case like this, set_basename must be made smarter, should check if info->path is not null (nor garbage somehow...). If it is null (or garbage) then it should use something else to name the save.

[garbear]

+1 to your idea. I could modify Kodi to behave like RetroArch, but we're too close to a release to risk that change now. In the short term I'll disable loading with no path for now to prevent segfaults in our upcoming release.

[andres-asm]

@Alcaro @twinaphex @Themaister

[Alcaro]

no, stop pinging me

[markwkidd]

I have given my best shot at writing a PR to libretro-common/libretro.h in order to return the content filename in the ::path when need_fullpath is false. I do this purely in the hopes of helping conversation.

[andres-asm]

thanks @markwkidd sadly the people with veto power in the API are not commenting so idk.

[markwkidd]

If anyone else is still interested in this topic, I found some information in the 2012 libretro spec about this behavior: libretro/libretro-common#83 (comment)

Essentially, the 2012 libretro spec states that if content was loaded from a file, even if need_fullpath is false, the path will be passed through the callback.