Re: [linux-minidisc] "netmdcli send" does not break on errors

Thomas Arp <manner.moe@gmx.de> · Fri, 23 Sep 2011 20:19:42 +0200

Am 22/09/2011 22:02, schrieb Michael Karcher:

You might want to factor out this pattern:

+    error = netmd_secure_enter_session(session->devh);
+    level = (error ? NETMD_LOG_ERROR : NETMD_LOG_DEBUG);
+    netmd_log(level, "netmd_secure_enter_session: %s\n", netmd_strerror(error));
+    if(error)
+        return NETMD_SESSION_FAILED;

to just read

     if(netmd_log_check(netmd_secure_enter_session(session->devh),
                        "netmd_secure_enter_session"))
       return NETMD_SESSION_FAILED;

with a function netmd_log_check remaining to be written.


O.K., sounds good. As some functions might produce logs of NETMD_LOG_WARNING type on errors i also added netmd_loglevel as a parameter to netmd_log_check() to set the correct loglevel for this function.


Why do you add a netmd_receive_track function stub to session.c? We
already have a corecctly working track receiption code in secure.c, ...

This seems not to work correctly on windows. Download with SonicStage 

and the python scripts work fine, but if i try netmdcli i got following:

F:\minidisc\netmdcli\debug>netmdcli -v 3 recv 12 test.wav
Net MD/Hi-MD
.
.
52 49 46 46 .... (writing wav header)   /* works fine */
.
.
0,0%
0,0%
0,0%
0,0%
0,0%
0,0%
0,0%
0,0%
0,0%
0,0%
0,0%
0,0%
0,0%
^C
F:\minidisc\netmdcli\debug>

My RH1 stuck showing "standby", i had to break the code and 

remove/reinsert the disc got get access to it.

This was when trying to recieve the LP2.wav test file i successfully 

downloaded before. But i am working on finding the error.




So from my point of view, there is no serious reason not to commit
your code if you fix the argument handling, remove the bad strerror
invocation and drop the pointless "receive in session" stub. Remaining
issues can be fixed later. (or discussed to not get "fixed" at all)

Your code has been tested to work on the Sony MZ-RH1 and the Sharp
IM-MT899.

Regards,
   Michael Karcher


I fixed this in the attached patch.

Thomas

From 41b5232c2b1499c3243e8d56aec5cc19f7a69822 Mon Sep 17 00:00:00 2001
From: Thomas Arp <manner.moe@gmx.de>
Date: Fri, 23 Sep 2011 19:30:54 +0200
Subject: [PATCH 2/2] fixed option parsing in netmdcli, fixed log message in
 netmd_transfer_song_packets, removed unneeded
 netmd_recieve_track function in session.c, implemented
 netmd_log_check function, moved some functions inside
 libnetmd source and header files

---
 libnetmd/libnetmd.c      |   19 ++++++
 libnetmd/libnetmd.h      |    9 +++
 libnetmd/log.c           |   10 +++
 libnetmd/log.h           |   13 ++++
 libnetmd/playercontrol.c |   35 +++++++++++
 libnetmd/playercontrol.h |    9 +++
 libnetmd/secure.c        |    2 +-
 libnetmd/session.c       |  152 ++++++++++++----------------------------------
 libnetmd/session.h       |   28 ---------
 netmdcli/netmdcli.c      |    8 +-
 10 files changed, 139 insertions(+), 146 deletions(-)

diff --git a/libnetmd/libnetmd.c b/libnetmd/libnetmd.c
index beba7bd..b4040a7 100644
--- a/libnetmd/libnetmd.c
+++ b/libnetmd/libnetmd.c
@@ -289,6 +289,25 @@ static void set_group_data(minidisc* md, const int group, const char* const name
     return;
 }
 
