Re: [linux-minidisc] changed himdformat.c to work on linux and windows

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

Am Freitag, den 02.04.2010, 23:34 +0200 schrieb manner.moe@gmx.de:
> I´ve implemented himd format feature in libhimd and qhimdtransfer
> using libsgutils2.
Thanks for the patch, it mostly looks great, please don't get
discouraged by my comments below. It's not that I think everything you
did your way is wrong, but just what I think might be done a bit better
that it is currently done.

> This is for test purposes only wether we are not sure if we can use 
> libsgutils2 on mac. If not we can use libusal/libscg instead, but the
> usage is a bit more complicated.
Alas, it looks like there is no libsgutils2 on MacOS X.

> The scsi device name is set by the detection code so it works on
> windows only yet because of missing code for the other OSes.
That's correct, but not a problem for your patch.

> +int himdscsi_send_format_cmd(int open_device, struct scsi_error * error)
While the scsi_error structure is more detailed than himd_status, you
should think about using himd_status here. Also, as this code is plain
C, I am thinking about putting it into libhimd instead of having it in
QHiMDTransfer.

> +void himdscsi_wait_for_unit_ready(int open_device)
> +{
> +    int status;
> +    unsigned char command[12];
> +    struct sg_pt_base *ptvp = NULL;
> +
> +    memset(command,0,12);
Please use static initialization here, like "static const unsigned char
command[12] = {0,0,0,0,0,0,0,0,0,0,0,0}". You can even omit all the
zeroes for static initialization, as statically constructed objects are
implicitly zero-initialized. Finally, the test unit ready command is
only 6 bytes. Do you really need 12 here?

> +    do
> +    {
> +        if (ptvp)
> +            destruct_scsi_pt_obj(ptvp);
> +        ptvp = construct_scsi_pt_obj();
> +        set_scsi_pt_cdb(ptvp, command, sizeof(command));
> +        status = do_scsi_pt(ptvp, open_device, SCSI_TIMEOUT, 0);
> +    }
> +    while(get_scsi_pt_result_category(ptvp) != SCSI_PT_RESULT_GOOD);
> +
> +    if (ptvp)
> +        destruct_scsi_pt_obj(ptvp);
You don't need the if here. ptvp shouldn't be NULL, anyway, as the loop
runs at least one iteration. And if construct_scsi_pt_obj failed and
returned NULL, I expect set_scsi_pt_cdb to already have crashed. I would
suggest to change your logic. You do not destruct ptvp, because you need
the status in the loop exit condition, und thus you have to put the
destruction at the beginning of the next iteration. I think it would be
clearer if you just copy the result of get_scsi_pt_result_category to a
temporary variable, destory the ptvp and then check the result. That
even makes it possible to put the ptvp variable inside the loop, as the
value doesn't need to be preserved across iterations.



> --- /dev/null
> +++ b/libhimd/himd_scsi.h
> @@ -0,0 +1,25 @@
> +#ifndef INCLUDED_LIBHIMD_SCSI_H
> +#define INCLUDED_LIBHIMD_SCSI_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +struct scsi_error {
> +    char * error_msg;
> +    char status[128];
> +    char sense[1024];
> +};
> +
> +#define SCSI_TIMEOUT 20
Why do you put this into the header file? Can code snippets that use the
himdscsi functions make any use of this value? I would suggest to put
this define at the top of himd_scsi.c


> -HEADERS += himd.h himd_private.h sony_oma.h
> -SOURCES += encryption.c himd.c mdstream.c trackindex.c sony_oma.c frag.c
> -LIBS    += -lmad -lmcrypt
> +HEADERS += himd.h himd_private.h sony_oma.h himd_scsi.h
> +SOURCES += encryption.c himd.c mdstream.c trackindex.c sony_oma.c frag.c himd_scsi.c
> +LIBS    += -lmad -lmcrypt -lsgutils2
Add the scsi stuff conditionally, take a look at libhimd.pro for the
disabling of mad/mcrypt on how to do that.


> +void QHiMDFormatDialog::on_buttonBox_accepted()
> +{
> +    int sg_open = -1;
> +    struct scsi_error error;
> +    QString errmsg;
> +    QMessageBox formatStatus;
> +
> +    if(device == NULL)
> +    {
> +        errmsg = tr("himd device is not present");
> +        goto clean;
> +    }
Hmm. Do we really need this extra logic step? If you put the scsi_device
string into the object instead of a pointer to the device structure, you
are independent of the lifetime of the device structure...

> +    if(device->scsi_device.isEmpty())
> +    {
> +        errmsg = tr("No scsi device information available");
> +        goto clean;
> +    }
You should not have the scsi_device string a public variable, in my
oppinion, but have it set by the constructor or maybe a setter function,
so you can assume that it is valid here, or, even better, open the
device already when the formatting dialog is opened. This should mark
the device as in-use for the "safely remove hardware" function in
Windows, and most importantly it would not show the format dialog if
formatting is not going to work.

> +    sg_open = himdscsi_open_device(device->scsi_device.toLatin1().data());
Could you use toLocal8Bit instead of toLatin1 here?


> @@ -527,6 +536,7 @@ void QHiMDMainWindow::himd_removed(QString
> HiMDPath)
>          ui->himd_devices->hide();
>          ui->himdpath->show();
>      }
> +    formatDialog->device = NULL;
I would rather have the dialog closed instead of just making it
non-functional.

Kind Regards,
  Michael Karcher

Attachment: signature.asc
Description: Dies ist ein digital signierter Nachrichtenteil