<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Sep 17, 2018 at 9:47 PM, Frediano Ziglio <span dir="ltr"><<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">> <br>
> Added implementation of system-dependent operations<br>
> defined in cd-device.h for Linux<br>
> <br>
> Signed-off-by: Yuri Benditovich <<a href="mailto:yuri.benditovich@daynix.com">yuri.benditovich@daynix.com</a>><br>
> ---<br>
> src/cd-device-linux.c | 137<br>
> ++++++++++++++++++++++++++++++<wbr>++++++++++++++++++++<br>
> 1 file changed, 137 insertions(+)<br>
> create mode 100644 src/cd-device-linux.c<br>
> <br>
> diff --git a/src/cd-device-linux.c b/src/cd-device-linux.c<br>
> new file mode 100644<br>
> index 0000000..c97e574<br>
> --- /dev/null<br>
> +++ b/src/cd-device-linux.c<br>
> @@ -0,0 +1,137 @@<br>
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */<br>
> +/*<br>
> +Copyright (C) 2018 Red Hat, Inc.<br>
> +<br>
> +Red Hat Authors:<br>
> +Yuri Benditovich<<a href="mailto:ybendito@redhat.com">ybendito@redhat.<wbr>com</a>><br>
> +<br>
> +This library is free software; you can redistribute it and/or<br>
> +modify it under the terms of the GNU Lesser General Public<br>
> +License as published by the Free Software Foundation; either<br>
> +version 2.1 of the License, or (at your option) any later version.<br>
> +<br>
> +This library is distributed in the hope that it will be useful,<br>
> +but WITHOUT ANY WARRANTY; without even the implied warranty of<br>
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU<br>
> +Lesser General Public License for more details.<br>
> +<br>
> +You should have received a copy of the GNU Lesser General Public<br>
> +License along with this library; if not, see <<a href="http://www.gnu.org/licenses/" rel="noreferrer" target="_blank">http://www.gnu.org/licenses/</a>><wbr>.<br>
> +*/<br>
> +<br>
> +#include "config.h"<br>
> +<br>
> +#include <glib-object.h><br>
> +<br>
> +#ifndef G_OS_WIN32<br>
> +#ifdef USE_USBREDIR<br>
<br>
</div></div>maybe: #if !defined(G_OS_WIN32) && defined(USE_USBREDIR)<br>
really minor, just style.<br>
<span class=""><br>
> +#include <inttypes.h><br>
> +#include <gio/gio.h><br>
> +#include "cd-device.h"<br>
> +#include "spice-client.h"<br>
> +#include <fcntl.h><br>
> +#include <errno.h><br>
> +#include <sys/stat.h><br>
> +#include <sys/ioctl.h><br>
> +#include <linux/fs.h><br>
> +#include <linux/cdrom.h><br>
<br>
</span>why the alternate mix of local and system headers?<br></blockquote><div><br></div><div>I did not see any requirement for that.</div><div>Anyway will be fixed in next round.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> +<br>
> +int cd_device_open_stream(<wbr>SpiceCdLU *unit, const char *filename)<br>
> +{<br>
> + int error = 0;<br>
> + unit->device = 0;<br>
> +<br>
> + if (!unit->filename && !filename) {<br>
> + SPICE_DEBUG("%s: file name not provided", __FUNCTION__);<br>
> + return -1; // TODO<br>
> + }<br>
> + if (unit->filename && filename) {<br>
> + g_free(unit->filename);<br>
> + unit->filename = NULL;<br>
> + }<br>
> + if (filename) {<br>
> + unit->filename = g_strdup(filename);<br>
> + }<br>
> +<br>
> + int fd = open(unit->filename, O_RDONLY | O_NONBLOCK);<br>
> + if (fd > 0) {<br>
> + struct stat file_stat = { 0 };<br>
> + if (fstat(fd, &file_stat) || file_stat.st_size == 0) {<br>
> + file_stat.st_size = 0;<br>
> + unit->device = 1;<br>
> + if (!ioctl(fd, BLKGETSIZE64, &file_stat.st_size) &&<br>
> + !ioctl(fd, BLKSSZGET, &unit->blockSize)) {<br>
<br>
</div></div>This if is weird, the result is ignored. Did you plan to implement<br>
error report?<br></blockquote><div><br></div><div>In general there is nothing to do, blank CD will behave like that and this is no reason to fail the operation.</div><div>User will need to reload the unit with better CD. Resulting size/blockSize is printed always.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> + }<br>
> + }<br>
> + unit->size = file_stat.st_size;<br>
> + close(fd);<br>
> + if (unit->size) {<br>
> + unit->file_object = g_file_new_for_path(unit-><wbr>filename);<br>
> + unit->stream = g_file_read(unit->file_object, NULL, NULL);<br>
> + }<br>
> + if (!unit->stream) {<br>
> + SPICE_DEBUG("%s: can't open stream on %s", __FUNCTION__,<br>
> unit->filename);<br>
> + g_object_unref(unit->file_<wbr>object);<br>
> + unit->file_object = NULL;<br>
> + error = -1; //TODO<br>
<br>
</span>What do you expect from these TODOs? Better/friendly error reports?<br>
To put the structure in a consistent state? Define more error codes?<br></blockquote><div><br></div><div>
<div style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial">Structure state is consistent.</div>But user-friendly error report on unit creation is in TODO list.</div><div>Currently the failure on creation is silent and this is bad.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> + }<br>
> + }<br>
> + else {<br>
> + SPICE_DEBUG("%s: can't open file %s", __FUNCTION__, unit->filename);<br>
> + error = -1; //TODO<br>
> + }<br>
> +<br>
> + return error;<br>
> +}<br>
> +<br>
> +int cd_device_load(SpiceCdLU *unit, gboolean load)<br>
> +{<br>
> + int error;<br>
> + if (!unit->device || !unit->filename) {<br>
> + return -1; //TODO<br>
> + }<br>
> + int fd = open(unit->filename, O_RDONLY | O_NONBLOCK);<br>
> + if (fd > 0) {<br>
> + if (load) {<br>
> + error = ioctl(fd, CDROMCLOSETRAY, 0);<br>
> + } else {<br>
> + error = ioctl(fd, CDROM_LOCKDOOR, 0);<br>
<br>
</span>In this statement "error" assignment is not used anymore, some<br>
compilers/checkers will probably report an error/warning (minor,<br>
not that important).<br></blockquote><div><br></div><div>In such case we will need to reference it.</div><div>The error here is not important.</div><div>Operations with the tray may require to be an admin on Linux.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
> + error = ioctl(fd, CDROMEJECT, 0);<br>
> + }<br>
> + if (error) {<br>
> + // note that ejecting might be available only for root<br>
> + // loading might be available also for regular user<br>
> + SPICE_DEBUG("%s: can't %sload %s, res %d, errno %d",<br>
> + __FUNCTION__, load ? "" : "un", unit->filename, error,<br>
> errno);<br>
> + }<br>
> + close(fd);<br>
> + } else {<br>
> + error = -1; //TODO<br>
> + }<br>
> + return error;<br>
> +}<br>
> +<br>
> +int cd_device_check(SpiceCdLU *unit)<br>
> +{<br>
> + int error;<br>
> + if (!unit->device || !unit->filename) {<br>
> + return -1; //TODO<br>
> + }<br>
> + int fd = open(unit->filename, O_RDONLY | O_NONBLOCK);<br>
> + if (fd > 0) {<br>
> + error = ioctl(fd, CDROM_DRIVE_STATUS, 0);<br>
> + error = (error == CDS_DISC_OK) ? 0 : -1;<br>
> + if (!error) {<br>
> + error = ioctl(fd, CDROM_DISC_STATUS, 0);<br>
> + error = (error == CDS_DATA_1) ? 0 : -1;<br>
> + }<br>
> + close(fd);<br>
> + }<br>
> + else {<br>
> + error = -1; //TODO<br>
> + }<br>
> + return error;<br>
> +}<br>
> +<br>
> +#endif<br>
> +#endif<br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">Frediano<br>
</font></span></blockquote></div><br></div></div>