+int netmd_get_disc_flags(netmd_dev_handle* dev)
+{
+    int ret = -1;
+    unsigned char disc_flags_request[] = {0x00, 0x18, 0x06, 0x01, 0x10, 0x10,
+                                          0x00, 0xff, 0x00, 0x00, 0x01, 0x00,
+                                          0x0b  };
+    unsigned char response[14];
+
+    ret = netmd_exch_message(dev, disc_flags_request, 0x0d, response);
+    if(ret < 0 || response[0] != NETMD_STATUS_ACCEPTED)
+    {
+        netmd_log(NETMD_LOG_WARNING, "netmd_get_disc_flags failed\n");
+        return 0;
+    }
+
+    netmd_log(NETMD_LOG_DEBUG, "netmd_get_disc_flags succeeded, disc flags: 0x%02x\n", response[13]);
+    return response[13] & 0xff;
+}
+
 /* Sonys code is utter bile. So far we've encountered the following first segments in the disc title:
  *
  * 0[-n];<title>// - titled disc.
diff --git a/libnetmd/libnetmd.h b/libnetmd/libnetmd.h
index 9e33900..b4eb989 100644
--- a/libnetmd/libnetmd.h
+++ b/libnetmd/libnetmd.h
@@ -136,6 +136,15 @@ int netmd_set_group_title(netmd_dev_handle* dev, minidisc* md, unsigned int grou
 int netmd_move_track(netmd_dev_handle* dev, const uint16_t start, const uint16_t finish);
 
 /**
+   Get disc flags to check if disc is write protected.
+
+   @param dev pointer to device returned by netmd_open
+   @return bitfield representing the disc flags, see NETMD_DISC_FLAG_* constants,
+           0 on error
+*/
+int netmd_get_disc_flags(netmd_dev_handle *dev);
+
+/**
    sets up buffer containing group info.
 
   @param dev pointer to device returned by netmd_open
diff --git a/libnetmd/log.c b/libnetmd/log.c
index 2ba3a2b..9a137f0 100644
--- a/libnetmd/log.c
+++ b/libnetmd/log.c
@@ -26,6 +26,7 @@
 #include <stdarg.h>
 
 #include "log.h"
+#include "error.h"
 
 static netmd_loglevel trace_level = 0;
 
@@ -99,3 +100,12 @@ void netmd_log(netmd_loglevel level, const char* const fmt, ...)
     vprintf(fmt, arg);
     va_end(arg);
 }
+
+int netmd_log_check(netmd_error error, netmd_loglevel errorlvl, const char *message)
+{
+    netmd_loglevel level = (error ? errorlvl : NETMD_LOG_DEBUG);
+
+    netmd_log(level, "%s: %s\n", message, netmd_strerror(error));
+
+    return (error ? 1 : 0);
+}
diff --git a/libnetmd/log.h b/libnetmd/log.h
index a72bca5..1b70511 100644
--- a/libnetmd/log.h
+++ b/libnetmd/log.h
@@ -1,6 +1,8 @@
 #ifndef LIBNETMD_TRACE_H
 #define LIBNETMD_TRACE_H
 
+#include "error.h"
+
 /**
    log level
 */
