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

[Michael Karcher <Michael.Karcher@fu-berlin.de>]

Am Dienstag, den 20.09.2011, 22:59 +0200 schrieb Thomas Arp:
> Am 18/09/2011 17:50, schrieb Michael Karcher:
> > Am Sonntag, den 18.09.2011, 17:05 +0200 schrieb Thomas Arp:
> >> The key generation/exchange mechanism should go into
> >> libnetmd. Maybe we should provide something like netmd_start_session()
> >> and netmd_end_session() externally which set up and clean a secure
> >> session  inside libnetmd.
> > Indeed it should. If you are going to implement that, you might want to
> > take a look at the MDSession object in netmd/libnetmd.py. While I don't
> > want to imply this is for sure the best way to design the interface, at
> > least that interface seemed to work out quite nicely for the
> > downloadhack script and gave the impression that will work for more
> > complex implementations, too.
> >
> > Regards,
> >    Michael Karcher
> >
> O.K., these are my changes until yet, Please take a look at it.
Thanks for your efforts. Some comments follow.

Adding a trace debug level seems like a good idea.


>      while (1) {
> -        c = getopt(argc, argv, "t");
> +        c = getopt(argc, argv, "v");
>          if (c == -1) {
>              break;
>          }
>          switch (c) {
> -        case 't':
> -            netmd_set_log_level(NETMD_LOG_ALL);
> +        case 'v':
> +            l = argv[optind][0];
Please do it correctly. Your new option "v" expects an argument, so it
must be "v:" instead of "v" in the option list. If you fix that, you
will find that "argv[optind]" does not point where you expect it to
point to, because argv[optind] is what getopt is going to process next,
and if getopt knows that the number after "-v" belongs to "-v", optind
will be past the debug level. Instead use "optarg" which points to the
argument of the current option.


> -    argv = &argv[optind - 1];
> -    argc -= (optind - 1);
> +    argv = &argv[optind];
> +    argc -= (optind);
This hunk breaks the option parsing in case there is no -v option. In
fact, this is just a work-around due to your earlier mis-use of getopt.
If you fix the errors pointed out in the previous paragraph, this change
is unneeded.


> --- 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, "%d %d\n", packet_size, error);
> +        netmd_log(NETMD_LOG_DEBUG, "Packets transferred: %d %s\n", packet_size, strerror(error));
>  
>          /* cleanup */
>          free(packet);

This is not going to work as you expect. libusb error codes are not
compatible with strerror, and a libusb_strerror function does not (yet)
exist. Just output the error code number for now. See
 http://www.libusb.org/ticket/102
and
 http://libusb.6.n5.nabble.com/libusb-102-Support-the-libusb-strerror-function-td3555193.html
for background information.

I don't think net_get_disc_flags and netmd_get_recording_paramters
really belong into session.c. For net_get_disc_flags, I am quite sure
that putting it next to the disk and group info functions (that is in
libnetmd.c) is the most sensible solution. For
netmd_get_recording_parameters, I would suggest to use playercontrol.c
with the prospective of adding a "record from analog input" command,
too. AFAIK this command exists.

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.

I don't think "parsing (chopping the header)" the sound file to download
and doing the data transfer in the same function is a great idea. Thus I
propose to not have


> +    /* read source */
> +    stat(file, &stat_buf);
> +    data_size = (size_t)stat_buf.st_size;
> +    data = malloc(data_size);
> +    f = fopen(file, "rb");
> +    fseek(f, 60, SEEK_CUR);
> +    fread(data, data_size - 60, 1, f);
> +    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)
> +        return NETMD_SESSION_TRANSFER_FAILED;

inside netmd_send_track, but have it in a separate function
netmd_read_track instead (which may or may not be a part of libnetmd).
That function should also determine the codec of the file. The same
applies to netmd_download_possible which also implies that the data you
are going to send *has* to be in a named file, so you can't do a check
like this if you just know you are going to receive 3.2MB data via HTTP
streaming. As libnetmd is a general-purpose library, in my oppinion the
download mechanism should depend on the type of the data source as less
as possible.

Currently, I'm having a design in mind where you have a
netmd_audio_file_open function that determines codec and frame count and
packet layout (currently the code is using one packet only, which we
could keep for now), puts that into a netmd_data_source structure
together with a "read" callback that reads one packet worth of data.
It's the job of the "open" function to decide how the data is
packetized.

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, and
track receiption does not make use of encryption at all, so we don't
need to mess with sessions to receive tracks.

Anyhow, the design of the interface is not something that needs to be
done with this patch, on the other hand, the error checking needs to go
in. 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