Skip to content
Snippets Groups Projects

VLCLibrary: add API to log debug info to a file

Closed Felix Paul Kühne requested to merge fkuehne/VLCKit:libraryfilelogging into master
7 unresolved threads

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Author Maintainer

    any feedback? :)

  • Maintainer

    @fkuehne sorry just got around to it. Do you have more reasoning as to why this is needed? I believe @tguillem already mentioned that a developer could just pipe the log into a file if needed.

  • Author Maintainer

    @caro There is currently no way to access the debug log because it is directly logged by NSLog, which is inaccessible when you remotely debug the app without Xcode attach (see the tvOS MR for reference). An alternative to the current approach of using libvlc to log to a file is to forward the messages through a callback to the client app and let that handle the further management. This is what I will implement in addition to the proposed API. Amended patch forthcoming later this week.

  • Carola
    Carola @caro started a thread on the diff
159 166 }
160 167 }
161 168
169 - (void)enableDebugLoggingToFile:(NSString *)filePath
170 {
171 if (!filePath)
172 return;
173
174 if (!_instance)
175 return;
176
177 if (_debugLogging) {
178 libvlc_log_unset(_instance);
179 }
180
181 _logFileStream = fopen([filePath UTF8String], "a");
  • Carola
  • 159 166 }
    160 167 }
    161 168
    169 - (void)enableDebugLoggingToFile:(NSString *)filePath
    170 {
    171 if (!filePath)
    172 return;
    173
    174 if (!_instance)
    175 return;
  • The API names are a bit weird. enable sounds like adding a feature, not removing the previous log handler, and enable followed by disable would need to be idempotent for me.

    Something like setLogToFile, setLogToNSLog and maybe unsetLogger might be more self-explaining if it is possible.

  • Author Maintainer

    This is a good idea, especially since I also add to add support for a custom selector dispatch to implement your own logger as a VLCKit client.

  • added 2 commits

    • c065dd7d - VLCLibrary: add API to log debug info to a file
    • cd70421b - library: add API to log to a custom target using a defined protocol

    Compare with previous version

  • added 1 commit

    • 53196630 - library: add API to log to a custom target using a defined protocol

    Compare with previous version

  • added 1 commit

    • 359e49dd - library: add API to log to a custom target using a defined protocol

    Compare with previous version

  • added 1 commit

    • 9ca1a07e - library: add API to log to a custom target using a defined protocol

    Compare with previous version

  • 222 288 }
    289
    290 static void HandleMessageForCustomTarget(void *data,
    291 int level,
    292 const libvlc_log_t *ctx,
    293 const char *fmt,
    294 va_list args)
    295 {
    296 VLCLibrary *libraryInstance = (__bridge VLCLibrary *)data;
    297 id debugLoggingTarget = libraryInstance.debugLoggingTarget;
    298
    299 if (!debugLoggingTarget) {
    300 return;
    301 }
    302
    303 char *str;
  • 290 static void HandleMessageForCustomTarget(void *data,
    291 int level,
    292 const libvlc_log_t *ctx,
    293 const char *fmt,
    294 va_list args)
    295 {
    296 VLCLibrary *libraryInstance = (__bridge VLCLibrary *)data;
    297 id debugLoggingTarget = libraryInstance.debugLoggingTarget;
    298
    299 if (!debugLoggingTarget) {
    300 return;
    301 }
    302
    303 char *str;
    304 if (vasprintf(&str, fmt, args) == -1) {
    305 if (str)
  • 303 char *str;
    304 if (vasprintf(&str, fmt, args) == -1) {
    305 if (str)
    306 free(str);
    307 str = NULL;
    308 return;
    309 }
    310
    311 if (str == NULL)
    312 return;
    313
    314 [debugLoggingTarget handleMessage:[NSString stringWithUTF8String:str] debugLevel:level];
    315
    316 free(str);
    317 str = NULL;
    318 }
    • Why do you set str to NULL in multiple places in the function when the function returns right after that? Nothing could accidentally use str anymore, or did I overlook something?

      Micro-optimization: You could use initWithBytesNoCopy:length:encoding:freeWhenDone: though likely not worth it as it only saves maybe one copy and only if the encoding matches what NSString natively uses on the system.

    • Please register or sign in to reply
  • 181 return;
    182
    183 if (!_instance)
    184 return;
    185
    186 if (_debugLogging) {
    187 libvlc_log_unset(_instance);
    188 }
    189
    190 _logFileStream = fopen([filePath UTF8String], "a");
    191
    192 if (_logFileStream) {
    193 libvlc_log_set_file(_instance, _logFileStream);
    194 _debugLogging = YES;
    195 }
    196 }
    • Calling this method multiple times will lead to the previous FILE descriptor being leaked (missing fclose), unless I am overlooking something.

      Additionally this method could have a BOOL return value to properly indicate failures, like when fopen fails.

      Another improvement, though thats more personal taste, would be to use NSURL instead of NSString for the file path. (Apple stated in Accessing Files and Directories that "The preferred way to specify the location of a file or directory is to use the NSURL class" and it seems overall more common in most APIs to use NSURLs for this.)

    • Maintainer

      agree to the bool return. if _logFileStream failed, we fail silently and the developer doesn't know.

    • Please register or sign in to reply
  • 85 107 * \param readableName Human-readable application name, e.g. "FooBar player 1.2.3"
    86 108 * \param userAgent HTTP User Agent, e.g. "FooBar/1.2.3 Python/2.6.0"
    87 109 */
    88 - (void)setHumanReadableName:(NSString *)readableName withHTTPUserAgent:(NSString *)userAgent;
    110 - (void)setHumanReadableName:(NSString * _Nullable)readableName withHTTPUserAgent:(NSString * _Nullable)userAgent;
    • Is readableName actually nullable? In libvlc this directly is passed to var_SetString and I could not figure out if it's ok to call that with a new value of NULL as the documentation does not mention it, the code does not make it clear either. I guess it is fine though, just want to make sure as in countless other places I see var_SetString(p_this, "something", (val != NULL) ? val : "");.

    • I don't know about NSString at all, but for var_SetString it can be NULL. However it must not be displayed as NULL, as printf("%s", NULL) is undefined behaviour. NULL is the default value for string variable, so is like "unset".

      That being said, they are places expecting a non-null "user-agent" (human readable name) and other checking before using it, and same for "http-user-agent". But nullable can also be used to define a default user-agent.

    • Please register or sign in to reply
  • added 1 commit

    • e8e28ec9 - library: add API to log to a custom target using a defined protocol

    Compare with previous version

  • Maintainer

    was merged with fdc3e809 & ae71c1fe and cherrypicked to 3.0 with 06fc2ac0 & a43c0496 :thumbsup:

  • closed

  • Please register or sign in to reply
    Loading