@@ -51,4 +53,15 @@ void netmd_log_hex(netmd_loglevel level, const unsigned char* const data, const
 */
 void netmd_log(netmd_loglevel level, const char* const fmt, ...);
 
+/**
+   Like netmd_log() but allows to switch log level according to the libnetmd error code.
+   Uses netmd_log() internally and NETMD_LOG_DEBUG at NETMD_NO_ERROR
+
+   @param error libnetmd error code
+   @param level Log level to use if error != NETMD_NO_ERROR.
+   @param message string containing libnetmd function which returns error
+   @return 0 if error == NETMD_NO_ERROR else 1
+*/
+int netmd_log_check(netmd_error error, netmd_loglevel errorlvl, const char *message);
+
 #endif
diff --git a/libnetmd/playercontrol.c b/libnetmd/playercontrol.c
index bc332a5..623ba39 100644
--- a/libnetmd/playercontrol.c
+++ b/libnetmd/playercontrol.c
@@ -25,6 +25,7 @@
 #include "playercontrol.h"
 #include "utils.h"
 #include "const.h"
+#include "log.h"
 
 
 static netmd_error netmd_playback_control(netmd_dev_handle* dev, unsigned char code)
@@ -221,3 +222,37 @@ netmd_error netmd_get_disc_capacity(netmd_dev_handle* dev, netmd_disc_capacity*
 
     return NETMD_NO_ERROR;
 }
+
+uint16_t netmd_get_recording_parameters(netmd_dev_handle *dev)
+{
+    int ret = -1;
+    char *enc;
+    unsigned char rec_param_request[] = {0x00, 0x18, 0x09, 0x80, 0x01, 0x03, 0x30, 0x88,
+                                         0x01, 0x00, 0x30, 0x88, 0x05, 0x00, 0x30, 0x88,
+                                         0x07, 0x00, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00};
+    unsigned char response[38];
+
+    ret = netmd_exch_message(dev, rec_param_request, 0x18, response);
+    if(ret < 0 || response[0] != NETMD_STATUS_ACCEPTED)
+    {
+        netmd_log(NETMD_LOG_WARNING, "netmd_get_recording_parameters failed\n");
+        return 0;
+    }
+
+    /* following switch case clause not really needed, just for debug output */
+    switch (response[34]) {
+    case NETMD_ENCODING_SP :
+        enc = ((response[35] & 1) ? "SP MONO" : "SP STEREO");
+        break;
+    case NETMD_ENCODING_LP2 :
+        enc = "LP2";
+        break;
+    case NETMD_ENCODING_LP4 :
+        enc = "LP4";
+        break;
+    default : enc = "unknown";
+    }
+
+    netmd_log(NETMD_LOG_DEBUG, "netmd_get_recording_parameters succeeded, encoding: %s\n", enc);
+    return (response[34] << 8) + response[35];;
+}
diff --git a/libnetmd/playercontrol.h b/libnetmd/playercontrol.h
index 41a6c03..d57856c 100644
--- a/libnetmd/playercontrol.h
+++ b/libnetmd/playercontrol.h
@@ -146,4 +146,13 @@ netmd_error netmd_get_position(netmd_dev_handle* dev, netmd_time* time);
 netmd_error netmd_get_disc_capacity(netmd_dev_handle* dev,
                                     netmd_disc_capacity* capacity);
 
+/**
+   Get recording parameters.
+
+   @param dev pointer to device returned by netmd_open
+   @return current encoding the device uses for recording, see NETMD_ENCODING_*
+           and NETMD_CHANNELS_* constants, 0 on error
+*/
+uint16_t netmd_get_recording_parameters(netmd_dev_handle *dev);
+
 #endif
diff --git a/libnetmd/secure.c b/libnetmd/secure.c
index 9314d3b..257aad8 100644
--- a/libnetmd/secure.c
+++ b/libnetmd/secure.c
@@ -372,7 +372,7 @@ void netmd_transfer_song_packets(netmd_dev_handle *dev,
 
         /* ... send it */
         error = libusb_bulk_transfer((libusb_device_handle*)dev, 2, packet, (int)packet_size, &transferred, 10000);
-        netmd_log(NETMD_LOG_DEBUG, "Packets transferred: %d %s\n", packet_size, strerror(error));
+        netmd_log(NETMD_LOG_DEBUG, "Packets transferred: %d, libusb errorcode: %d\n", packet_size, error);
 
         /* cleanup */
         free(packet);
diff --git a/libnetmd/session.c b/libnetmd/session.c
index b75c531..c4cdcd0 100644
--- a/libnetmd/session.c
+++ b/libnetmd/session.c
@@ -107,59 +107,6 @@ static void retailmac(unsigned char *rootkey, unsigned char *hostnonce,
     gcry_cipher_close(handle2);
 }
 
-int netmd_get_disc_flags(netmd_dev_handle* dev)
-{
-    int ret = -1;
-    unsigned char disc_flags_request[] = {0x00, 0x18, 0x06, 0x01, 0x10, 0x10,
-                                          0x00, 0xff, 0x00, 0x00, 0x01, 0x00,
-                                          0x0b  };
-    unsigned char response[14];
-
-    ret = netmd_exch_message(dev, disc_flags_request, 0x0d, response);
-    if(ret < 0 || response[0] != NETMD_STATUS_ACCEPTED)
-    {
-        netmd_log(NETMD_LOG_WARNING, "netmd_get_disc_flags failed\n");
-        return 0;
-    }
-
-    netmd_log(NETMD_LOG_DEBUG, "netmd_get_disc_flags succeeded, disc flags: 0x%02x\n", response[13]);
-    return response[13] & 0xff;
-}
-
-uint16_t netmd_get_recording_parameters(netmd_dev_handle *dev)
-{
-    int ret = -1;
-    char *enc;
-    unsigned char rec_param_request[] = {0x00, 0x18, 0x09, 0x80, 0x01, 0x03, 0x30, 0x88,
-                                         0x01, 0x00, 0x30, 0x88, 0x05, 0x00, 0x30, 0x88,
-                                         0x07, 0x00, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00};
-    unsigned char response[38];
-
-    ret = netmd_exch_message(dev, rec_param_request, 0x18, response);
-    if(ret < 0 || response[0] != NETMD_STATUS_ACCEPTED)
-    {
-        netmd_log(NETMD_LOG_WARNING, "netmd_get_recording_parameters failed\n");
-        return 0;
-    }
-
-    /* following switch case clause not really needed, just for debug output */
-    switch (response[34]) {
-    case NETMD_ENCODING_SP :
-        enc = ((response[35] & 1) ? "SP MONO" : "SP STEREO");
-        break;
-    case NETMD_ENCODING_LP2 :
-        enc = "LP2";
-        break;
-    case NETMD_ENCODING_LP4 :
-        enc = "LP4";
-        break;
-    default : enc = "unknown";
-    }
-
-    netmd_log(NETMD_LOG_DEBUG, "netmd_get_recording_parameters succeeded, encoding: %s\n", enc);
-    return (response[34] << 8) + response[35];;
-}
-
 netmd_error netmd_download_possible(netmd_dev_handle *dev, const char *file, netmd_wireformat wireformat)
 {
     size_t dicsflags;
@@ -187,8 +134,6 @@ netmd_error netmd_download_possible(netmd_dev_handle *dev, const char *file, net
 
 netmd_error netmd_start_session(netmd_session *session, netmd_dev_handle *dev)
 {
-    netmd_error error;
-    netmd_loglevel level;
     netmd_ekb ekb;
     netmd_keychain *keychain = NULL;
     netmd_keychain *next = NULL;
@@ -208,24 +153,20 @@ netmd_error netmd_start_session(netmd_session *session, netmd_dev_handle *dev)
     netmd_secure_session_key_forget(session->devh);
     netmd_secure_leave_session(session->devh);
 
-    error = netmd_secure_set_track_protection(session->devh, 0x01);
-    if(error)
-        netmd_log(NETMD_LOG_WARNING, "netmd_secure_set_track_protection not supported on this device\n");
-    else
-        netmd_log(NETMD_LOG_DEBUG, "netmd_secure_set_track_protection succeeded\n");
+    netmd_log_check(netmd_secure_set_track_protection(session->devh, 0x01),
+                    NETMD_LOG_WARNING,
+                    "netmd_secure_set_track_protection");
 
-    error = netmd_secure_enter_session(session->devh);
-    level = (error ? NETMD_LOG_ERROR : NETMD_LOG_DEBUG);
-    netmd_log(level, "netmd_secure_enter_session: %s\n", netmd_strerror(error));
-    if(error)
+    if(netmd_log_check(netmd_secure_enter_session(session->devh),
+                       NETMD_LOG_ERROR,
+                       "netmd_secure_enter_session"))
         return NETMD_SESSION_FAILED;
 
     build_ekb(&ekb, keychain, next);
 
-    error = netmd_secure_send_key_data(session->devh, &ekb);
-    level = (error ? NETMD_LOG_ERROR : NETMD_LOG_DEBUG);
-    netmd_log(level, "netmd_secure_send_key_data: %s\n", netmd_strerror(error));
-    if(error)
+    if(netmd_log_check(netmd_secure_send_key_data(session->devh, &ekb),
+                       NETMD_LOG_ERROR,
+                       "netmd_secure_send_key_data"))
     {
         netmd_leave_session(session);
         return NETMD_SESSION_FAILED;
@@ -235,10 +176,9 @@ netmd_error netmd_start_session(netmd_session *session, netmd_dev_handle *dev)
 
     /* exchange nonces */
     gcry_create_nonce(hostnonce, sizeof(hostnonce));
-    error = netmd_secure_session_key_exchange(session->devh, hostnonce, devnonce);
-    level = (error ? NETMD_LOG_ERROR : NETMD_LOG_DEBUG);
-    netmd_log(level, "netmd_secure_session_key_exchange: %s\n", netmd_strerror(error));
-    if(error)
+    if(netmd_log_check(netmd_secure_session_key_exchange(session->devh, hostnonce, devnonce),
+                       NETMD_LOG_ERROR,
+                       "netmd_secure_session_key_exchange"))
     {
         netmd_leave_session(session);
         return NETMD_SESSION_FAILED;
@@ -254,17 +194,13 @@ netmd_error netmd_start_session(netmd_session *session, netmd_dev_handle *dev)
 
 void netmd_leave_session(netmd_session * session)
 {
-    netmd_error error;
-    netmd_loglevel level;
+    netmd_log_check(netmd_secure_session_key_forget(session->devh),
+                    NETMD_LOG_WARNING,
+                    "netmd_secure_session_key_forget");
 
-    error = netmd_secure_session_key_forget(session->devh);
-    level = (error ? NETMD_LOG_WARNING : NETMD_LOG_DEBUG);
-    netmd_log(level, "netmd_secure_session_key_forget: %s\n", netmd_strerror(error));
-
-    /* leave session */
-    error = netmd_secure_leave_session(session->devh);
-    level = (error ? NETMD_LOG_WARNING : NETMD_LOG_DEBUG);
-    netmd_log(level, "netmd_secure_session_key_forget: %s\n", netmd_strerror(error));
+    netmd_log_check(netmd_secure_leave_session(session->devh),
+                    NETMD_LOG_WARNING,
+                    "netmd_secure_leave_session");
 
     session->active = 0;
     memset(session->sessionkey, 0x0, 8);
@@ -275,8 +211,6 @@ void netmd_leave_session(netmd_session * session)
 netmd_error netmd_send_track(netmd_session *session, const char *file, const char *title)
 {
     FILE *f;
-    netmd_error error;
-    netmd_loglevel level;
     netmd_track_packets *packets = NULL;
     size_t packet_count = 0;
     struct stat stat_buf;
@@ -297,10 +231,9 @@ netmd_error netmd_send_track(netmd_session *session, const char *file, const cha
         return NETMD_SESSION_NOT_INITIALIZED;
     }
 
-    error = netmd_secure_setup_download(session->devh, contentid, kek, session->sessionkey);
-    level = (error ? NETMD_LOG_ERROR : NETMD_LOG_DEBUG);
-    netmd_log(level, "netmd_secure_setup_download: %s\n", netmd_strerror(error));
-    if(error)
+    if(netmd_log_check(netmd_secure_setup_download(session->devh, contentid, kek, session->sessionkey),
+                       NETMD_LOG_ERROR,
+                       "netmd_secure_setup_download"))
         return NETMD_SESSION_TRANSFER_FAILED;
 
     /* read source */
@@ -313,26 +246,25 @@ netmd_error netmd_send_track(netmd_session *session, const char *file, const cha
     fclose(f);
     frames = (data_size - 60) / 192;
 
-    error = netmd_prepare_packets(data, data_size-60, &packets, &packet_count, kek);
-    level = (error ? NETMD_LOG_ERROR : NETMD_LOG_DEBUG);
-    netmd_log(level, "netmd_prepare_packets: %s\n", netmd_strerror(error));
-    if(error)
+    if(netmd_log_check(netmd_prepare_packets(data, data_size-60, &packets, &packet_count, kek),
+                       NETMD_LOG_ERROR,
+                       "netmd_prepare_packets"))
         return NETMD_SESSION_TRANSFER_FAILED;
 
     /* send to device */
-    error = netmd_secure_send_track(session->devh, NETMD_WIREFORMAT_LP2,
-                                    NETMD_DISKFORMAT_LP2,
-                                    frames, packets,
-                                    packet_count, session->sessionkey,
-                                    &track, uuid, new_contentid);
-    level = (error ? NETMD_LOG_ERROR : NETMD_LOG_DEBUG);
-    netmd_log(level, "netmd_secure_send_track: %s\n", netmd_strerror(error));
-
-    /* cleanup */
-    netmd_cleanup_packets(&packets);
-
-    if(error)
+    if(netmd_log_check(netmd_secure_send_track(session->devh, NETMD_WIREFORMAT_LP2,
+                                               NETMD_DISKFORMAT_LP2,
+                                               frames, packets,
+                                               packet_count, session->sessionkey,
+                                               &track, uuid, new_contentid),
+                       NETMD_LOG_ERROR,
+                       "netmd_secure_send_track"))
+    {
+        netmd_cleanup_packets(&packets);
         return NETMD_SESSION_TRANSFER_FAILED;
+    }
+
+    netmd_cleanup_packets(&packets);
 
     /* set title */
     netmd_log(NETMD_LOG_DEBUG, "New Track: %d\n", track);
@@ -344,19 +276,13 @@ netmd_error netmd_send_track(netmd_session *session, const char *file, const cha
     netmd_sync_toc(session->devh);
 
     /* commit track */
-    error = netmd_secure_commit_track(session->devh, track, session->sessionkey);
-    level = (error ? NETMD_LOG_ERROR : NETMD_LOG_DEBUG);
-    netmd_log(level, "netmd_secure_commit_track: %s\n", netmd_strerror(error));
-    if(error)
+    if(netmd_log_check(netmd_secure_commit_track(session->devh, track, session->sessionkey),
+                       NETMD_LOG_ERROR,
+                       "netmd_secure_commit_track"))
         return NETMD_SESSION_TRANSFER_FAILED;
 
     netmd_log(NETMD_LOG_DEBUG, "netmd_send_track succeeded\n");
     return NETMD_NO_ERROR;
 }
 
-netmd_error netmd_recieve_track(netmd_session *session, size_t track, const char * file)
-{
-    /* TODO: implement track upload */
-    return NETMD_NOT_IMPLEMENTED;
-}
 
diff --git a/libnetmd/session.h b/libnetmd/session.h
index 7d57d10..ff58bbe 100644
--- a/libnetmd/session.h
+++ b/libnetmd/session.h
@@ -14,24 +14,6 @@ typedef struct netmd_session {
 } netmd_session;
 
 /**
-   Get disc flags to check if disc is write protected.
-
-   @param dev pointer to device returned by netmd_open
-   @return bitfield representing the disc flags, see NETMD_DISC_FLAG_* constants,
-           0 on error
-*/
-int netmd_get_disc_flags(netmd_dev_handle *dev);
-
-/**
-   Get recording parameters.
-
-   @param dev pointer to device returned by netmd_open
-   @return current encoding the device uses for recording, see NETMD_ENCODING_*
-           and NETMD_CHANNELS_* constants, 0 on error
-*/
-uint16_t netmd_get_recording_parameters(netmd_dev_handle *dev);
-
-/**
    Check if download is possible, disc write protected? enough free space ?.
 
    @param dev pointer to device returned by netmd_open
@@ -68,14 +50,4 @@ void netmd_leave_session(netmd_session *session);
 */
 netmd_error netmd_send_track(netmd_session *session, const char *file, const char *title);
 
-/**
-   Recieve a track from the device usind a secure session (Sony MZ-RH1 only).
-
-   @param session pointer to a netmd_session struct initialized by netmd_start_session
-   @param track track number
-   @param file audio filename to store the track
-   @return NETMD_NO_ERROR on success else error code of the last failed function
-*/
-netmd_error netmd_recieve_track(netmd_session *session, size_t track, const char * file);
-
 #endif
diff --git a/netmdcli/netmdcli.c b/netmdcli/netmdcli.c
index 6dd3a8a..8377830 100644
--- a/netmdcli/netmdcli.c
+++ b/netmdcli/netmdcli.c
@@ -197,13 +197,13 @@ int main(int argc, char* argv[])
 
     /* parse options */
     while (1) {
-        c = getopt(argc, argv, "v");
+        c = getopt(argc, argv, "v:");
         if (c == -1) {
             break;
         }
         switch (c) {
         case 'v':
-            l = argv[optind][0];
+            l = optarg[0];
             switch (l) {
             case '0' :
                 netmd_set_log_level(NETMD_LOG_NONE);
@@ -232,8 +232,8 @@ int main(int argc, char* argv[])
     }
 
     /* update argv and argc after parsing options */
-    argv = &argv[optind];
-    argc -= (optind);
+    argv = &argv[optind - 1];
+    argc -= (optind - 1);
 
     /* parse commands */
     if(argc > 1)
-- 
1.7.6.msysgit.0

