On Tue, Jul 30, 2019 at 4:59 PM Frediano Ziglio <fziglio@xxxxxxxxxx> wrote: > > > > > On Tue, Jul 30, 2019 at 1:43 PM Frediano Ziglio <fziglio@xxxxxxxxxx> wrote: > > > > > > > > > > > This module contains implementation of emulated device > > > > interface for shared CD. > > > > > > > > Signed-off-by: Yuri Benditovich <yuri.benditovich@xxxxxxxxxx> > > > > --- > > > > src/usb-device-cd.c | 794 ++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 794 insertions(+) > > > > create mode 100644 src/usb-device-cd.c > > > > > > > > diff --git a/src/usb-device-cd.c b/src/usb-device-cd.c > > > > new file mode 100644 > > > > index 0000000..18dbcea > > > > --- /dev/null > > > > +++ b/src/usb-device-cd.c > > > > @@ -0,0 +1,794 @@ > > > > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */ > > > > +/* > > > > + Copyright (C) 2019 Red Hat, Inc. > > > > + > > > > + Red Hat Authors: > > > > + Yuri Benditovich<ybendito@xxxxxxxxxx> > > > > + > > > > + This library is free software; you can redistribute it and/or > > > > + modify it under the terms of the GNU Lesser General Public > > > > + License as published by the Free Software Foundation; either > > > > + version 2.1 of the License, or (at your option) any later version. > > > > + > > > > + This library is distributed in the hope that it will be useful, > > > > + but WITHOUT ANY WARRANTY; without even the implied warranty of > > > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > > > + Lesser General Public License for more details. > > > > + > > > > + You should have received a copy of the GNU Lesser General Public > > > > + License along with this library; if not, see > > > > <http://www.gnu.org/licenses/>. > > > > +*/ > > > > + > > > > +#include "config.h" > > > > + > > > > +#ifdef USE_USBREDIR > > > > + > > > > +#include <glib-object.h> > > > > +#include <inttypes.h> > > > > +#include <gio/gio.h> > > > > +#include <glib/gi18n-lib.h> > > > > +#include <errno.h> > > > > +#include <libusb.h> > > > > +#include <fcntl.h> > > > > + > > > > +#ifdef G_OS_WIN32 > > > > +#include <windows.h> > > > > +#include <ntddcdrm.h> > > > > +#include <ntddmmc.h> > > > > +#else > > > > +#include <sys/stat.h> > > > > +#include <sys/ioctl.h> > > > > +#include <linux/fs.h> > > > > +#include <linux/cdrom.h> > > > > +#endif > > > > + > > > > +#include "usb-emulation.h" > > > > +#include "cd-usb-bulk-msd.h" > > > > + > > > > +typedef struct SpiceCdLU > > > > +{ > > > > + char *filename; > > > > + GFile *file_object; > > > > + GFileInputStream *stream; > > > > + uint64_t size; > > > > + uint32_t blockSize; > > > > + uint32_t loaded : 1; > > > > + uint32_t device : 1; > > > > +} SpiceCdLU; > > > > + > > > > +#define MAX_LUN_PER_DEVICE 1 > > > > +#define USB2_BCD 0x200 > > > > +#define CD_DEV_VID 0x2b23 > > > > +#define CD_DEV_PID 0xCDCD > > > > > > Where these values came? Later you are stating vendor > > > is "Red Hat" but Red Hat is 0x1b36 and Qumranet is 0x1af4. > > > Are these temporary values? > > > > I suppose you mean PCI vendor IDs. USB one come from another source. > > https://bugzilla.redhat.com/show_bug.cgi?id=1132330 > > > > Ok, worth a mention for the VID. > And the PID ? Was this 0xCDCD registered somehow? Not yet. Our todo to allocate it (inside Red Hat). > > > > > > > > +#define CD_DEV_CLASS 8 > > > > +#define CD_DEV_SUBCLASS 6 > > > > +#define CD_DEV_PROTOCOL 0x50 > > > > +#define CD_DEV_BLOCK_SIZE 0x200 > > > > > > I didn't expected this. I though all CDs were 2048 sector sized. > > > > As far as I remember, I've checked that during development, CDFS has > > sector of 512 bytes. > > > > > > > > > +#define CD_DEV_MAX_SIZE 737280000 > > > > > > This is unused. I would remove I suppose DVD devices or images > > > won't fit here anyway. > > > > When the size is bigger than regular CD image, sector size is 2K. > > As far as I remember, other FS used in this case (UDF etc) > > won't fir where? This is CD-DVD reader, we used it with large images. > > > > A DVD can be larger. By the way, this constant is not used in the code. > OK, this is my fault, indeed the constant can be removed (just forgotten that I've changed the logic of 512/2K). When the image size can be divided to 2K, there is no problem to report 2K sector. The problem was with small images that require 512 bytes. > > > > > > > +#define DVD_DEV_BLOCK_SIZE 0x800 > > > > +#define MAX_BULK_IN_REQUESTS 64 > > > > + > > > > +struct BufferedBulkRead > > > > +{ > > > > + struct usb_redir_bulk_packet_header hout; > > > > + uint64_t id; > > > > +}; > > > > + > > > > +struct _SpiceUsbEmulatedDevice > > > > +{ > > > > + UsbDeviceOps dev_ops; > > > > + SpiceUsbBackend *backend; > > > > + SpiceUsbBackendDevice *parent; > > > > + struct usbredirparser *parser; > > > > + void * msc; > > > > + SpiceCdLU units[MAX_LUN_PER_DEVICE]; > > > > + gboolean locked; > > > > + gboolean delete_on_eject; > > > > + gboolean deleting; > > > > + uint32_t num_reads; > > > > + struct BufferedBulkRead read_bulk[MAX_BULK_IN_REQUESTS]; > > > > + uint16_t serial[4]; > > > > > > Specifications state that serial should be at least 12 characters. > > > > Which specification? I do not remember in USB spec such requirement. > > > > https://www.usb.org/sites/default/files/usbmassbulk_10.pdf Indeed. Worth to change. > > > > > > > > + uint8_t max_lun_index; > > > > +}; > > > > + > > > > +typedef SpiceUsbEmulatedDevice UsbCd; > > > > + > > > > +#ifndef G_OS_WIN32 > > > > + > > > > +static int cd_device_open_stream(SpiceCdLU *unit, const char *filename) > > > > +{ > > > > + int error = 0; > > > > + unit->device = 0; > > > > + > > > > + if (!unit->filename && !filename) { > > > > + SPICE_DEBUG("%s: file name not provided", __FUNCTION__); > > > > + return -1; > > > > + } > > > > + if (unit->filename && filename) { > > > > + g_free(unit->filename); > > > > + unit->filename = NULL; > > > > + } > > > > + if (filename) { > > > > + unit->filename = g_strdup(filename); > > > > + } > > > > + > > > > + int fd = open(unit->filename, O_RDONLY | O_NONBLOCK); > > > > + if (fd > 0) { > > > > + struct stat file_stat = { 0 }; > > > > + if (fstat(fd, &file_stat) || file_stat.st_size == 0) { > > > > + file_stat.st_size = 0; > > > > + unit->device = 1; > > > > + if (!ioctl(fd, BLKGETSIZE64, &file_stat.st_size) && > > > > + !ioctl(fd, BLKSSZGET, &unit->blockSize)) { > > > > + } > > > > + } > > > > + unit->size = file_stat.st_size; > > > > + close(fd); > > > > + if (unit->size) { > > > > + unit->file_object = g_file_new_for_path(unit->filename); > > > > + unit->stream = g_file_read(unit->file_object, NULL, NULL); > > > > + } > > > > + if (!unit->stream) { > > > > + SPICE_DEBUG("%s: can't open stream on %s", __FUNCTION__, > > > > unit->filename); > > > > + g_object_unref(unit->file_object); > > > > + unit->file_object = NULL; > > > > + error = -1; > > > > + } > > > > + } > > > > + else { > > > > + SPICE_DEBUG("%s: can't open file %s", __FUNCTION__, > > > > unit->filename); > > > > + error = -1; > > > > + } > > > > + > > > > + return error; > > > > +} > > > > + > > > > +static int cd_device_load(SpiceCdLU *unit, gboolean load) > > > > +{ > > > > + int error; > > > > + if (!unit->device || !unit->filename) { > > > > + return -1; > > > > + } > > > > + int fd = open(unit->filename, O_RDONLY | O_NONBLOCK); > > > > + if (fd < 0) { > > > > + return -1; > > > > + } > > > > + if (load) { > > > > + error = ioctl(fd, CDROMCLOSETRAY, 0); > > > > + } else { > > > > + error = ioctl(fd, CDROM_LOCKDOOR, 0); > > > > + error = ioctl(fd, CDROMEJECT, 0); > > > > + } > > > > + if (error) { > > > > + // note that ejecting might be available only for root > > > > + // loading might be available also for regular user > > > > + SPICE_DEBUG("%s: can't %sload %s, res %d, errno %d", > > > > + __FUNCTION__, load ? "" : "un", unit->filename, error, > > > > errno); > > > > + } > > > > + close(fd); > > > > + return error; > > > > +} > > > > + > > > > +static int cd_device_check(SpiceCdLU *unit) > > > > +{ > > > > + int error; > > > > + if (!unit->device || !unit->filename) { > > > > + return -1; > > > > + } > > > > + int fd = open(unit->filename, O_RDONLY | O_NONBLOCK); > > > > + if (fd < 0) { > > > > + return -1; > > > > + } > > > > + error = ioctl(fd, CDROM_DRIVE_STATUS, 0); > > > > + error = (error == CDS_DISC_OK) ? 0 : -1; > > > > + if (!error) { > > > > + error = ioctl(fd, CDROM_DISC_STATUS, 0); > > > > + error = (error == CDS_DATA_1) ? 0 : -1; > > > > + } > > > > + close(fd); > > > > + return error; > > > > +} > > > > + > > > > +#else > > > > + > > > > +static gboolean is_device_name(const char *filename) > > > > +{ > > > > + gboolean b = strlen(filename) == 2 && filename[1] == ':'; > > > > + return b; > > > > > > Here "::" or "0:" are accepted. I have a follow up for this. > > > > > > > +} > > > > + > > > > +static HANDLE open_file(const char *filename) > > > > +{ > > > > + HANDLE h = CreateFileA(filename, > > > > > > Minor but maybe we want to convert from utf8 (usually used by > > > GLib) to utf16 and use CreateFileW. > > > > > > > + GENERIC_READ, > > > > + FILE_SHARE_READ | FILE_SHARE_WRITE, > > > > > > This will allow other program to change the ISO while the program is > > > running. Is this wanted? I know it's compatible with Unix somehow but > > > I think usually on Windows you don't expect this. > > > But honestly is just a minor. > > > > > > > + NULL, OPEN_EXISTING, > > > > + 0, > > > > + NULL); > > > > + if (h == INVALID_HANDLE_VALUE) { > > > > + h = NULL; > > > > + } > > > > + return h; > > > > +} > > > > + > > > > +static uint32_t ioctl_out(HANDLE h, uint32_t code, void *out_buffer, > > > > uint32_t out_size) > > > > +{ > > > > + uint32_t error; > > > > + DWORD ret; > > > > + BOOL b = DeviceIoControl(h, code, NULL, 0, out_buffer, out_size, > > > > &ret, > > > > NULL); > > > > + error = b ? 0 : GetLastError(); > > > > + return error; > > > > +} > > > > + > > > > +static uint32_t ioctl_none(HANDLE h, uint32_t code) > > > > +{ > > > > + return ioctl_out(h, code, NULL, 0); > > > > +} > > > > + > > > > +static gboolean check_device(HANDLE h) > > > > +{ > > > > + GET_CONFIGURATION_IOCTL_INPUT cfgIn = > > > > + { FeatureCdRead, SCSI_GET_CONFIGURATION_REQUEST_TYPE_ALL, {} }; > > > > + DWORD ret; > > > > + GET_CONFIGURATION_HEADER cfgOut; > > > > + return DeviceIoControl(h, IOCTL_CDROM_GET_CONFIGURATION, > > > > + &cfgIn, sizeof(cfgIn), &cfgOut, > > > > sizeof(cfgOut), > > > > + &ret, NULL); > > > > +} > > > > + > > > > +static int cd_device_open_stream(SpiceCdLU *unit, const char *filename) > > > > +{ > > > > + int error = 0; > > > > + HANDLE h; > > > > + unit->device = 0; > > > > + if (!unit->filename && !filename) { > > > > + SPICE_DEBUG("%s: unnamed file", __FUNCTION__); > > > > + return -1; > > > > + } > > > > + if (unit->filename && filename) { > > > > + g_free(unit->filename); > > > > + unit->filename = NULL; > > > > + } > > > > + if (!filename) { > > > > + // reopening the stream on existing file name > > > > + } else if (is_device_name(filename)) { > > > > + unit->filename = g_strdup_printf("\\\\.\\%s", filename); > > > > + } else { > > > > + unit->filename = g_strdup(filename); > > > > + } > > > > + h = open_file(unit->filename); > > > > + if (h) { > > > > + LARGE_INTEGER size = { 0 }; > > > > + if (!GetFileSizeEx(h, &size)) { > > > > + uint64_t buffer[256]; > > > > + uint32_t res; > > > > + unit->device = check_device(h); > > > > + SPICE_DEBUG("%s: CD device %srecognized on %s", > > > > + __FUNCTION__, unit->device ? "" : "NOT ", > > > > unit->filename); > > > > + res = ioctl_out(h, IOCTL_DISK_GET_DRIVE_GEOMETRY_EX, > > > > + buffer, sizeof(buffer)); > > > > + if (!res) > > > > + { > > > > + DISK_GEOMETRY_EX *pg = (DISK_GEOMETRY_EX *)buffer; > > > > + unit->blockSize = pg->Geometry.BytesPerSector; > > > > + size = pg->DiskSize; > > > > + } else { > > > > + SPICE_DEBUG("%s: can't obtain size of %s (error %u)", > > > > + __FUNCTION__, unit->filename, res); > > > > + } > > > > + } > > > > + unit->size = size.QuadPart; > > > > + CloseHandle(h); > > > > + if (unit->size) { > > > > + unit->file_object = g_file_new_for_path(unit->filename); > > > > + unit->stream = g_file_read(unit->file_object, NULL, NULL); > > > > + } > > > > + if (!unit->stream) { > > > > + SPICE_DEBUG("%s: can't open stream on %s", __FUNCTION__, > > > > unit->filename); > > > > + g_object_unref(unit->file_object); > > > > + unit->file_object = NULL; > > > > + error = -1; > > > > + } > > > > + } else { > > > > + SPICE_DEBUG("%s: can't open file %s", __FUNCTION__, > > > > unit->filename); > > > > + error = -1; > > > > + } > > > > + return error; > > > > +} > > > > + > > > > +static int cd_device_load(SpiceCdLU *unit, gboolean load) > > > > +{ > > > > + int error = 0; > > > > + HANDLE h; > > > > + if (!unit->device || !unit->filename) { > > > > + return -1; > > > > + } > > > > + h = open_file(unit->filename); > > > > + if (h) { > > > > + uint32_t res = ioctl_none(h, load ? IOCTL_STORAGE_LOAD_MEDIA : > > > > IOCTL_STORAGE_EJECT_MEDIA); > > > > + if (res) { > > > > + SPICE_DEBUG("%s: can't %sload %s, win error %u", > > > > + __FUNCTION__, load ? "" : "un", unit->filename, > > > > res); > > > > + error = -1; > > > > + } else { > > > > + SPICE_DEBUG("%s: device %s [%s]", > > > > + __FUNCTION__, load ? "loaded" : "ejected", > > > > unit->filename); > > > > + } > > > > + CloseHandle(h); > > > > + } > > > > + return error; > > > > +} > > > > + > > > > +static int cd_device_check(SpiceCdLU *unit) > > > > +{ > > > > + int error = 0; > > > > + CDROM_DISK_DATA data; > > > > + HANDLE h; > > > > + if (!unit->device || !unit->filename) { > > > > + return -1; > > > > + } > > > > + h = open_file(unit->filename); > > > > + if (h) { > > > > + uint32_t res = ioctl_none(h, IOCTL_STORAGE_CHECK_VERIFY); > > > > + if (!res) { > > > > + res = ioctl_out(h, IOCTL_CDROM_DISK_TYPE, &data, > > > > sizeof(data)); > > > > + } > > > > + if (res != 0 || data.DiskData != CDROM_DISK_DATA_TRACK) { > > > > + error = -1; > > > > + } > > > > + CloseHandle(h); > > > > + } > > > > + return error; > > > > +} > > > > + > > > > +#endif > > > > + > > > > +static gboolean open_stream(SpiceCdLU *unit, const char *filename) > > > > +{ > > > > + gboolean b = FALSE; > > > > + b = cd_device_open_stream(unit, filename) == 0; > > > > + return b; > > > > +} > > > > + > > > > +static void close_stream(SpiceCdLU *unit) > > > > +{ > > > > + if (unit->stream) { > > > > + g_object_unref(unit->stream); > > > > + unit->stream = NULL; > > > > + } > > > > + if (unit->file_object) { > > > > + g_object_unref(unit->file_object); > > > > + unit->file_object = NULL; > > > > + } > > > > +} > > > > + > > > > +static gboolean load_lun(UsbCd *d, int unit, gboolean load) > > > > +{ > > > > + gboolean b = TRUE; > > > > + if (load && d->units[unit].device) { > > > > + // there is one possible problem in case our backend is the > > > > + // local CD device and it is ejected > > > > + cd_device_load(&d->units[unit], TRUE); > > > > + close_stream(&d->units[unit]); > > > > + if (cd_device_check(&d->units[unit]) || > > > > !open_stream(&d->units[unit], NULL)) { > > > > + return FALSE; > > > > + } > > > > + } > > > > + > > > > + if (load) { > > > > + CdScsiMediaParameters media_params = { 0 }; > > > > + > > > > + media_params.stream = d->units[unit].stream; > > > > + media_params.size = d->units[unit].size; > > > > + media_params.block_size = d->units[unit].blockSize; > > > > + if (media_params.block_size == CD_DEV_BLOCK_SIZE && > > > > + media_params.size % DVD_DEV_BLOCK_SIZE == 0) { > > > > + media_params.block_size = DVD_DEV_BLOCK_SIZE; > > > > + } > > > > + SPICE_DEBUG("%s: loading %s, size %" PRIu64 ", block %u", > > > > + __FUNCTION__, d->units[unit].filename, > > > > media_params.size, media_params.block_size); > > > > + > > > > + b = !cd_usb_bulk_msd_load(d->msc, unit, &media_params); > > > > + > > > > + d->units[unit].loaded = !!b; > > > > + > > > > + } else { > > > > + SPICE_DEBUG("%s: unloading %s", __FUNCTION__, > > > > d->units[unit].filename); > > > > + cd_usb_bulk_msd_unload(d->msc, unit); > > > > + d->units[unit].loaded = FALSE; > > > > + } > > > > + return b; > > > > +} > > > > + > > > > +static void usb_cd_delete(UsbCd *d) > > > > +{ > > > > + uint32_t unit = 0; > > > > > > Why unit is always 0? Here and in other places. > > > Maybe worth adding a mnemonic? > > > > Because currently we define the device as single-unit. > > But the code of MSD/SCSI in general supports multiple units. > > > > > > > > > + cd_usb_bulk_msd_unrealize(d->msc, unit); > > > > + g_clear_pointer(&d->units[unit].filename, g_free); > > > > + close_stream(&d->units[unit]); > > > > + g_clear_pointer(&d->msc, cd_usb_bulk_msd_free); > > > > + g_free(d); > > > > +} > > > > + > > > > +static gboolean usb_cd_get_descriptor(UsbCd *d, uint8_t type, uint8_t > > > > index, > > > > + void **buffer, uint16_t *size) > > > > +{ > > > > + static struct libusb_device_descriptor desc = > > > > + { > > > > + .bLength = 18, > > > > + .bDescriptorType = LIBUSB_DT_DEVICE, > > > > + .bcdUSB = USB2_BCD, > > > > + .bDeviceClass = 0, > > > > + .bDeviceSubClass = 0, > > > > + .bDeviceProtocol = 0, > > > > + .bMaxPacketSize0 = 64, > > > > + .idVendor = CD_DEV_VID, > > > > + .idProduct = CD_DEV_PID, > > > > + .bcdDevice = 0x100, > > > > + .iManufacturer = 1, > > > > + .iProduct = 2, > > > > + .iSerialNumber = 3, > > > > + .bNumConfigurations = 1 > > > > + }; > > > > > > So are we expecting the caller to be able to change these static > > > structures? > > > Both function declaration and data ones are suggesting it. > > > > > > > + static uint8_t cfg[] = > > > > + { > > > > + 9, //len of cfg desc > > > > + LIBUSB_DT_CONFIG, // desc type > > > > + 0x20, // wlen > > > > + 0, > > > > + 1, // num if > > > > + 1, // cfg val > > > > + 0, // cfg name > > > > + 0x80, // bus powered > > > > + 0x32, // 100 ma > > > > + 9, // len of IF desc > > > > + LIBUSB_DT_INTERFACE, > > > > + 0, // num if > > > > + 0, // alt setting > > > > + 2, // num of endpoints > > > > + CD_DEV_CLASS, > > > > + CD_DEV_SUBCLASS, > > > > + CD_DEV_PROTOCOL, > > > > + 0, // if name > > > > + 7, > > > > + LIBUSB_DT_ENDPOINT, > > > > + 0x81, //->Direction : IN - EndpointID : 1 > > > > + 0x02, //->Bulk Transfer Type > > > > + 0, //wMaxPacketSize : 0x0200 = 0x200 max bytes > > > > + 2, > > > > + 0, //bInterval > > > > + 7, > > > > + LIBUSB_DT_ENDPOINT, > > > > + 0x02, //->Direction : OUT - EndpointID : 2 > > > > + 0x02, //->Bulk Transfer Type > > > > + 0, //wMaxPacketSize : 0x0200 = 0x200 max bytes > > > > + 2, > > > > + 0, //bInterval > > > > + }; > > > > + static uint16_t s0[2] = { 0x304, 0x409 }; > > > > + static uint16_t s1[8] = { 0x310, 'R', 'e', 'd', ' ', 'H', 'a', 't' > > > > }; > > > > + static uint16_t s2[9] = { 0x312, 'S', 'p', 'i', 'c', 'e', ' ', 'C', > > > > 'D' > > > > > > Minor: these will work only on little endian, not a big issue at the moment > > > but > > > I saw other code dealing with big endian machines. > > > > > Currently I do not have any possibility to test the code on big-endian > > machine. > > > > > > }; > > > > + > > > > + void *p = NULL; > > > > + uint16_t len = 0; > > > > + > > > > + switch (type) > > > > + { > > > > + case LIBUSB_DT_DEVICE: > > > > + p = &desc; > > > > + len = sizeof(desc); > > > > + break; > > > > + case LIBUSB_DT_CONFIG: > > > > + p = cfg; > > > > + len = sizeof(cfg); > > > > + break; > > > > + case LIBUSB_DT_STRING: > > > > + if (index == 0) { > > > > + p = s0; len = sizeof(s0); > > > > + } else if (index == 1) { > > > > + p = s1; len = sizeof(s1); > > > > + } else if (index == 2) { > > > > + p = s2; len = sizeof(s2); > > > > + } else if (index == 3) { > > > > + p = d->serial; len = sizeof(d->serial); > > > > + } > > > > + break; > > > > + } > > > > + > > > > + if (p) { > > > > + *buffer = p; > > > > + *size = len; > > > > + } > > > > + > > > > + return p != NULL; > > > > +} > > > > + > > > > +static void usb_cd_attach(UsbCd *device, struct usbredirparser *parser) > > > > +{ > > > > + device->parser = parser; > > > > +} > > > > + > > > > +static void usb_cd_detach(UsbCd *device) > > > > +{ > > > > + device->parser = NULL; > > > > +} > > > > + > > > > +static void usb_cd_reset(UsbCd *device) > > > > +{ > > > > + cd_usb_bulk_msd_reset(device->msc); > > > > +} > > > > + > > > > +static void usb_cd_control_request(UsbCd *device, > > > > + uint8_t *data, int data_len, > > > > + struct > > > > usb_redir_control_packet_header > > > > *h, > > > > + void **buffer) > > > > +{ > > > > + uint8_t reqtype = h->requesttype & 0x7f; > > > > + if (!device->msc) { > > > > + return; > > > > + } > > > > + if (reqtype == (LIBUSB_REQUEST_TYPE_STANDARD | > > > > LIBUSB_RECIPIENT_ENDPOINT)) { > > > > + // might be clear stall request > > > > + h->length = 0; > > > > + h->status = usb_redir_success;; > > > > + return; > > > > + } > > > > + > > > > + if (reqtype == (LIBUSB_REQUEST_TYPE_CLASS | > > > > LIBUSB_RECIPIENT_INTERFACE)) > > > > { > > > > + switch (h->request) { > > > > + case 0xFF: > > > > + // mass-storage class request 'reset' > > > > + usb_cd_reset(device); > > > > + h->length = 0; > > > > + h->status = usb_redir_success; > > > > + break; > > > > + case 0xFE: > > > > + // mass-storage class request 'get max lun' > > > > + // returning one byte > > > > + if (h->length) { > > > > + h->length = 1; > > > > + h->status = usb_redir_success;; > > > > + *buffer = &device->max_lun_index; > > > > + } > > > > + break; > > > > + } > > > > + } > > > > +} > > > > + > > > > +static void usb_cd_bulk_out_request(UsbCd *device, > > > > + uint8_t ep, uint8_t *data, int > > > > data_len, > > > > + uint8_t *status) > > > > +{ > > > > + if (!cd_usb_bulk_msd_write(device->msc, data, data_len)) { > > > > + *status = usb_redir_success; > > > > + } > > > > +} > > > > + > > > > +void cd_usb_bulk_msd_read_complete(void *user_data, > > > > + uint8_t *data, uint32_t length, CdUsbBulkStatus status) > > > > +{ > > > > + UsbCd *d = (UsbCd *)user_data; > > > > + > > > > + if (d->deleting) { > > > > + d->deleting = FALSE; > > > > + spice_usb_backend_device_eject(d->backend, d->parent); > > > > > > I would place a return after this, the parent is deleted and potentially > > > all this structure too. > > > > Nobody is deleted, just indication issued and the delete will happen > > on next idle. > > We need to respond the USB request. > > > > > > > > > + } > > > > + > > > > + if (d->parser) { > > > > + int nread; > > > > + uint32_t offset = 0; > > > > + > > > > + for (nread = 0; nread < d->num_reads; nread++) { > > > > + uint32_t max_len; > > > > + max_len = (d->read_bulk[nread].hout.length_high << 16) | > > > > + d->read_bulk[nread].hout.length; > > > > + if (max_len > length) { > > > > + max_len = length; > > > > + d->read_bulk[nread].hout.length = length; > > > > + d->read_bulk[nread].hout.length_high = length >> 16; > > > > + } > > > > + > > > > + switch (status) { > > > > + case BULK_STATUS_GOOD: > > > > + d->read_bulk[nread].hout.status = usb_redir_success; > > > > + break; > > > > + case BULK_STATUS_CANCELED: > > > > + d->read_bulk[nread].hout.status = usb_redir_cancelled; > > > > + break; > > > > + case BULK_STATUS_ERROR: > > > > + d->read_bulk[nread].hout.status = usb_redir_ioerror; > > > > + break; > > > > + case BULK_STATUS_STALL: > > > > + default: > > > > + d->read_bulk[nread].hout.status = usb_redir_stall; > > > > + break; > > > > + } > > > > + > > > > + SPICE_DEBUG("%s: responding %" PRIu64 " with len %u out of > > > > %u, > > > > status %d", > > > > + __FUNCTION__, d->read_bulk[nread].id, max_len, > > > > + length, d->read_bulk[nread].hout.status); > > > > + usbredirparser_send_bulk_packet(d->parser, > > > > d->read_bulk[nread].id, > > > > + &d->read_bulk[nread].hout, > > > > + max_len ? (data + offset) : > > > > NULL, > > > > + max_len); > > > > + offset += max_len; > > > > + length -= max_len; > > > > + } > > > > + d->num_reads = 0; > > > > + usbredirparser_do_write(d->parser); > > > > + > > > > + if (length) { > > > > + SPICE_DEBUG("%s: ERROR: %u bytes were not reported!", > > > > __FUNCTION__, length); > > > > + } > > > > + > > > > + } else { > > > > + SPICE_DEBUG("%s: broken device<->channel relationship!", > > > > __FUNCTION__); > > > > + } > > > > +} > > > > + > > > > +/* device reset completion callback */ > > > > +void cd_usb_bulk_msd_reset_complete(void *user_data, int status) > > > > +{ > > > > + // UsbCd *d = (UsbCd *)user_data; > > > > +} > > > > + > > > > +static gboolean usb_cd_bulk_in_request(UsbCd *d, uint64_t id, > > > > + struct usb_redir_bulk_packet_header > > > > *h) > > > > +{ > > > > + int res; > > > > + uint32_t len = (h->length_high << 16) | h->length; > > > > + if (d->num_reads >= MAX_BULK_IN_REQUESTS) { > > > > + h->length = h->length_high = 0; > > > > + SPICE_DEBUG("%s: too many pending reads", __FUNCTION__); > > > > + h->status = usb_redir_babble; > > > > + return FALSE; > > > > + } > > > > + > > > > + if (d->num_reads) { > > > > + SPICE_DEBUG("%s: already has %u pending reads", __FUNCTION__, > > > > d->num_reads); > > > > + } > > > > + > > > > + d->read_bulk[d->num_reads].hout = *h; > > > > + d->read_bulk[d->num_reads].id = id; > > > > + d->num_reads++; > > > > + res = cd_usb_bulk_msd_read(d->msc, len); > > > > + if (!res) { > > > > + return TRUE; > > > > + } > > > > + > > > > + SPICE_DEBUG("%s: error on bulk read", __FUNCTION__); > > > > + d->num_reads--; > > > > + h->length = h->length_high = 0; > > > > + h->status = usb_redir_ioerror; > > > > + > > > > + return FALSE; > > > > +} > > > > + > > > > +static void usb_cd_cancel_request(UsbCd *d, uint64_t id) > > > > +{ > > > > + uint32_t nread; > > > > + > > > > + for (nread = 0; nread < d->num_reads; nread++) { > > > > + if (d->read_bulk[nread].id == id) { > > > > + if (cd_usb_bulk_msd_cancel_read(d->msc)) { > > > > + cd_usb_bulk_msd_read_complete(d, NULL, 0, > > > > BULK_STATUS_CANCELED); > > > > + } > > > > + return; > > > > + } > > > > + } > > > > + SPICE_DEBUG("%s: ERROR: no such id to cancel!", __FUNCTION__); > > > > +} > > > > + > > > > +// called when a change happens on SCSI layer > > > > +void cd_usb_bulk_msd_lun_changed(void *user_data, uint32_t lun) > > > > +{ > > > > + UsbCd *d = (UsbCd *)user_data; > > > > + CdScsiDeviceInfo cd_info; > > > > + > > > > + if (!cd_usb_bulk_msd_get_info(d->msc, lun, &cd_info)) { > > > > + // load or unload command received from SCSI > > > > + if (d->units[lun].loaded != cd_info.loaded) { > > > > + if (!load_lun(d, lun, cd_info.loaded)) { > > > > + SPICE_DEBUG("%s: load failed, unloading unit", > > > > __FUNCTION__); > > > > + cd_usb_bulk_msd_unload(d->msc, lun); > > > > + } > > > > + } > > > > + } > > > > + > > > > + if (d->delete_on_eject) { > > > > + d->delete_on_eject = FALSE; > > > > + d->deleting = TRUE; > > > > + } else { > > > > + spice_usb_backend_device_report_change(d->backend, d->parent); > > > > + } > > > > +} > > > > + > > > > +static gchar *usb_cd_get_product_description(UsbCd *device) > > > > +{ > > > > + gchar *base_name = g_path_get_basename(device->units[0].filename); > > > > + gchar *res = g_strdup_printf("SPICE CD(%s)", base_name); > > > > > > Maybe leave a space between "SPICE CD" and the file name? > > > On the GUI does not appear that great without. > > > > No problem > > > > > > > > > + g_free(base_name); > > > > + return res; > > > > +} > > > > + > > > > +static const UsbDeviceOps devops = > > > > +{ > > > > + .get_descriptor = usb_cd_get_descriptor, > > > > + .get_product_description = usb_cd_get_product_description, > > > > + .attach = usb_cd_attach, > > > > + .reset = usb_cd_reset, > > > > + .detach = usb_cd_detach, > > > > + .control_request = usb_cd_control_request, > > > > + .bulk_out_request = usb_cd_bulk_out_request, > > > > + .bulk_in_request = usb_cd_bulk_in_request, > > > > + .cancel_request = usb_cd_cancel_request, > > > > + .delete = usb_cd_delete, > > > > +}; > > > > + > > > > +static int usb_cd_create(SpiceUsbBackend *be, > > > > + SpiceUsbBackendDevice *parent, > > > > + UsbCreateDeviceParameters *param, > > > > + UsbCd **dev) > > > > +{ > > > > + int error = 0; > > > > > > Is this result value used or planned to be used? Looks like we use > > > (and the caller too) like a boolean. > > > > > > > + uint32_t unit = 0; > > > > > > This is the other "unit" constant set to 0 (comment above). > > > > The code is in general was prepared also to work also with > > multiple-unit devices. > > Switch from single-unit to multiple-unit, if needed, will be very simple. > > > > > > > > > + UsbCd *d = g_new0(UsbCd, 1); > > > > + CdScsiDeviceParameters dev_params = { 0 }; > > > > + > > > > + d->dev_ops = devops; > > > > + d->backend = be; > > > > + d->parent = parent; > > > > + d->delete_on_eject = > > > > !!param->device_param.create_cd.delete_on_eject; > > > > + d->locked = !d->delete_on_eject; > > > > + d->serial[0] = 0x0308; > > > > + d->serial[1] = 'X'; > > > > > > Specifications state that value characters are 0-9 and A-F (capital case > > > hexadecimal, yes quite a waste of space I would say). > > > > Which spec? > > This is serial string for USB to distinguish between multiple CDs. > > > > See above for link to specs > > > > > > > > + d->serial[2] = '0' + param->address / 10; > > > > + d->serial[3] = '0' + param->address % 10; > > > > + d->max_lun_index = MAX_LUN_PER_DEVICE - 1; > > > > + > > > > + dev_params.vendor = "SPICE"; > > > > + dev_params.product = "shared CD"; > > > > > > Why not "Red Hat" and "SPICE CD" even here? > > > > Everything you want. They are SCSI parameters. > > > > > > > > > + dev_params.version = "0"; > > > > + > > > > + d->msc = cd_usb_bulk_msd_alloc(d, MAX_LUN_PER_DEVICE); > > > > + if (!d->msc) { > > > > + g_clear_pointer(&d, g_free); > > > > + param->error = g_error_new(SPICE_CLIENT_ERROR, > > > > SPICE_CLIENT_ERROR_FAILED, > > > > + _("can't allocate device")); > > > > + return 1; > > > > + } > > > > + d->units[unit].blockSize = CD_DEV_BLOCK_SIZE; > > > > + if (!cd_usb_bulk_msd_realize(d->msc, unit, &dev_params)) { > > > > + if (open_stream(&d->units[unit], > > > > param->device_param.create_cd.filename) && > > > > + load_lun(d, unit, TRUE)) { > > > > + if (d->locked) { > > > > + cd_usb_bulk_msd_lock(d->msc, unit, TRUE); > > > > + } > > > > + } else { > > > > + close_stream(&d->units[unit]); > > > > + cd_usb_bulk_msd_unrealize(d->msc, unit); > > > > + param->error = g_error_new(SPICE_CLIENT_ERROR, > > > > SPICE_CLIENT_ERROR_FAILED, > > > > + _("can't create device with %s"), > > > > + > > > > param->device_param.create_cd.filename); > > > > + error = 3; > > > > + } > > > > + } else { > > > > + error = 2; > > > > + param->error = g_error_new(SPICE_CLIENT_ERROR, > > > > SPICE_CLIENT_ERROR_FAILED, > > > > + _("can't allocate device")); > > > > + } > > > > + if (error) { > > > > + g_clear_pointer(&d->msc, cd_usb_bulk_msd_free); > > > > + g_clear_pointer(&d, g_free); > > > > + } > > > > + > > > > + *dev = d; > > > > + return error; > > > > +} > > > > + > > > > +void spice_usb_device_register_cd(SpiceUsbBackend *be) > > > > +{ > > > > + spice_usb_backend_register_device_type(be, USB_DEV_TYPE_CD, > > > > usb_cd_create); > > > > +} > > > > + > > > > +#endif /* USE_USBREDIR */ > > > > > > By the way, preparing some follow ups. > > > > > > Frediano > > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel