[Spice-devel] [spice-gtk v4 02/13] cd-sharing: implement cd-device for Linux

[Yuri Benditovich]

On Mon, Sep 17, 2018 at 9:47 PM, Frediano Ziglio <fziglio at redhat.com> wrote:

> >
> > Added implementation of system-dependent operations
> > defined in cd-device.h for Linux
> >
> > Signed-off-by: Yuri Benditovich <yuri.benditovich at daynix.com>
> > ---
> >  src/cd-device-linux.c | 137
> >  ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 137 insertions(+)
> >  create mode 100644 src/cd-device-linux.c
> >
> > diff --git a/src/cd-device-linux.c b/src/cd-device-linux.c
> > new file mode 100644
> > index 0000000..c97e574
> > --- /dev/null
> > +++ b/src/cd-device-linux.c
> > @@ -0,0 +1,137 @@
> > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> > +/*
> > +Copyright (C) 2018 Red Hat, Inc.
> > +
> > +Red Hat Authors:
> > +Yuri Benditovich<ybendito at redhat.com>
> > +
> > +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"
> > +
> > +#include <glib-object.h>
> > +
> > +#ifndef G_OS_WIN32
> > +#ifdef USE_USBREDIR
>
> maybe: #if !defined(G_OS_WIN32) && defined(USE_USBREDIR)
> really minor, just style.
>
> > +#include <inttypes.h>
> > +#include <gio/gio.h>
> > +#include "cd-device.h"
> > +#include "spice-client.h"
> > +#include <fcntl.h>
> > +#include <errno.h>
> > +#include <sys/stat.h>
> > +#include <sys/ioctl.h>
> > +#include <linux/fs.h>
> > +#include <linux/cdrom.h>
>
> why the alternate mix of local and system headers?
>

I did not see any requirement for that.
Anyway will be fixed in next round.


>
> > +
> > +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; // TODO
> > +    }
> > +    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)) {
>
> This if is weird, the result is ignored. Did you plan to implement
> error report?
>

In general there is nothing to do, blank CD will behave like that and this
is no reason to fail the operation.
User will need to reload the unit with better CD. Resulting size/blockSize
is printed always.


>
> > +            }
> > +        }
> > +        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; //TODO
>
> What do you expect from these TODOs? Better/friendly error reports?
> To put the structure in a consistent state? Define more error codes?
>

Structure state is consistent.
But user-friendly error report on unit creation is in TODO list.
Currently the failure on creation is silent and this is bad.


>
> > +        }
> > +    }
> > +    else {
> > +        SPICE_DEBUG("%s: can't open file %s", __FUNCTION__,
> unit->filename);
> > +        error = -1; //TODO
> > +    }
> > +
> > +    return error;
> > +}
> > +
> > +int cd_device_load(SpiceCdLU *unit, gboolean load)
> > +{
> > +    int error;
> > +    if (!unit->device || !unit->filename) {
> > +        return -1; //TODO
> > +    }
> > +    int fd = open(unit->filename, O_RDONLY | O_NONBLOCK);
> > +    if (fd > 0) {
> > +        if (load) {
> > +            error = ioctl(fd, CDROMCLOSETRAY, 0);
> > +        } else {
> > +            error = ioctl(fd, CDROM_LOCKDOOR, 0);
>
> In this statement "error" assignment is not used anymore, some
> compilers/checkers will probably report an error/warning (minor,
> not that important).
>

In such case we will need to reference it.
The error here is not important.
Operations with the tray may require to be an admin on Linux.


>
> > +            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);
> > +    } else {
> > +        error = -1; //TODO
> > +    }
> > +    return error;
> > +}
> > +
> > +int cd_device_check(SpiceCdLU *unit)
> > +{
> > +    int error;
> > +    if (!unit->device || !unit->filename) {
> > +        return -1; //TODO
> > +    }
> > +    int fd = open(unit->filename, O_RDONLY | O_NONBLOCK);
> > +    if (fd > 0) {
> > +        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);
> > +    }
> > +    else {
> > +        error = -1; //TODO
> > +    }
> > +    return error;
> > +}
> > +
> > +#endif
> > +#endif
>
> Frediano
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180918/5c76a149/attachment-0001.html>