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